[lldb] Set COFF module ABI from default triple and make it an option

[alvinhochun]

Summary

PE/COFF can use either MSVC or GNU (MinGW) ABI for C++ code, however
LLDB had defaulted to MSVC implicitly with no way to override it. This
causes issues when debugging modules built with the GNU ABI, sometimes
even crashes.

This changes the PE/COFF plugin to set the module triple according to
the default target triple used to build LLDB. If the default target
triple is Windows and a valid environment is specified, then this
environment will be used for the module spec. This not only works for
MSVC and GNU, but also other environments.

A new setting, plugin.object-file.pe-coff.abi, has been added to
allow overriding this default ABI.

• Fixes https://github.com/llvm/llvm-project/issues/50775
• Fixes https://github.com/mstorsjo/llvm-mingw/issues/226
• Fixes https://github.com/mstorsjo/llvm-mingw/issues/282

[alvinhochun]

Added tests

[alvinhochun]

Fixed tests

[alvinhochun]

Changed test requirement to be more accurate

[alvinhochun]

Fixed tests again

[DavidSpickett]

This changes the PE/COFF and PDB plugins to set the module triple according to the default target triple used to build LLDB.

I'm missing some context here but using triple used when *building* lldb seems to conflict with the idea that the same lldb (client at least) can be used to debug many architectures.

Or is this not a concern because remote debugging is not a thing for Windows? For instance, is it possible that I'm on Linux with some AArch64 linux targeted lldb trying to remote debug a windows system. If the default triple is the best signal you've got then fair enough I guess just wanted to check there wasn't some more dynamic way to get it.

And I assume PE/COFF have no embedded flag in them to indicate this ABI?

[mstorsjo]

In D127048#3559866, @DavidSpickett wrote:

This changes the PE/COFF and PDB plugins to set the module triple according to the default target triple used to build LLDB.

I'm missing some context here but using triple used when *building* lldb seems to conflict with the idea that the same lldb (client at least) can be used to debug many architectures.

Yes, that's true. The idea here is that when doing local debugging in either the mingw or msvc ecosystems, the flavour of the lldb build itself could be a decent hint - but it's not indeed not a correct and faultproof fix.

Or is this not a concern because remote debugging is not a thing for Windows? For instance, is it possible that I'm on Linux with some AArch64 linux targeted lldb trying to remote debug a windows system. If the default triple is the best signal you've got then fair enough I guess just wanted to check there wasn't some more dynamic way to get it.

And I assume PE/COFF have no embedded flag in them to indicate this ABI?

No, there's no flag for that. Mingw and MSVC environment binaries are quite the same.

(Qt Creator tries to do some similar heuristics to guess which ABI a binary uses, by looking at e.g. linker version numbers in the binary, to deduce whether a binary has been linked with GNU ld.bfd or MS link.exe - but when it comes to LLD, that logic falls apart. A more complex heuristic that can be used is e.g. looking at the list of DLLs that a binary links against, but that's also much more complex and also not entirely faultproof.)

@alvinhochun has got another patch in progress - https://reviews.llvm.org/D127053 (which isn't yet submitted for review as it lacks tests), which tries to decude the same based on whether a binary contains dwarf debug info.

That's more fault proof, but is also not an entirely complete fix - while finding dwarf debug sections mostly can imply using the itanium C++ ABI, you can also use PDB files with itanium C++ ABI (i.e. in mingw setups). It's less common but still a viable scenario. And reversely, I think I've heard about special cases where people use dwarf debug info with MSVC-ecosystem builds too...

So in short, what would be needed is a good enough heuristic that works for the vast majority of cases, and an option that lets users specify it for the cases that can't easily be autodetected.

[alvinhochun]

In D127048#3559918, @mstorsjo wrote:

In D127048#3559866, @DavidSpickett wrote:

This changes the PE/COFF and PDB plugins to set the module triple according to the default target triple used to build LLDB.

I'm missing some context here but using triple used when *building* lldb seems to conflict with the idea that the same lldb (client at least) can be used to debug many architectures.

Yes, that's true. The idea here is that when doing local debugging in either the mingw or msvc ecosystems, the flavour of the lldb build itself could be a decent hint - but it's not indeed not a correct and faultproof fix.

Yes, the aim is to at least have a default that at least works natively for the default target in local debugging. Before this change, C++ debugging for mingw target just doesn't work at all from what I understand so far.

@alvinhochun has got another patch in progress - https://reviews.llvm.org/D127053 (which isn't yet submitted for review as it lacks tests), which tries to decude the same based on whether a binary contains dwarf debug info.

That's more fault proof, but is also not an entirely complete fix - while finding dwarf debug sections mostly can imply using the itanium C++ ABI, you can also use PDB files with itanium C++ ABI (i.e. in mingw setups). It's less common but still a viable scenario. And reversely, I think I've heard about special cases where people use dwarf debug info with MSVC-ecosystem builds too...

So in short, what would be needed is a good enough heuristic that works for the vast majority of cases, and an option that lets users specify it for the cases that can't easily be autodetected.

I'm feeling wishy-washy on that patch because of the reason you stated. It may still be a good heuristic, but I think having an option to specify the ABI is a requirement before that patch can be landed.

I still don't have a clue how I can make it an option. It seems impossible to pass the option into ObjectFilePECOFF::GetModuleSpecifications cleanly.

[DavidSpickett]

There is an arch setting to target create but I doubt passing the whole triple there will make a difference (truncated to arch only at best).

Platforms do have settings so you could try adding something to remote-windows or presumably host when run on Windows. How you plumb that into where you need it I don't know.

(lldb) settings show platform.plugin.qemu-user.architecture
platform.plugin.qemu-user.architecture (string) =

[DavidSpickett]

I won't have time to comment much more but Omair might be able to help.

Also you might want to look at how the apple platforms select their ABI if you haven't already. If the answer is "with a flag in the object file" then oh well, but perhaps they have an override in there you could crib from.

[alvinhochun]

Thanks for the hint. I think I can add a setting, say plugin.object-file.pe-coff.abi. It needs some plumbing but should be doable.

[alvinhochun]

Removed the changes for PDB, I don't think they actually affect anything.

Added a new setting plugin.object-file.pe-coff.abi to allow overriding the default ABI.

[omjavaid]

I am not fully familiar with PE/COFF but lets try to get this merged. For starters I have a couple of questions here:

1. This rev apparently assumes x86_64 only but what about Arm/AArch64?
2. If we have DLLs what abi setting they ll follow the one set by the user or default which may be MSVC?
3. Is there anything in PE/COFF header information that can serve is a good guess for the underlying ABI.
4. What more information can we get from debug info in this case? DWARF may not be a good guess as we can generate it for MSVC abi as well.

[mstorsjo]

In D127048#3565557, @omjavaid wrote:

I am not fully familiar with PE/COFF but lets try to get this merged. For starters I have a couple of questions here:

1. This rev apparently assumes x86_64 only but what about Arm/AArch64?

Where do you get that impression? This should all be architecture independent (or done in the same way for all architectures).

2. If we have DLLs what abi setting they ll follow the one set by the user or default which may be MSVC?

Here, all DLLs are expected to use the same ABI (either the implicit one based on the LLDB build). D127234 is a follow-up that allows you to set a specific ABI choice per DLL.

3. Is there anything in PE/COFF header information that can serve is a good guess for the underlying ABI.

No, there's no such hint. It's possible to make heuristic guesses based on e.g. what DLLs they import, but it's all guesses, possibly brittle. Having an option allows you to get it right even for the cases where the default is wrong.

4. What more information can we get from debug info in this case? DWARF may not be a good guess as we can generate it for MSVC abi as well.

Exactly, one could use presence of DWARF as a heuristic hint for this, but it's not always right (you can use PDB for mingw uses too, and DWARF for msvc abi too in uncommon cases).

[alvinhochun]

Thanks @mstorsjo for answering for me.

4. What more information can we get from debug info in this case? DWARF may not be a good guess as we can generate it for MSVC abi as well.

Exactly, one could use presence of DWARF as a heuristic hint for this, but it's not always right (you can use PDB for mingw uses too, and DWARF for msvc abi too in uncommon cases).

I suppose we may try to look for mangled names and identify which name mangling scheme was used (MSVC vs Itanium ABI)? Though I am not keen on implementing it myself.

[omjavaid]

1. This rev apparently assumes x86_64 only but what about Arm/AArch64?

Where do you get that impression? This should all be architecture independent (or done in the same way for all architectures).

Sorry I just brushed over the code and saw x86_64 specifc obj files in tests.

This looks good to me and I also gave this change a test on AArch64/Windows Surface X pro. No regressions. LGTM

[alvinhochun]

Rebased patch

lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp

 #include "lldb/Utility/StreamString.h"
 #include "llvm/BinaryFormat/Magic.h"
 #include "llvm/DebugInfo/PDB/Native/DbiStream.h"
 #include "llvm/DebugInfo/PDB/Native/InfoStream.h"
 #include "llvm/DebugInfo/PDB/Native/NativeSession.h"
 #include "llvm/DebugInfo/PDB/Native/PDBFile.h"
 #include "llvm/DebugInfo/PDB/PDB.h"
 #include "llvm/Support/BinaryByteStream.h"
+#include "llvm/Support/Host.h"
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace llvm::pdb;
 using namespace llvm::codeview;
 
 LLDB_PLUGIN_DEFINE(ObjectFilePDB)
 
size_t ObjectFilePDB::GetModuleSpecifications(
   if (!dbi_stream) {
     llvm::consumeError(dbi_stream.takeError());
     return initial_count;
   }
 
   lldb_private::UUID &uuid = module_spec.GetUUID();
   uuid = GetPDBUUID(*info_stream);
 
+  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;
+  }();
+
   ArchSpec &module_arch = module_spec.GetArchitecture();
   switch (dbi_stream->getMachineType()) {
   case PDB_Machine::Amd64:
     module_arch.SetTriple("x86_64-pc-windows");
+    module_arch.GetTriple().setEnvironment(default_env);
     specs.Append(module_spec);
     break;
   case PDB_Machine::x86:
     module_arch.SetTriple("i386-pc-windows");
+    module_arch.GetTriple().setEnvironment(default_env);
     specs.Append(module_spec);
     module_arch.SetTriple("i686-pc-windows");
+    module_arch.GetTriple().setEnvironment(default_env);
     specs.Append(module_spec);
     break;
   case PDB_Machine::ArmNT:
     module_arch.SetTriple("armv7-pc-windows");
+    module_arch.GetTriple().setEnvironment(default_env);
     specs.Append(module_spec);
     break;
   case PDB_Machine::Arm64:
     module_arch.SetTriple("aarch64-pc-windows");
+    module_arch.GetTriple().setEnvironment(default_env);
     specs.Append(module_spec);
     break;
   default:
     break;
   }
 
   return specs.GetSize() - initial_count;
 }

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

 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/StreamString.h"
 #include "lldb/Utility/Timer.h"
 #include "lldb/Utility/UUID.h"
 #include "llvm/BinaryFormat/COFF.h"
 
 #include "llvm/Object/COFFImportFile.h"
 #include "llvm/Support/Error.h"
+#include "llvm/Support/Host.h"
 #include "llvm/Support/MemoryBuffer.h"
 
 #define IMAGE_DOS_SIGNATURE 0x5A4D    // MZ
 #define IMAGE_NT_SIGNATURE 0x00004550 // PE00
 #define OPT_HEADER_MAGIC_PE32 0x010b
 #define OPT_HEADER_MAGIC_PE32_PLUS 0x020b
 
 using namespace lldb;
if (!COFFObj)
     return initial_count;
 
   ModuleSpec module_spec(file);
   ArchSpec &spec = module_spec.GetArchitecture();
   lldb_private::UUID &uuid = module_spec.GetUUID();
   if (!uuid.IsValid())
     uuid = GetCoffUUID(*COFFObj);
 
+  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;
+  }();
+
   switch (COFFObj->getMachine()) {
   case MachineAmd64:
     spec.SetTriple("x86_64-pc-windows");
+    spec.GetTriple().setEnvironment(default_env);
     specs.Append(module_spec);
     break;
   case MachineX86:
     spec.SetTriple("i386-pc-windows");
+    spec.GetTriple().setEnvironment(default_env);
     specs.Append(module_spec);
     spec.SetTriple("i686-pc-windows");
+    spec.GetTriple().setEnvironment(default_env);
     specs.Append(module_spec);
     break;
   case MachineArmNt:
     spec.SetTriple("armv7-pc-windows");
+    spec.GetTriple().setEnvironment(default_env);
     specs.Append(module_spec);
     break;
   case MachineArm64:
     spec.SetTriple("aarch64-pc-windows");
+    spec.GetTriple().setEnvironment(default_env);
     specs.Append(module_spec);
     break;
   default:
     break;
   }
 
   return specs.GetSize() - initial_count;
 }

lldb/test/Shell/ObjectFile/PDB/object.test

+# XFAIL: windows-gnu
+
 # RUN: llvm-pdbutil yaml2pdb %s -pdb=%t.pdb
 # RUN: lldb-test object-file %t.pdb | FileCheck %s
 
 # CHECK: Plugin name: pdb
 # CHECK: Architecture: x86_64-pc-windows-msvc
 # CHECK: UUID: 61AF583F-29A8-7A6C-4C4C-44205044422E-00000001
 # CHECK: Executable: false
 # CHECK: Stripped: false

lldb/test/Shell/ObjectFile/PECOFF/default-triple-windows-gnu.yaml

+# XFAIL: windows-msvc
+
+# RUN: yaml2obj %s -o %t
+# RUN: lldb-test object-file %t | FileCheck %s
+
+# CHECK: Architecture: x86_64-pc-windows-gnu
+
+--- !COFF
+OptionalHeader:
+  AddressOfEntryPoint: 5152
+  ImageBase:       5368709120
+  SectionAlignment: 4096
+  FileAlignment:   512
+  MajorOperatingSystemVersion: 6
+  MinorOperatingSystemVersion: 0
+  MajorImageVersion: 0
+  MinorImageVersion: 0
+  MajorSubsystemVersion: 6
+  MinorSubsystemVersion: 0
+  Subsystem:       IMAGE_SUBSYSTEM_WINDOWS_CUI
+  DLLCharacteristics: [ IMAGE_DLL_CHARACTERISTICS_HIGH_ENTROPY_VA, IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE, IMAGE_DLL_CHARACTERISTICS_NX_COMPAT, IMAGE_DLL_CHARACTERISTICS_TERMINAL_SERVER_AWARE ]
+  SizeOfStackReserve: 1048576
+  SizeOfStackCommit: 4096
+  SizeOfHeapReserve: 1048576
+  SizeOfHeapCommit: 4096
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [ IMAGE_FILE_EXECUTABLE_IMAGE, IMAGE_FILE_LARGE_ADDRESS_AWARE ]
+sections:
+  - Name:            .text
+    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+    VirtualAddress:  4096
+    VirtualSize:     64
+    SectionData:     DEADBEEFBAADF00D
+  - Name:            .data
+    Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ ]
+    VirtualAddress:  8192
+    VirtualSize:     64
+    SectionData:     DEADBEEFBAADF00D
+symbols:         []
+...

lldb/test/Shell/ObjectFile/PECOFF/default-triple-windows-msvc.yaml

+# XFAIL: windows-gnu
+
+# RUN: yaml2obj %s -o %t
+# RUN: lldb-test object-file %t | FileCheck %s
+
+# CHECK: Architecture: x86_64-pc-windows-msvc
+
+--- !COFF
+OptionalHeader:
+  AddressOfEntryPoint: 5152
+  ImageBase:       5368709120
+  SectionAlignment: 4096
+  FileAlignment:   512
+  MajorOperatingSystemVersion: 6
+  MinorOperatingSystemVersion: 0
+  MajorImageVersion: 0
+  MinorImageVersion: 0
+  MajorSubsystemVersion: 6
+  MinorSubsystemVersion: 0
+  Subsystem:       IMAGE_SUBSYSTEM_WINDOWS_CUI
+  DLLCharacteristics: [ IMAGE_DLL_CHARACTERISTICS_HIGH_ENTROPY_VA, IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE, IMAGE_DLL_CHARACTERISTICS_NX_COMPAT, IMAGE_DLL_CHARACTERISTICS_TERMINAL_SERVER_AWARE ]
+  SizeOfStackReserve: 1048576
+  SizeOfStackCommit: 4096
+  SizeOfHeapReserve: 1048576
+  SizeOfHeapCommit: 4096
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [ IMAGE_FILE_EXECUTABLE_IMAGE, IMAGE_FILE_LARGE_ADDRESS_AWARE ]
+sections:
+  - Name:            .text
+    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+    VirtualAddress:  4096
+    VirtualSize:     64
+    SectionData:     DEADBEEFBAADF00D
+  - Name:            .data
+    Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ ]
+    VirtualAddress:  8192
+    VirtualSize:     64
+    SectionData:     DEADBEEFBAADF00D
+symbols:         []
+...

lldb/test/Shell/lit.cfg.py

 
 llvm_config.use_default_substitutions()
 toolchain.use_lldb_substitutions(config)
 toolchain.use_support_substitutions(config)
 
 if re.match(r'^arm(hf.*-linux)|(.*-linux-gnuabihf)', config.target_triple):
     config.available_features.add("armhf-linux")
 
+if re.match(r'.*-(windows-msvc)$', config.target_triple):
+    config.available_features.add("windows-msvc")
+
+if re.match(r'.*-(windows-gnu|mingw32)$', config.target_triple):
+    config.available_features.add("windows-gnu")
+
 def calculate_arch_features(arch_string):
     # This will add a feature such as x86, arm, mips, etc for each built
     # target
     features = []
     for arch in arch_string.split():
         features.append(arch.lower())
     return features