[Lldb-commits] [PATCH] D137873: [LLDB][Minidump] Set abi environment for windows.

Wed Nov 16 14:34:29 PST 2022

zequanwu added a comment.

In D137873#3931888 <https://reviews.llvm.org/D137873#3931888>, @mstorsjo wrote:

> In D137873#3930683 <https://reviews.llvm.org/D137873#3930683>, @labath wrote:
>
>> If that doesn't work then (besides knowing why), I'd like us try some kind of a single-setting solution, as I don't think it makes sense to have two different settings like this (happy to hear counterarguments to that)
>
> Overall, the ideal is if the user usually needs to twiddle one single setting. For a majority of cases, picking the default format (like is done for the objectfile setting, and afaik this one does too even though I didn't quite clearly see the codepath for it) will be what you'd want: If you're working with mingw tools, debugging with a mingw built lldb, then your app that you're debugging also probably uses mingw ABIs. But it's clearly not implausible to want to debug executables from the other ecosystem, so being able to change it with a single setting is good too.
>
> And for the objectfile setting, it's possible to set it even on a per-DLL basis. This may sound contrieved, but it's actually totally reasonable. While the C++ ABIs are incompatible, such DLLs can interoperate over C APIs - this is not an uncommon scenario. Plus, every mingw executable links against msvcrt.dll or ucrtbase.dll (provided by the system) which use the MSVC C++ ABIs internally (if you use the versions of them with debug info).
>
> So the ability to set it per DLL for the objectfile part is totally warranted. How that fits in towards minidump reading, I don't really know though (since the whole minidump contains stuff for the whole process). In which cases does the C++ ABI form matter for the minidump btw - since the actual interpretation of things still would be based on per-DLL debug info (either as DWARF or PDB)?

lldb creates AST for user input expression which uses the abi from minidump process. That AST's class layout info is imported from another AST created from debug info that uses the abi specified in debug info. The current situation is that minidump process uses itanium ABI (because it's not specified in minidump, falls back to use itanium), but the debug info uses msvc ABI. The inconsistent causes crash when lowering class.

It looks like it's impossible to make it works with DLLs that have different ABIs, no matter which ABI we set for minidump. Unless we change how clang interacts with external layout: https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/RecordLayoutBuilder.cpp#L1357. Maybe change it so that if using external layout failed due to mismatched ABI, try again with another ABI https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/RecordLayoutBuilder.cpp#L3302?

================
Comment at: lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp:452
 ArchSpec ProcessMinidump::GetArchitecture() {
+  // "settings set plugin.process.minidump.abi" overrides the env in minidump.
+  ArchSpec arch = m_minidump_parser->GetArchitecture();
----------------
labath wrote:
> Is "overrides" the correct word here? Are there any circumstances in which we are able to determine the environment from the minidump file? Because if it is, then I would expect this to be the other way around (that the environment from a specific file overrides the generic catch-all setting)...
Yeah, the word is not correct. It should be "sets". 
I think minidump file just doesn't have that environment info: https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/BinaryFormat/Minidump.h#L161-L179. lldb sets the environment based on platform for android: https://github.com/llvm/llvm-project/blob/main/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp#L195.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137873