Page MenuHomePhabricator

[LLDB] [PECOFF] Only use PECallFrameInfo on the one supported architecture
ClosedPublic

Authored by mstorsjo on Mar 28 2020, 4:38 PM.

Details

Summary

The RuntimeFunction struct, which PECallFrameInfo interprets, has a different layout and differnet semantics on all architectures.

Diff Detail

Event Timeline

mstorsjo created this revision.Mar 28 2020, 4:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2020, 4:38 PM

Hello! But does the format of an exception directory for aarch64 differ completely from x86-64 one? Can we somehow adopt the existing parser to support it? 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?

Hello! But does the format of an exception directory for aarch64 differ completely from x86-64 one? Can we somehow adopt the existing parser to support it? 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?

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).

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

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?

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.

mstorsjo updated this revision to Diff 253548.Mar 30 2020, 4:16 AM
mstorsjo retitled this revision from [lldb] [PECOFF] Check that PECallFrameInfo operates on the expected architecture before interpreting RuntimeFunction structs to [LLDB] [PECOFF] Only use PECallFrameInfo on the one supported architecture.
mstorsjo edited the summary of this revision. (Show Details)

Moved the check up to ObjectFilePECOFF.

@labath @aleksandr.urakov Does this version look good to you?

aleksandr.urakov accepted this revision.Mar 31 2020, 4:51 AM

Sorry for the long delay, LGTM now, thanks!

This revision is now accepted and ready to land.Mar 31 2020, 4:51 AM
This revision was automatically updated to reflect the committed changes.