[LLDB][Minidump] Merge executable module's architecture into target's architecture.

[zequanwu]

Summary

This allows minidump process ABI to match the PE/COFF file ABI.

[labath]

This dual environment thing is tricky.. MSVC is probably a better default, but it would still mean that the same bugs will appear if the situation is reversed (we assume MSVC environ, but the binary actually uses GNU). Have you looked at how this interacts with https://reviews.llvm.org/D127048 (and the setting introduced there) and whether you can achieve the same effect by changing that setting?

[zequanwu]

In D137873#3926821, @labath wrote:

This dual environment thing is tricky.. MSVC is probably a better default, but it would still mean that the same bugs will appear if the situation is reversed (we assume MSVC environ, but the binary actually uses GNU). Have you looked at how this interacts with https://reviews.llvm.org/D127048 (and the setting introduced there) and whether you can achieve the same effect by changing that setting?

Thanks, I didn't realize the existence of this setting.

[zequanwu]

Oh, they are not the same. I will work on a similar setting for minidump.

[zequanwu]

Add settings set plugin.process.minidump.abi msvc/gnu to override minidump abi triple.

[labath]

Adding a new setting isn't exactly what I had in mind. I was actually hoping that it would be enough to fiddle with that setting and leave the minidump environment blank (because we have code to fill in the missing pieces of a target ArchSpec from other sources.

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). I'd consider either moving that environment setting to some place more generic (so it can be accessed from both plugins) or just having the minidump plugin fetch the setting from inside ObjectFilePECOFF (I'd say that a process->object dependency is kinda OK, and we already have ProcessMachCore depending on ObjectFileMachO).

lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp

393

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

[mstorsjo]

In 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)?

[zequanwu]

In D137873#3931888, @mstorsjo wrote:

In 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?

Or maybe for now, we should just choose the default ABI for windows minidump to be MSVC to at least to prevent that crash happening in our case. That's what I did in first diff.

lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp

393

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.

[alvinhochun]

I think the module spec of the modules should reflect the value of the plugin.object-file.pe-coff.abi setting. Can ProcessMinidump take the module spec of the executable module to use its target environment?

Mixing MSVC and MinGW DLLs is a tricky case so I will not be surprised if something doesn't work correctly. It would be nice to have it working but that would be outside the scope of this review. (I don't yet have a use case for this but I am getting close with some COM / WinRT experiments.)

[labath]

In D137873#3933171, @alvinhochun wrote:

I think the module spec of the modules should reflect the value of the plugin.object-file.pe-coff.abi setting. Can ProcessMinidump take the module spec of the executable module to use its target environment?

That's kinda what Target::MergeArchitecture is for.

Mixing MSVC and MinGW DLLs is a tricky case so I will not be surprised if something doesn't work correctly. It would be nice to have it working but that would be outside the scope of this review. (I don't yet have a use case for this but I am getting close with some COM / WinRT experiments.)

Sounds fun. :)

[zequanwu]

Use plugin.object-file.pe-coff.abi as minidump process abi when OS is Windows.

[labath]

Did you look at the Target::MergeArchitecture function? Is there a suitable place where we could insert a call to that function so that the executable object file environment is automatically propagated to the target?

[zequanwu]

Merge executable module's architecture into target's architecture.

[zequanwu]

lldb/test/Shell/SymbolFile/Breakpad/unwind-via-stack-win-no-memory-info.yaml

74 ↗

(On Diff #477008)

Processor Arch is X86, so this should be i386.

[labath]

Cool, great.

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

static bool SaveCore(const lldb::ProcessSP &process_sp,
                        const lldb_private::FileSpec &outfile,
                        lldb::SaveCoreStyle &core_style,
                        lldb_private::Status &error);
 
   static bool MagicBytesMatch(lldb::DataBufferSP data_sp);
 
   static lldb::SymbolType MapSymbolType(uint16_t coff_symbol_type);
 
+  static llvm::Triple::EnvironmentType GetGlobalPluginEnvironment();
+
   // LLVM RTTI support
   static char ID;
   bool isA(const void *ClassID) const override {
     return ClassID == &ID || ObjectFile::isA(ClassID);
   }
   static bool classof(const ObjectFile *obj) { return obj->isA(&ID); }
 
   bool ParseHeader() override;

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

if (base_type == llvm::COFF::IMAGE_SYM_TYPE_NULL &&
       complex_type == llvm::COFF::IMAGE_SYM_DTYPE_NULL) {
     // Unknown type. LLD and GNU ld uses this for variables on MinGW, so
     // consider these symbols to be data to enable printing.
     return lldb::eSymbolTypeData;
   }
   return lldb::eSymbolTypeInvalid;
 }
 
+llvm::Triple::EnvironmentType ObjectFilePECOFF::GetGlobalPluginEnvironment() {
+  return GetGlobalPluginProperties().ABI();
+}
+
 bool ObjectFilePECOFF::CreateBinary() {
   if (m_binary)
     return true;
 
   Log *log = GetLog(LLDBLog::Object);
 
   auto binary = llvm::object::createBinary(llvm::MemoryBufferRef(
       toStringRef(m_data.GetData()), m_file.GetFilename().GetStringRef()));

lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp

 //===-- ProcessMinidump.cpp -----------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "ProcessMinidump.h"
 
+#include "Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h"
 #include "ThreadMinidump.h"
 
 #include "lldb/Core/DumpDataExtractor.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/Section.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandObject.h"
 #include "lldb/Interpreter/CommandObjectMultiword.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
 #include "lldb/Interpreter/OptionArgParser.h"
 #include "lldb/Interpreter/OptionGroupBoolean.h"
 #include "lldb/Target/JITLoaderList.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/UnixSignals.h"
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/State.h"
 #include "llvm/BinaryFormat/Magic.h"
+#include "llvm/Support/Host.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Threading.h"
 
 #include "Plugins/Process/Utility/StopInfoMachException.h"
 
 #include <memory>
 
 using namespace lldb;
if (mem.empty()) {
     return 0;
   }
 
   std::memcpy(buf, mem.data(), mem.size());
   return mem.size();
 }
 
 ArchSpec ProcessMinidump::GetArchitecture() {
   if (!m_is_wow64) {
-    return m_minidump_parser->GetArchitecture();
+    ArchSpec arch = m_minidump_parser->GetArchitecture();
+    if (arch.GetTriple().getOS() == llvm::Triple::OSType::Win32) {
+      static llvm::Triple::EnvironmentType default_env = [] {
+        auto def_target = llvm::Triple(
+            llvm::Triple::normalize(llvm::sys::getDefaultTargetTriple()));
+        if (def_target.getOS() == llvm::Triple::Win32 &&
+            def_target.getEnvironment() != llvm::Triple::UnknownEnvironment)
+          return def_target.getEnvironment();
+        return llvm::Triple::MSVC;
+      }();
+      llvm::Triple::EnvironmentType env =
+          ObjectFilePECOFF::GetGlobalPluginEnvironment();
+      if (env == llvm::Triple::UnknownEnvironment)
+        env = default_env;
+      arch.GetTriple().setEnvironment(env);
+    }
+    return arch;
   }
 
   llvm::Triple triple;
   triple.setVendor(llvm::Triple::VendorType::UnknownVendor);
   triple.setArch(llvm::Triple::ArchType::x86);
   triple.setOS(llvm::Triple::OSType::Win32);
   return ArchSpec(triple);
 }

lldb/test/Shell/Minidump/Windows/find-module.test

 Test that we correctly find a PE/COFF file in our executable search path, and
 use it when opening minidumps.
 
 RUN: yaml2obj %S/Inputs/find-module.exe.yaml -o %T/find-module.exe
 RUN: yaml2obj %S/Inputs/find-module.dmp.yaml -o %T/find-module.dmp
 RUN: %lldb -O "settings set target.exec-search-paths %T" \
-RUN:   -c %T/find-module.dmp -o "image dump objfile" -o exit | FileCheck %s
+RUN:   -c %T/find-module.dmp -o "image dump objfile" -o "target list" -o exit \
+RUN:   | FileCheck --check-prefix=DEFAULT %s
 
-CHECK-LABEL: image dump objfile
-CHECK: ObjectFilePECOFF, file = '{{.*}}find-module.exe', arch = i386
+RUN: %lldb -O "settings set plugin.object-file.pe-coff.abi msvc" \
+RUN:   -O "settings set target.exec-search-paths %T" -c %T/find-module.dmp \
+RUN:   -o "target list" -o exit | FileCheck --check-prefix=MSVC %s
+
+RUN: %lldb -O "settings set plugin.object-file.pe-coff.abi gnu" \
+RUN:   -O "settings set target.exec-search-paths %T" -c %T/find-module.dmp \
+RUN:   -o "target list" -o exit | FileCheck --check-prefix=GNU %s
+
+DEFAULT-LABEL: image dump objfile
+DEFAULT: ObjectFilePECOFF, file = '{{.*}}find-module.exe', arch = i386
+
+DEFAULT-LABEL: target list
+DEFAULT: arch=i386-pc-windows-{{msvc|gnu}}
+
+MSVC-LABEL: target list
+MSVC: arch=i386-pc-windows-msvc
+
+GNU-LABEL: target list
+GNU: arch=i386-pc-windows-gnu