Unify the code that updates the ArchSpec after finding a fat binary with how it is done for a lean binary

[aprantl]

Summary

In particular this affects how target create --arch is handled — it allowed us to override the deployment target (a useful feature for the expression evaluator), but the fat binary case didn't.

rdar://problem/66024437

[aprantl]

It would be nice to (in a follow-up-patch) clearly state what this function's goal is and then re-implement it with slightly fewer fallbacks. The code as it is is very convoluted.

[davide]

lldb/source/Target/TargetList.cpp

110–111

Is this check strict enough? I thought it should be only TripleOSWasSpecified -- what we can infer from the vendor?

[aprantl]

lldb/source/Target/TargetList.cpp

159

this is wrong. it should be at line 132!

Thanks @jingham !

[aprantl]

Address review feedback.

[aprantl]

lldb/source/Target/TargetList.cpp

110–111

Git archeology says that this comes from 3f19ada88e24008b24a5db20b7f78f5cff525a0b / SVN r212783.
Quoting: - Fixed the coded in TargetList::CreateTarget() so it does the right thing with an underspecified triple where just the arch is specified.

I think it's specifically supposed to detect a triple with just a CPU, which is what we get from target create --arch. I think your comment still rings true.

[jasonmolenda]

lldb/source/Target/TargetList.cpp

140–141

Should this conditional be replaced with your update_platform_arch lambda?

[jasonmolenda]

LGTM, it's hard to keep all the supported behaviors in my head but I think this is right.

[aprantl]

lldb/source/Target/TargetList.cpp

140–141

Yes!

lldb/source/Target/TargetList.cpp

if (platform_options && platform_options->PlatformWasSpecified() &&
     platform_sp = platform_options->CreatePlatformWithOptions(
         debugger.GetCommandInterpreter(), arch, select_platform, error,
         platform_arch);
     if (!platform_sp)
       return error;
   }
 
   bool prefer_platform_arch = false;
+  auto update_platform_arch = [&](const ArchSpec &module_arch) {
+    // If the OS or vendor weren't specified, then adopt the module's
+    // architecture so that the platform matching can be more accurate.
+    if (!platform_arch.TripleOSWasSpecified() ||
+        !platform_arch.TripleVendorWasSpecified()) {
+      prefer_platform_arch = true;
+      platform_arch = module_arch;
+    }
+  };
 
   if (!user_exe_path.empty()) {
     ModuleSpec module_spec(FileSpec(user_exe_path, FileSpec::Style::native));
     FileSystem::Instance().Resolve(module_spec.GetFileSpec());
     // Resolve the executable in case we are given a path to a application
     // bundle like a .app bundle on MacOSX.
     Host::ResolveExecutableInBundle(module_spec.GetFileSpec());
 
     lldb::offset_t file_offset = 0;
     lldb::offset_t file_size = 0;
     ModuleSpecList module_specs;
     const size_t num_specs = ObjectFile::GetModuleSpecifications(
         module_spec.GetFileSpec(), file_offset, file_size, module_specs);
 
     if (num_specs > 0) {
       ModuleSpec matching_module_spec;
 
       if (num_specs == 1) {
         if (module_specs.GetModuleSpecAtIndex(0, matching_module_spec)) {
           if (platform_arch.IsValid()) {
             if (platform_arch.IsCompatibleMatch(
                     matching_module_spec.GetArchitecture())) {
               // If the OS or vendor weren't specified, then adopt the module's
               // architecture so that the platform matching can be more
               // accurate.
-              if (!platform_arch.TripleOSWasSpecified() ||
-                  !platform_arch.TripleVendorWasSpecified()) {
-                prefer_platform_arch = true;
-                platform_arch = matching_module_spec.GetArchitecture();
-              }
+              update_platform_arch(matching_module_spec.GetArchitecture());
             } else {
               StreamString platform_arch_strm;
               StreamString module_arch_strm;
 
               platform_arch.DumpTriple(platform_arch_strm.AsRawOstream());
               matching_module_spec.GetArchitecture().DumpTriple(
                   module_arch_strm.AsRawOstream());
               error.SetErrorStringWithFormat(
                   "the specified architecture '%s' is not compatible with '%s' "
                   "in '%s'",
                   platform_arch_strm.GetData(), module_arch_strm.GetData(),
                   module_spec.GetFileSpec().GetPath().c_str());
               return error;
             }
           } else {
             // Only one arch and none was specified.
             prefer_platform_arch = true;
             platform_arch = matching_module_spec.GetArchitecture();
           }
         }
       } else if (arch.IsValid()) {
-        // A (valid) architecture was specified.
+        // Fat binary. A (valid) architecture was specified.
         module_spec.GetArchitecture() = arch;
         if (module_specs.FindMatchingModuleSpec(module_spec,
-                                                matching_module_spec)) {
-          prefer_platform_arch = true;
-          platform_arch = matching_module_spec.GetArchitecture();
-        }
+                                                matching_module_spec))
+            update_platform_arch(matching_module_spec.GetArchitecture());
       } else {
-        // No architecture specified, check if there is only one platform for
-        // all of the architectures.
+        // Fat binary. No architecture specified, check if there is
+        // only one platform for all of the architectures.
         PlatformSP host_platform_sp = Platform::GetHostPlatform();
         std::vector<PlatformSP> platforms;
         for (size_t i = 0; i < num_specs; ++i) {
           ModuleSpec module_spec;
           if (module_specs.GetModuleSpecAtIndex(i, module_spec)) {
             // First consider the platform specified by the user, if any, and
             // the selected platform otherwise.
             if (platform_sp) {
if (num_specs > 0) {
         }
       }
     }
   }
 
   // If we have a valid architecture, make sure the current platform is
   // compatible with that architecture.
   if (!prefer_platform_arch && arch.IsValid()) {
-    if (!platform_sp->IsCompatibleArchitecture(arch, false, &platform_arch)) {
+    if (!platform_sp->IsCompatibleArchitecture(arch, false, nullptr)) {
       platform_sp = Platform::GetPlatformForArchitecture(arch, &platform_arch);
       if (!is_dummy_target && platform_sp)
         debugger.GetPlatformList().SetSelectedPlatform(platform_sp);
     }
   } else if (platform_arch.IsValid()) {
     // If "arch" isn't valid, yet "platform_arch" is, it means we have an
     // executable file with a single architecture which should be used.
     ArchSpec fixed_platform_arch;
-    if (!platform_sp->IsCompatibleArchitecture(platform_arch, false,
-                                               &fixed_platform_arch)) {
+    if (!platform_sp->IsCompatibleArchitecture(platform_arch, false, nullptr)) {
       platform_sp = Platform::GetPlatformForArchitecture(platform_arch,
                                                          &fixed_platform_arch);
       if (!is_dummy_target && platform_sp)
         debugger.GetPlatformList().SetSelectedPlatform(platform_sp);
     }
   }
 
   if (!platform_arch.IsValid())

lldb/test/API/macosx/universal/Makefile

 EXE := testit
 
 include Makefile.rules
 
 all: testit
 
 testit: testit.x86_64h testit.x86_64
   lipo -create -o testit $^
 
 testit.x86_64h: testit.x86_64h.o
-  $(CC) -isysroot $(SDKROOT) -arch x86_64h -o testit.x86_64h $<
+  $(CC) -isysroot $(SDKROOT) -target x86_64h-apple-macosx10.9 -o testit.x86_64h $<
 
 testit.x86_64: testit.x86_64.o
-  $(CC) -isysroot $(SDKROOT) -arch x86_64 -o testit.x86_64 $<
+  $(CC) -isysroot $(SDKROOT) -target x86_64-apple-macosx10.9 -o testit.x86_64 $<
 
 testit.x86_64h.o: main.c
-  $(CC) -isysroot $(SDKROOT) -g -O0 -arch x86_64h -c -o testit.x86_64h.o $<
+  $(CC) -isysroot $(SDKROOT) -g -O0 -target x86_64h-apple-macosx10.9-apple-macosx10.9-apple-macosx10.9-apple-macosx10.9 -c -o testit.x86_64h.o $<
 
 testit.x86_64.o: main.c
-  $(CC) -isysroot $(SDKROOT) -g -O0 -arch x86_64 -c -o testit.x86_64.o $<
+  $(CC) -isysroot $(SDKROOT) -g -O0 -target x86_64-apple-macosx10.9 -c -o testit.x86_64.o $<

lldb/test/API/macosx/universal/TestUniversal.py

-"""Test aspects of lldb commands on universal binaries."""
-
-
-
 import unittest2
 import os
 import lldb
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
 
 def haswellOrLater():
     features = subprocess.check_output(["sysctl", "machdep.cpu"])
     return "AVX2" in features.split()
 
 class UniversalTestCase(TestBase):
+    """Test aspects of lldb commands on universal binaries."""
 
     NO_DEBUG_INFO_TESTCASE = True
     mydir = TestBase.compute_mydir(__file__)
 
     def setUp(self):
         # Call super's setUp().
         TestBase.setUp(self)
         # Find the line number to break inside main().
def test_sbdebugger_create_target_with_file_and_target_triple(self):
         # Invoke the default build rule.
         self.build()
 
         # Note that "testit" is a universal binary.
         exe = self.getBuildArtifact("testit")
 
         # Create a target by the debugger.
         target = self.dbg.CreateTargetWithFileAndTargetTriple(
-            exe, "x86_64-apple-macosx")
+            exe, "x86_64-apple-macosx10.10")
         self.assertTrue(target, VALID_TARGET)
-        self.expect("image list -A -b", substrs=["x86_64 testit"])
+        self.expect("image list -t -b", substrs=["x86_64-apple-macosx10.9.0 testit"])
+        self.expect("target list", substrs=["testit", "arch=x86_64-apple-macosx10.10"])
 
         # Now launch the process, and do not stop at entry point.
         process = target.LaunchSimple(
             None, None, self.get_process_working_directory())
         self.assertTrue(process, PROCESS_IS_VALID)
 
     @skipUnlessDarwin
     @unittest2.skipUnless(hasattr(os, "uname") and os.uname()[4] in