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

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D71372#1781316 <https://reviews.llvm.org/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 even without logging enabled.


That's a good idea, and should be easy to do.

Since you can't really fail in constructors, the way the ThreadPlans work is that you make a thread plan, and if making it fails you leave a note to yourself that construction failed.  Then when you go to queue the thread plan, Thread::QueueThreadPlan calls ValidatePlan on the new plan, and if that returns false the plan is unshipped.  ValidatePlan takes a Stream argument as well, so the plans can write error messages.  That's how you see the "Could not create return address breakpoint" in this patch.  Finally, if QueueThreadPlan fails, CommandObjectThread::CommandObjectThreadStepWithTypeAndScope copies the Status message into the command result.

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 return address breakpoint.");
    return false;
  }

Write the error string you made in the constructor.

It's a good idea to log it as well, since the logs don't get command output so having it there will make logs easier to read.

> As for the test, you should be able to do something similar to the tests in the `test/Shell/Unwind` folder (e.g. eh-frame-dwarf-unwind.test). I.e., you could write an assembly function which uses a non-standard frame layout, but do *not* describe that layout via .eh_frame. Then, stop in that function and try to step out...
> 
> In D71372#1780508 <https://reviews.llvm.org/D71372#1780508>, @mossberg wrote:
> 
>> 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 at the `GetLoadAddressPermissions` call because 0x22 is not mapped. However, it only ended up failing when the permissions (which were empty) were checked for an execute bit. It seems to me like this might be another bug, but I'm not sure.
> 
> 
> Are you sure there is nothing mapped at that address? I'm not a darwin expert, but I have a vague knowledge that the darwin loader (or some other component of the system) actually maps a couple of pages of unreadable memory around the address zero...

Yes the whole first 32 bits of the address space get mapped with no access.  That was done to catch any 32 ->64 bit address handling goofs when macOS was going from 32 to 64 bit architectures.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71372/new/

https://reviews.llvm.org/D71372