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

[mstorsjo]

Summary

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

[aleksandr.urakov]

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?

[labath]

In D77000#1949247, @aleksandr.urakov wrote:

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

[mstorsjo]

In 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, @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.

[mstorsjo]

Moved the check up to ObjectFilePECOFF.

[mstorsjo]

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

[aleksandr.urakov]

Sorry for the long delay, LGTM now, thanks!

lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

std::unique_ptr<CallFrameInfo> ObjectFilePECOFF::CreateCallFrameInfo() {
   if (coff_data_dir_exception_table >= m_coff_header_opt.data_dirs.size())
     return {};
 
   data_directory data_dir_exception =
       m_coff_header_opt.data_dirs[coff_data_dir_exception_table];
   if (!data_dir_exception.vmaddr)
     return {};
 
+  if (m_coff_header.machine != llvm::COFF::IMAGE_FILE_MACHINE_AMD64)
+    return {};
+
   return std::make_unique<PECallFrameInfo>(*this, data_dir_exception.vmaddr,
                                            data_dir_exception.vmsize);
 }
 
 bool ObjectFilePECOFF::IsStripped() {
   // TODO: determine this for COFF
   return false;
 }