[lldb] Fix reading i686-windows executables with GNU environment

[mstorsjo]

Summary

25c8a061c5739677d2fc0af29a8cc9520207b923 / D127048 added an option
for setting the ABI to GNU.

When an object file is loaded, there's only minimal verification
done for the architecture spec set for it, if the object file only
provides one.

However, for i386 object files, the PECOFF object file plugin
provides two architectures, i386-pc-windows and i686-pc-windows.
This picks a totally different codepath in
TargetList::CreateTargetInternal, where it's treated as a fat
binary. This goes through more verifications to see if the
architectures provided by the object file matches what the
platform plugin supports.

The PlatformWindows() constructor explicitly adds the
"i386-pc-windows" and "i686-pc-windows" architectures (even when
running on other architectures), which allows this "fat binary
verification" to succeed for the i386 object files that provide
two architectures.

However, after that commit, if the object file is advertised with
the different environment (either when lldb is built in a mingw
environment, or if that setting is set), the fat binary validation
won't accept the file any longer.

Update ArchSpec::IsEqualTo with more logic for the Windows use
cases; mismatching vendors is not an issue (they don't have any
practical effect on Windows), and GNU and MSVC environments are
compatible to the point that PlatformWindows can handle object
files for both environments/ABIs.

As a separate path forward, one could also consider to stop returning
two architecture specs from ObjectFilePECOFF::GetModuleSpecifications
for i386 files.

[DavidSpickett]

However, after this commit, if the object file is advertised with the different environment (either when built in a mingw environment, or if that setting is set), the fat binary validation won't accept the file any longer.

Is "this commit" referring to https://reviews.llvm.org/rG25c8a061c5739677d2fc0af29a8cc9520207b923 or to the change we're reviewing here?

I'm struggling to think through what this is actually fixing. Is the issue that one half of a fat binary can have a different ABI? Wouldn't both sides of the fat binary be loaded as the ABI chosen in the setting, or is that exactly what you're fixing.

lldb/source/Utility/ArchSpec.cpp

1006

You could use isOSWindows if you want. Maybe use an intermediate bool to clear up the if statement.

bool both_windows = lhs_triple.isOSWindows() && rhs_triple.isOSWindows();
if ((lhs_triple_vendor != rhs_triple_vendor) && 
(exact_match || !both_windows)) {

[alvinhochun]

Yes, this (patch) looks right to me.

[mstorsjo]

In D128268#3598967, @DavidSpickett wrote:

However, after this commit, if the object file is advertised with the different environment (either when built in a mingw environment, or if that setting is set), the fat binary validation won't accept the file any longer.

Is "this commit" referring to https://reviews.llvm.org/rG25c8a061c5739677d2fc0af29a8cc9520207b923 or to the change we're reviewing here?

Sorry, I meant 25c8a061c5739677d2fc0af29a8cc9520207b923.

I'm struggling to think through what this is actually fixing. Is the issue that one half of a fat binary can have a different ABI? Wouldn't both sides of the fat binary be loaded as the ABI chosen in the setting, or is that exactly what you're fixing.

The issue is that in the case of a fat binary, we run a loop to check and pick an architecture out of it: https://github.com/llvm/llvm-project/blob/main/lldb/source/Target/TargetList.cpp#L173-L184 This is checked against the architectures added here: https://github.com/llvm/llvm-project/blob/main/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp#L127-L131

(In the case of a non-fat binary, i.e. all other architectures than i386/i686 on PECOFF, we hit this case here, where we don't validate the arch spec at all: https://github.com/llvm/llvm-project/blob/main/lldb/source/Target/TargetList.cpp#L160-L164)

Now since the addition of the new setting in 25c8a061c5739677d2fc0af29a8cc9520207b923, ObjectFile::GetModuleSpecifications can return i386-pc-windows-gnu and i686-pc-windows-gnu, while PlatformWindows provides a list containing i386-pc-windows (which is normalized to i386-pc-windows-msvc) and i686-pc-windows (...-msvc). Due to the differing environments, these are deemed incompatible, and LLDB flat out refuses to load the file, with the error message error: no matching platforms found for this file.

[DavidSpickett]

Now since the addition of the new setting in 25c8a061c5739677d2fc0af29a8cc9520207b923, ObjectFile::GetModuleSpecifications can return i386-pc-windows-gnu and i686-pc-windows-gnu, while PlatformWindows provides a list containing i386-pc-windows (which is normalized to i386-pc-windows-msvc) and i686-pc-windows (...-msvc). Due to the differing environments, these are deemed incompatible, and LLDB flat out refuses to load the file, with the error message error: no matching platforms found for this file.

Cool, now I understand. Can you add something like that as a comment in the test file? Without that context the test seems not to be testing much at all.

[mstorsjo]

Added a comment to the testcase.

[DavidSpickett]

LGTM. I like the ifs the way I suggested but up to you, either way it's not the easiest logic to parse.

[mstorsjo]

In D128268#3601104, @DavidSpickett wrote:

LGTM. I like the ifs the way I suggested but up to you, either way it's not the easiest logic to parse.

Oh, sorry, I forgot about that comment. Yes, that does indeed look even nicer, I'll try to add that.

[mstorsjo]

Refactored the code to add the both_windows variable as suggested (which turned out much nicer, thanks!).

[labath]

As a separate path forward, one could also consider to stop returning
two architecture specs from ObjectFilePECOFF::GetModuleSpecifications
for i386 files.

I think that would be much more preferable. I think the current implementation GetModuleSpecifications is based on a misconception about what the multiple results of that function mean (a fat binary -- as you've discovered).

We wouldn't want to return armv6-*-* (or armv7, v8, ...) for a file that claims it can be run on armv5. Elf files also return just a single result for i386 file.

[mstorsjo]

In D128268#3603931, @labath wrote:

As a separate path forward, one could also consider to stop returning
two architecture specs from ObjectFilePECOFF::GetModuleSpecifications
for i386 files.

I think that would be much more preferable. I think the current implementation GetModuleSpecifications is based on a misconception about what the multiple results of that function mean (a fat binary -- as you've discovered).

We wouldn't want to return armv6-*-* (or armv7, v8, ...) for a file that claims it can be run on armv5. Elf files also return just a single result for i386 file.

Yep, that's probably a good path forward for the future - I was just weary of that possibly opening another huge can of worms. And regardless of that, I think the functional change of this patch, allowing considering various variations of triples that commonly occur on windows as compatible, is sensible on its own (such issues have caused unnecessary tweaking back and forth once or twice before too).

[mstorsjo]

In D128268#3603931, @labath wrote:

As a separate path forward, one could also consider to stop returning
two architecture specs from ObjectFilePECOFF::GetModuleSpecifications
for i386 files.

I think that would be much more preferable. I think the current implementation GetModuleSpecifications is based on a misconception about what the multiple results of that function mean (a fat binary -- as you've discovered).

We wouldn't want to return armv6-*-* (or armv7, v8, ...) for a file that claims it can be run on armv5. Elf files also return just a single result for i386 file.

If we'd just set this to the baseline, i386, would that have any effect for how lldb e.g. is able to disassemble/interpret instructions that don't exist in the i386 baseline architecture?

[labath]

In D128268#3604077, @mstorsjo wrote:

In D128268#3603931, @labath wrote:

As a separate path forward, one could also consider to stop returning
two architecture specs from ObjectFilePECOFF::GetModuleSpecifications
for i386 files.

I think that would be much more preferable. I think the current implementation GetModuleSpecifications is based on a misconception about what the multiple results of that function mean (a fat binary -- as you've discovered).

We wouldn't want to return armv6-*-* (or armv7, v8, ...) for a file that claims it can be run on armv5. Elf files also return just a single result for i386 file.

If we'd just set this to the baseline, i386, would that have any effect for how lldb e.g. is able to disassemble/interpret instructions that don't exist in the i386 baseline architecture?

It should not have any effect (if it does, that's a separate fix). In the disassembler, we explicitly enable the latest instruction set, and I can't think of anything else that would be impacted by it.

[mstorsjo]

In D128268#3604081, @labath wrote:

If we'd just set this to the baseline, i386, would that have any effect for how lldb e.g. is able to disassemble/interpret instructions that don't exist in the i386 baseline architecture?

It should not have any effect (if it does, that's a separate fix). In the disassembler, we explicitly enable the latest instruction set, and I can't think of anything else that would be impacted by it.

Thanks - I did some cursory testing with removing the extra i686 everywhere, and at least on a quick test, it seems to work just fine (and requires a minor adjustment to only one testcase).

I found that this duality was introduced in 5e6f45201f0b62c1e7a24fc396f3ea6e10dc880d / D7120 and ad587ae4ca143d388c0ec4ef2faa1b5eddedbf67 / D4658 (CC @zturner), what do you make out of the reasonings in those commits?

[labath]

In D128268#3604555, @mstorsjo wrote:

In D128268#3604081, @labath wrote:

If we'd just set this to the baseline, i386, would that have any effect for how lldb e.g. is able to disassemble/interpret instructions that don't exist in the i386 baseline architecture?

It should not have any effect (if it does, that's a separate fix). In the disassembler, we explicitly enable the latest instruction set, and I can't think of anything else that would be impacted by it.

Thanks - I did some cursory testing with removing the extra i686 everywhere, and at least on a quick test, it seems to work just fine (and requires a minor adjustment to only one testcase).

I found that this duality was introduced in 5e6f45201f0b62c1e7a24fc396f3ea6e10dc880d / D7120 and ad587ae4ca143d388c0ec4ef2faa1b5eddedbf67 / D4658 (CC @zturner), what do you make out of the reasonings in those commits?

The first patch seems like it's just working around some mismatches in the windows dynamic loader plugin. I think a better approach would be to have the dynamic loader claim both architectures, though I don't think that is necessary if we're just consistent about what we use. I don't see anything wrong with the second patch (the darwin platform does something similar for arm architectures, even though I'm not convinced it's necessary (the reason it's necessary for darwin is because there they actually make a distinction between armv6XX and armv7YY and treat those as different architectures).

[mstorsjo]

In D128268#3608197, @labath wrote:

In D128268#3604555, @mstorsjo wrote:

I found that this duality was introduced in 5e6f45201f0b62c1e7a24fc396f3ea6e10dc880d / D7120 and ad587ae4ca143d388c0ec4ef2faa1b5eddedbf67 / D4658 (CC @zturner), what do you make out of the reasonings in those commits?

The first patch seems like it's just working around some mismatches in the windows dynamic loader plugin. I think a better approach would be to have the dynamic loader claim both architectures, though I don't think that is necessary if we're just consistent about what we use. I don't see anything wrong with the second patch (the darwin platform does something similar for arm architectures, even though I'm not convinced it's necessary (the reason it's necessary for darwin is because there they actually make a distinction between armv6XX and armv7YY and treat those as different architectures).

The odd thing about the second one is the added hardcoded AddArch(ArchSpec("i686-pc-windows")); and AddArch(ArchSpec("i386-pc-windows")); at the top of PlatformWindows.cpp.

Anyway, it does seem to work fine in my quick test to get rid of this duality, so I'll post a patch that does that.

[labath]

In D128268#3611053, @mstorsjo wrote:

The odd thing about the second one is the added hardcoded AddArch(ArchSpec("i686-pc-windows")); and AddArch(ArchSpec("i386-pc-windows")); at the top of PlatformWindows.cpp.

Anyway, it does seem to work fine in my quick test to get rid of this duality, so I'll post a patch that does that.

I think those hardcoded lines are there precisely because of the duality (i.e. wanting to support both i386 and i686 regardless of what the host layer reports). If that is removed, maybe all of that can be changed to the pattern used in other platform plugins.

[mstorsjo]

In D128268#3614661, @labath wrote:

In D128268#3611053, @mstorsjo wrote:

The odd thing about the second one is the added hardcoded AddArch(ArchSpec("i686-pc-windows")); and AddArch(ArchSpec("i386-pc-windows")); at the top of PlatformWindows.cpp.

Anyway, it does seem to work fine in my quick test to get rid of this duality, so I'll post a patch that does that.

I think those hardcoded lines are there precisely because of the duality (i.e. wanting to support both i386 and i686 regardless of what the host layer reports).

Both that, and because the i386+i686 fat binary arches from ObjectFile triggered the more elaborate arch validation, these had to be present to make it work. Anyway, after getting rid of the duality, those hardcoded ones don't seem to be needed either.

If that is removed, maybe all of that can be changed to the pattern used in other platform plugins.

That looks reasonable!

lldb/source/Utility/ArchSpec.cpp

if (GetByteOrder() != rhs.GetByteOrder() ||
       !cores_match(GetCore(), rhs.GetCore(), true, exact_match))
     return false;
 
   const llvm::Triple &lhs_triple = GetTriple();
   const llvm::Triple &rhs_triple = rhs.GetTriple();
 
   const llvm::Triple::VendorType lhs_triple_vendor = lhs_triple.getVendor();
   const llvm::Triple::VendorType rhs_triple_vendor = rhs_triple.getVendor();
-  if (lhs_triple_vendor != rhs_triple_vendor) {
+
+  const llvm::Triple::OSType lhs_triple_os = lhs_triple.getOS();
+  const llvm::Triple::OSType rhs_triple_os = rhs_triple.getOS();
+
+  bool both_windows = lhs_triple.isOSWindows() && rhs_triple.isOSWindows();
+
+  // On Windows, the vendor field doesn't have any practical effect, but
+  // it is often set to either "pc" or "w64".
+  if ((lhs_triple_vendor != rhs_triple_vendor) &&
+      (exact_match || !both_windows)) {
     const bool rhs_vendor_specified = rhs.TripleVendorWasSpecified();
     const bool lhs_vendor_specified = TripleVendorWasSpecified();
     // Both architectures had the vendor specified, so if they aren't equal
     // then we return false
     if (rhs_vendor_specified && lhs_vendor_specified)
       return false;
 
     // Only fail if both vendor types are not unknown
     if (lhs_triple_vendor != llvm::Triple::UnknownVendor &&
         rhs_triple_vendor != llvm::Triple::UnknownVendor)
       return false;
   }
 
-  const llvm::Triple::OSType lhs_triple_os = lhs_triple.getOS();
-  const llvm::Triple::OSType rhs_triple_os = rhs_triple.getOS();
   const llvm::Triple::EnvironmentType lhs_triple_env =
       lhs_triple.getEnvironment();
   const llvm::Triple::EnvironmentType rhs_triple_env =
       rhs_triple.getEnvironment();
 
   if (!exact_match) {
     // x86_64-apple-ios-macabi, x86_64-apple-macosx are compatible, no match.
     if ((lhs_triple_os == llvm::Triple::IOS &&
          lhs_triple_env == llvm::Triple::MacABI &&
          rhs_triple_os == llvm::Triple::MacOSX) ||
if (lhs_os_specified && rhs_os_specified)
       return false;
 
     // If the pair of os+env is both unspecified, match any other os+env combo.
     if (!exact_match && ((!lhs_os_specified && !lhs_triple.hasEnvironment()) ||
                          (!rhs_os_specified && !rhs_triple.hasEnvironment())))
       return true;
   }
 
+  if (!exact_match && both_windows)
+    return true; // The Windows environments (MSVC vs GNU) are compatible
+
   return IsCompatibleEnvironment(lhs_triple_env, rhs_triple_env);
 }
 
 void ArchSpec::UpdateCore() {
   llvm::StringRef arch_name(m_triple.getArchName());
   const CoreDefinition *core_def = FindCoreDefinition(arch_name);
   if (core_def) {
     m_core = core_def->core;

lldb/test/Shell/ObjectFile/PECOFF/settings-abi-i686.yaml

+## Check that i386 executables can be opened in both ABI modes.
+## I386 executables are special in the sense that the PECOFF object
+## file plugin returns two architectures for them, i386 and i686, which
+## causes more elaborate comparisons to be run against the Platform plugin's
+## architecture list. Check that this is accepted despite potential mismatches
+## in the environment part of the triple.
+
+# RUN: yaml2obj %s -o %t
+
+## Default ABI is msvc:
+# RUN: %lldb -O "settings set plugin.object-file.pe-coff.abi msvc" \
+# RUN:   -f %t -o "image list --triple --basename" -o exit | \
+# RUN:   FileCheck -DABI=msvc -DFILENAME=%basename_t.tmp %s
+
+## Default ABI is gnu:
+# RUN: %lldb -O "settings set plugin.object-file.pe-coff.abi gnu" \
+# RUN:   -f %t -o "image list --triple --basename" -o exit | \
+# RUN:   FileCheck -DABI=gnu -DFILENAME=%basename_t.tmp %s
+
+# CHECK-LABEL: image list --triple --basename
+# CHECK-NEXT: i686-pc-windows-[[ABI]] [[FILENAME]]
+
+--- !COFF
+OptionalHeader:
+  AddressOfEntryPoint: 4480
+  ImageBase:       268435456
+  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_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_I386
+  Characteristics: [ IMAGE_FILE_EXECUTABLE_IMAGE, IMAGE_FILE_32BIT_MACHINE ]
+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
+  - Name:            .debug_info
+    Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_DISCARDABLE, IMAGE_SCN_MEM_READ ]
+    VirtualAddress:  16384
+    VirtualSize:     64
+    SectionData:     DEADBEEFBAADF00D
+symbols:         []
+...