[Lldb-commits] [PATCH] D66638: Unwind: Add a stack scanning mechanism to support win32 unwinding

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda accepted this revision.
jasonmolenda added a comment.
This revision is now accepted and ready to land.

Sorry for taking so long to look at this one -- it looks like a good solution to this.

Greg's idea of an additional heuristic when walking the stack and having a POSSIBLE return address, of backing up the RA and seeing if there is a call/jump instruction there is an interesting one that lldb could use in scenarios where the stack walk is going poorly and it's trying to decide whether to fall back to the architectural default unwind plan or whatever. I don't think I'd hold off on landing this to add that heuristic, but it's an interesting thing, we could add an instruction recognizer to the ABI plugin given a RA which knows how to do that, and see about using it in the unwinder.



================
Comment at: source/Plugins/Process/Utility/RegisterContextLLDB.cpp:1875
+      if (addr.SetLoadAddress(candidate, &process.GetTarget()) &&
+          addr.GetSection()->GetPermissions() & lldb::ePermissionsExecutable) {
+        address = candidate_addr;
----------------
clayborg wrote:
> WE should probably be using the memory region permissions using:
> 
> ```
> bool Process::GetLoadAddressPermissions(lldb::addr_t load_addr, uint32_t &permissions);
> ```
> 
> Why? You might have a stack going through JIT'ed code that might not have a section. But the memory region info will know about the permissions.
> 
Checking memory permissions is an interesting addition here, but not every gdb remote serial protocol stub support the memory permission packets, so I might try both.  RegisterContextLLDB::InitializeNonZerothFrame does this, for instance - it checks if there is a Module that contains the address (it could check the permissions of that section like Pavel's patch does here, too) and then it calls GetLoadAddressPermissions to see if the memory is executable.


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

https://reviews.llvm.org/D66638