[Lldb-commits] [PATCH] D77000: [LLDB] [PECOFF] Only use PECallFrameInfo on the one supported architecture

Mon Mar 30 04:17:53 PDT 2020

mstorsjo added a comment.

In D77000#1949247 <https://reviews.llvm.org/D77000#1949247>, @aleksandr.urakov wrote:

> Hello! But does the format of an exception directory for aarch64 differ completely from x86-64 one?

As far as I know, it's pretty different yes. There's one format for arm32 as well (which isn't quite as thoroughly supported within llvm iirc) which I think is pretty close to the one for aarch64.

> Can we somehow adopt the existing parser to support it?

Not sure if it'd be an enhancement to the existing class or would warrant a separate class.

> If we can, I think that this check looks good (we encapsulate the difference inside `PECallFrameInfo`). Or may be it is worth to make a different parser for aarch64? Then I think the check would look better in `ObjectFilePECOFF::CreateEHCallFrameInfo` (and then we will choose a right exception directory parser depending on an architecture). What do you think on this?

In D77000#1949428 <https://reviews.llvm.org/D77000#1949428>, @labath wrote:

> I think it would make sense to put the function there (I guess you meant `ObjectFilePECOFF::CreateCallFrameInfo`) even if we later do end up adding arm support to the `PECallFrameInfo` class).

Yeah, that place does indeed look much nicer than where I placed the check initially - will update the patch.

>> Not sure how easy it is to make a sensible test for this; it makes a difference when the aarch64 RuntimeFunction struct, when interpreted as an x86_64 RuntimeFunction, happens to falsely match address ranges that the unwinder looks for
> 
> One could construct a yaml file with a "falsely matching record" and then run "image show-unwind" to demonstrate that the record is _not_ parsed. The tricky part there is that this command only works on  "live" processes (one day i'd like to change that, but right now it's hard because of... reasons). The "breakpad" unwinding tests work around that by creating a process out of a minidump core file. That might be overkill for a change of this type...

The issue where I noticed this was actually much more complicated than that. There was no visible traces of PECallFrameInfo in `image show-unwind` anywhere.

The situation I looked at was two nearly identical binaries, one with a few other libraries (not involved in the unwind) linked dynamically, one with them linked statically. Backtraces from one of them worked nicely, while the other one didn't give any useful backtrace. `image show-unwind -a <address>` on the crash location gave the exact same output on both of them, saying `Asynchronous (not restricted to call-sites) UnwindPlan is 'EmulateInstructionARM64'` and the same emulated interpretation of the function. The call frame info based unwind plan didn't actually show up anywhere there.

However even if `image show-unwind` indicated that `EmulateInstructionARM64` would be used, in practice it fell back to using the arch default unwind plan, on the binary where backtraces failed. The reason for this was that `PECallFrameInfo::GetAddressRange` reported a bogus length for the function, which then caused `UnwindAssemblyInstEmulation::GetNonCallSiteUnwindPlanFromAssembly` to fail because `process_sp->GetTarget().ReadMemory` failed, because it tried to read a too long range.

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

https://reviews.llvm.org/D77000