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

ted added a comment. In D71372#1813703 , @labath wrote: > In D71372#1813142 , @ted wrote: > > > In D71372#1811594 , @labath wrote: > > > > > In D71372#1810687

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

labath added a comment. In D71372#1813142 , @ted wrote: > In D71372#1811594 , @labath wrote: > > > In D71372#1810687 , @ted wrote: > > > > > I've got another failure case

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

ted added a comment. In D71372#1811594 , @labath wrote: > In D71372#1810687 , @ted wrote: > > > I've got another failure case for this. If the remote gdbserver doesn't > > implement qMemoryRegionInfo or

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

labath added a comment. In D71372#1810687 , @ted wrote: > I've got another failure case for this. If the remote gdbserver doesn't > implement qMemoryRegionInfo or qXfer:memory-map:read, thread step-out will > fail. > That's a good point Ted. I

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

ted added a comment. I've got another failure case for this. If the remote gdbserver doesn't implement qMemoryRegionInfo or qXfer:memory-map:read, thread step-out will fail. error: Could not create return address breakpoint. Return address (0x5bc0) permissions not found. That comes from this

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

Okay, I pushed a change to the test to only run on darwin. When you have a patch for the other systems, we can take that out again. Jim > On Dec 20, 2019, at 2:08 PM, Mark Mossberg via Phabricator > wrote: > > mossberg added a comment. > > Took a quick look, and I think the good news is

[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: >

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

Yes, and Fedora Linux too. Jan added a partial fix but that doesn't get the test passing IIUC. I think Mark only had a Darwin machine to test this on. I'm going to limit the test to darwin for now to give Mark a chance to analyze the failures. Jim > On Dec 20, 2019, at 1:25 PM, Stella

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

stella.stamenova added a comment. It looks like this also caused a failure on the Windows LLDB bot: http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/11896 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'

jingham added a comment. Hey, Mark... The test you added is failing on the Debian bots, e.g.: http://lab.llvm.org:8011/builders/lldb-x86_64-debian/builds/1903 Could you take a look? 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'

jingham added a comment. I committed this (2a42a5a2f4144cd99812ad0d230480f94a1d1c92 ) but I forgot to attribute the patch to Mark in the commit message. My apologies! Repository: rG LLVM Github Monorepo CHANGES SINCE

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

This revision was automatically updated to reflect the committed changes. Closed by commit rG2a42a5a2f414 (authored by jingham). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71372/new/ https://reviews.llvm.org/D71372 Files:

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

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. LGTM 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'

jingham added a comment. This looks fine to me. We should let Greg have a final chance to weigh in and then I'll check it in. 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 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'

labath added a comment. In D71372#1785241 , @mossberg wrote: > 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

[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'

labath added a comment. The test looks good. I think it should run fine on linux, and there's a decent chance it might work on windows too. It's fine to let check it in this way, and we can add some skips later if bots start complaining. As for the location, /maybe/ ExecControl would be a

[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'

clayborg added a comment. In D71372#1782567 , @jingham wrote: > In D71372#1782549 , @clayborg wrote: > > > In D71372#1782538 , @jingham wrote: > > > > > In D71372#1782536

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

jingham added a comment. In D71372#1782549 , @clayborg wrote: > In D71372#1782538 , @jingham wrote: > > > In D71372#1782536 , @clayborg > > wrote: > > > > > I believe it is

[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'

clayborg added a comment. In D71372#1782538 , @jingham wrote: > In D71372#1782536 , @clayborg wrote: > > > I believe it is ok for permissions to succeed as long as they don't return > > invalid permissions. For

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

jingham 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'

clayborg added a comment. 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. Looking up address mappings for zero will return [0x - 0x1) --- no

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

jingham added a comment. In D71372#1782512 , @mossberg wrote: > Screenshots with new error messages. > > F11034423: image.png > > F11034433: image.png > > (I artificially forced

[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'

jingham 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. 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'

labath added a comment. 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 even without logging enabled. As for the test, you should be able to do something

[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'

jingham added a comment. Please do print out the return address in the log. That will be helpful. The LLDB_LOG* macros check the log for you. They are convenient if the thing you are going to log happen in one step. If you need to do some more logic when gathering up the log message and

[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'

clayborg requested changes to this revision. clayborg added inline comments. This revision now requires changes to proceed. Comment at: lldb/source/Target/ThreadPlanStepOut.cpp:127-129 +if (!return_address_section) { + LLDB_LOGF(log, "Return address had no section."); +

[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