[Lldb-commits] [PATCH] D71789: [lldb] Refactor thread-step-out-ret-addr-check test to use .data instead of stack variable

mossberg abandoned this revision. mossberg added a comment. Thanks all for testing this! I had no idea it was this hard to get a non-executable section :) Superseded by D71784 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D71784: Fedora Linux fails `Unwind/thread-step-out-ret-addr-check.test`

mossberg added a comment. The Windows failure (https://reviews.llvm.org/D71372#1793371) seems like it may also be caused by the underscore symbol issue. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71784/new/ https://reviews.llvm.org/D71784

[Lldb-commits] [PATCH] D71789: [lldb] Refactor thread-step-out-ret-addr-check test to use .data instead of stack variable

mossberg created this revision. mossberg added reviewers: jingham, jankratochvil, labath, clayborg, stella.stamenova. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. Original diff: D71372 Related diff: D71784

[Lldb-commits] [PATCH] D71784: Fedora Linux fails `Unwind/thread-step-out-ret-addr-check.test`

mossberg added a comment. Thanks for looking into this @jankratochvil! > Another problem is that Fedora Linux has executable stack by default and all > programs indicate non-executable stack by PT_GNU_STACK, after fixing the > underscore I was getting: > > (lldb) thread step-out > Process

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

mossberg added a comment. Took a quick look, and I think the good news is that the failure looks fairly superficial. > clang-10: warning: argument unused during compilation: >

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

mossberg added a comment. I believe all the comments should be addressed at this point; thanks very much for your reviews so far. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71372/new/ https://reviews.llvm.org/D71372

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

mossberg added a comment. I wanted to also mention that this patch won't address buggy behavior if, for example, the stub takes a function pointer on the stack (vs a normal data pointer). In this case, the executable check will succeed, and the breakpoint will be written, but to a potentially

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

mossberg marked an inline comment as done. mossberg added inline comments. Comment at: lldb/source/Target/ThreadPlanStepOut.cpp:136-137 + m_return_addr); + LLDB_LOGF(log, "ThreadPlanStepOut(%p): %s", static_cast(this), +

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

mossberg added a comment. I noticed some other tests specify the OS's they apply to-- I wasn't sure if there were any restrictions I should put on this one. I only tested on MacOS, and I imagine it should work on Linux. Not sure about Windows. Repository: rG LLVM Github Monorepo CHANGES

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

mossberg updated this revision to Diff 233708. mossberg added a comment. Add test. I wasn't sure where to put it, so I left it in the `Unwind/` directory since the `call-asm.c` infrastructure is already there, and this fix is *kind of* vaguely related to unwinding. Please let me know if

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

mossberg added a comment. In D71372#1782536 , @clayborg wrote: > I believe it is ok for permissions to succeed as long as they don't return > invalid permissions. For any address outside any mapped ranges, we should > have zero as the permissions.

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

mossberg added a comment. Screenshots with new error messages. F11034423: image.png F11034433: image.png (I artificially forced this branch to execute, still unable to make it execute at runtime) F11034446: image.png

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

mossberg updated this revision to Diff 233673. mossberg added a comment. Output error messages to console in addition to logging. I wasn't sure, but decided to unconditionally output the `Could not create return address breakpoint.` message for UX reasons. That message provides a bit of

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

mossberg added a comment. > So just add a std::string to ThreadPlanStepOut, and cons up your error > message there. Then in ThreadPlanStepOut::ValidatePlan, instead of doing: > > if (m_return_bp_id == LLDB_INVALID_BREAK_ID) { > if (error) > error->PutCString("Could not create

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

mossberg added a comment. In D71372#1781316 , @labath wrote: > I'm not sure how easy it is to do that here, but it would certainly be nice > to include these error messages in the actual error output from the "finish" > command so that they are visible

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

mossberg added a comment. Current output. F11025690: image.png F11025692: image.png Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71372/new/

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

mossberg updated this revision to Diff 233470. mossberg added a comment. Output return address in logging Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71372/new/ https://reviews.llvm.org/D71372 Files: lldb/source/Target/ThreadPlanStepOut.cpp

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

mossberg updated this revision to Diff 233466. mossberg added a comment. Remove lingering Section.h include Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71372/new/ https://reviews.llvm.org/D71372 Files: lldb/source/Target/ThreadPlanStepOut.cpp

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

mossberg added a comment. Something I noticed while updating the patch was that the `GetLoadedAddressPermissions` call would succeed, even if passed an address that is obviously not mapped. In my test case I placed an int 0x22 where the return address would be, expecting the validation to fail

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

mossberg added a comment. Here are a few screenshots of what the bug looks like: Memory corruption leading to a "crash": F11025392: image.png Silent memory corruption: F11025394: lldbbug.png Repository: rG LLVM Github

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

mossberg marked 2 inline comments as done. mossberg added inline comments. Comment at: lldb/source/Target/ThreadPlanStepOut.cpp:127-129 +if (!return_address_section) { + LLDB_LOGF(log, "Return address had no section."); + return; clayborg wrote: >

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

mossberg updated this revision to Diff 233442. mossberg added a comment. Updated to get permissions information from the Process rather than the Section. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71372/new/ https://reviews.llvm.org/D71372

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

mossberg created this revision. mossberg added reviewers: labath, jingham. mossberg added a project: LLDB. Herald added a subscriber: lldb-commits. During the 'thread step-out' command, check that the memory we are about to place a breakpoint in is 1. at an Address with a valid Section and 2. in