[lldb] Fix invalid error message in TargetList::CreateTargetInternal

[JDevlieghere]

Summary

Currently, target create has no --platform option. However, TargetList::CreateTargetInternal which is called under the hood will return an error when either no platform or more than one is found that matches the given binary.

This patch adds the platform option, but that doesn't solve either of these errors. If more than one platform matches, specifying the platform isn't going to fix that. The current code will look at the architecture instead. So I've updated the error message to ask the user to specify an architecture. If no architecture is found, specifying a new one via platform isn't going to change that either because we already try to find one that matches the given architecture.

While I was there I also did some general cleanup/refactoring and improve the test coverage.

[JDevlieghere]

Reduce yaml

[jingham]

This LGTM with a couple of quibbles but I haven't been working on the platform support recently so we should wait till people who have done so weight in.

lldb/source/Target/TargetList.cpp

175–176

This comment was confusing before, but it's more confusing now because platform_sp might be the user specified platform or it might be the one passed in with --platform.

243

I'm not sure why you removed mention of --platform from this error because with this patch it should now be possible to disambiguate either by specifying --platform or --arch, right?

lldb/test/API/commands/target/basic/bogus.yaml

946

You don't use the debug info, might as well leave it out.

[jingham]

lldb/source/Target/TargetList.cpp

243

Ah, okay I didn't see that the platform doesn't get used everywhere in this patch.

lldb/test/API/commands/target/basic/bogus.yaml

946

Done...

[JDevlieghere]

lldb/source/Target/TargetList.cpp

243

Another reason is that this is generic code, it shouldn't mention command interpreter options. You could've called in here trough the SB API as well.

[labath]

The changes don't seem contentious to me, though I am also not very familiar with this code.

What surprised me was the OptionGroupPlatform argument to TargetList::CreateTargetInternal. It seems like a slighly unusual thing to have, as that means the equivalent SB call would have to construct an OptionGroup object even though it has nothing to do with CLI. But that is not a problem caused by this patch...

lldb/source/Commands/CommandObjectTarget.cpp

 #include "lldb/Host/OptionParser.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
 #include "lldb/Interpreter/OptionArgParser.h"
 #include "lldb/Interpreter/OptionGroupArchitecture.h"
 #include "lldb/Interpreter/OptionGroupBoolean.h"
 #include "lldb/Interpreter/OptionGroupFile.h"
 #include "lldb/Interpreter/OptionGroupFormat.h"
+#include "lldb/Interpreter/OptionGroupPlatform.h"
 #include "lldb/Interpreter/OptionGroupString.h"
 #include "lldb/Interpreter/OptionGroupUInt64.h"
 #include "lldb/Interpreter/OptionGroupUUID.h"
 #include "lldb/Interpreter/OptionGroupValueObjectDisplay.h"
 #include "lldb/Interpreter/OptionGroupVariable.h"
 #include "lldb/Interpreter/Options.h"
 #include "lldb/Symbol/CompileUnit.h"
 #include "lldb/Symbol/FuncUnwinders.h"

 class CommandObjectTargetCreate : public CommandObjectParsed {
 public:
   CommandObjectTargetCreate(CommandInterpreter &interpreter)
       : CommandObjectParsed(
             interpreter, "target create",
             "Create a target using the argument as the main executable.",
             nullptr),
         m_option_group(), m_arch_option(),
+        m_platform_options(true), // Include the --platform option.
         m_core_file(LLDB_OPT_SET_1, false, "core", 'c', 0, eArgTypeFilename,
                     "Fullpath to a core file to use for this target."),
         m_symbol_file(LLDB_OPT_SET_1, false, "symfile", 's', 0,
                       eArgTypeFilename,
                       "Fullpath to a stand alone debug "
                       "symbols file for when debug symbols "
                       "are not in the executable."),
         m_remote_file(
CommandObjectTargetCreate(CommandInterpreter &interpreter)
     // There is only one variant this argument could be; put it into the
     // argument entry.
     arg.push_back(file_arg);
 
     // Push the data for the first argument into the m_arguments vector.
     m_arguments.push_back(arg);
 
     m_option_group.Append(&m_arch_option, LLDB_OPT_SET_ALL, LLDB_OPT_SET_1);
+    m_option_group.Append(&m_platform_options, LLDB_OPT_SET_ALL, 1);
     m_option_group.Append(&m_core_file, LLDB_OPT_SET_ALL, LLDB_OPT_SET_1);
     m_option_group.Append(&m_symbol_file, LLDB_OPT_SET_ALL, LLDB_OPT_SET_1);
     m_option_group.Append(&m_remote_file, LLDB_OPT_SET_ALL, LLDB_OPT_SET_1);
     m_option_group.Append(&m_add_dependents, LLDB_OPT_SET_ALL, LLDB_OPT_SET_1);
     m_option_group.Finalize();
   }
 
   ~CommandObjectTargetCreate() override = default;
if (argc == 1 || core_file || remote_file) {
       bool must_set_platform_path = false;
 
       Debugger &debugger = GetDebugger();
 
       TargetSP target_sp;
       llvm::StringRef arch_cstr = m_arch_option.GetArchitectureName();
       Status error(debugger.GetTargetList().CreateTarget(
           debugger, file_path, arch_cstr,
-          m_add_dependents.m_load_dependent_files, nullptr, target_sp));
+          m_add_dependents.m_load_dependent_files, &m_platform_options,
+          target_sp));
 
       if (target_sp) {
         // Only get the platform after we create the target because we might
         // have switched platforms depending on what the arguments were to
         // CreateTarget() we can't rely on the selected platform.
 
         PlatformSP platform_sp = target_sp->GetPlatform();
 
if (argc == 1 || core_file || remote_file) {
       result.SetStatus(eReturnStatusFailed);
     }
     return result.Succeeded();
   }
 
 private:
   OptionGroupOptions m_option_group;
   OptionGroupArchitecture m_arch_option;
+  OptionGroupPlatform m_platform_options;
   OptionGroupFile m_core_file;
   OptionGroupFile m_symbol_file;
   OptionGroupFile m_remote_file;
   OptionGroupDependents m_add_dependents;
 };
 
 #pragma mark CommandObjectTargetList
 

lldb/source/Target/TargetList.cpp

 }
 
 Status TargetList::CreateTargetInternal(
     Debugger &debugger, llvm::StringRef user_exe_path,
     llvm::StringRef triple_str, LoadDependentFiles load_dependent_files,
     const OptionGroupPlatform *platform_options, TargetSP &target_sp,
     bool is_dummy_target) {
   Status error;
-  PlatformSP platform_sp;
 
-  // This is purposely left empty unless it is specified by triple_cstr. If not
-  // initialized via triple_cstr, then the currently selected platform will set
-  // the architecture correctly.
+  // Let's start by looking at the selected platform.
+  PlatformSP platform_sp = debugger.GetPlatformList().GetSelectedPlatform();
+
+  // This variable corresponds to the architecture specified by the triple
+  // string. If that string was empty the currently selected platform will
+  // determine the architecture.
   const ArchSpec arch(triple_str);
-  if (!triple_str.empty()) {
-    if (!arch.IsValid()) {
-      error.SetErrorStringWithFormat("invalid triple '%s'",
-                                     triple_str.str().c_str());
-      return error;
-    }
-  }
+  if (!triple_str.empty() && !arch.IsValid()) {
 
   ArchSpec platform_arch(arch);
 
-  bool prefer_platform_arch = false;
-
-  CommandInterpreter &interpreter = debugger.GetCommandInterpreter();
-
-  // let's see if there is already an existing platform before we go creating
-  // another...
-  platform_sp = debugger.GetPlatformList().GetSelectedPlatform();
-
-  if (platform_options && platform_options->PlatformWasSpecified()) {
-    // Create a new platform if it doesn't match the selected platform
-    if (!platform_options->PlatformMatches(platform_sp)) {
-      const bool select_platform = true;
-      platform_sp = platform_options->CreatePlatformWithOptions(
-          interpreter, arch, select_platform, error, platform_arch);
-      if (!platform_sp)
-        return error;
-    }
-  }
+  // Create a new platform if a platform was specified in the platform options
+  // and doesn't match the selected platform.
+  if (platform_options && platform_options->PlatformWasSpecified() &&
+      !platform_options->PlatformMatches(platform_sp)) {
+        debugger.GetCommandInterpreter(), arch, select_platform, error,
+        platform_arch);
+
+  bool prefer_platform_arch = false;
 
   if (!user_exe_path.empty()) {
-    ModuleSpecList module_specs;
-    ModuleSpec module_spec;
-    module_spec.GetFileSpec().SetFile(user_exe_path, FileSpec::Style::native);
+    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
+    // 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
+              // accurate.
               if (!platform_arch.TripleOSWasSpecified() ||
                   !platform_arch.TripleVendorWasSpecified()) {
                 prefer_platform_arch = true;
                 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
+            // Only one arch and none was specified.
             prefer_platform_arch = true;
             platform_arch = matching_module_spec.GetArchitecture();
           }
         }
-      } else {
-        if (arch.IsValid()) {
-          module_spec.GetArchitecture() = arch;
-          if (module_specs.FindMatchingModuleSpec(module_spec,
-                                                  matching_module_spec)) {
-            prefer_platform_arch = true;
-            platform_arch = matching_module_spec.GetArchitecture();
-          }
-        } else {
-          // No architecture specified, check if there is only one platform for
-          // all of the architectures.
-
-          typedef std::vector<PlatformSP> PlatformList;
-          PlatformList platforms;
-          PlatformSP host_platform_sp = Platform::GetHostPlatform();
-          for (size_t i = 0; i < num_specs; ++i) {
-            ModuleSpec module_spec;
-            if (module_specs.GetModuleSpecAtIndex(i, module_spec)) {
-              // See if there was a selected platform and check that first
-              // since the user may have specified it.
-              if (platform_sp) {
-                if (platform_sp->IsCompatibleArchitecture(
-                        module_spec.GetArchitecture(), false, nullptr)) {
-                  platforms.push_back(platform_sp);
-                  continue;
-                }
-              }
+      } else if (arch.IsValid()) {
+        // A (valid) architecture was specified.
+        std::vector<PlatformSP> platforms;
+            // First consider the platform specified by the user, if any, and
+            // the selected platform otherwise.
 
-              // Next check the host platform it if wasn't already checked
-              // above
-              if (host_platform_sp &&
-                  (!platform_sp ||
-                   host_platform_sp->GetName() != platform_sp->GetName())) {
-                if (host_platform_sp->IsCompatibleArchitecture(
-                        module_spec.GetArchitecture(), false, nullptr)) {
-                  platforms.push_back(host_platform_sp);
-                  continue;
-                }
-              }
+            // Now consider the host platform if it is different from the
+            // specified/selected platform.
 
-              // Just find a platform that matches the architecture in the
-              // executable file
-              PlatformSP fallback_platform_sp(
-                  Platform::GetPlatformForArchitecture(
-                      module_spec.GetArchitecture(), nullptr));
-              if (fallback_platform_sp) {
-                platforms.push_back(fallback_platform_sp);
-              }
-            }
-          }
+            // Finally find a platform that matches the architecture in the
+            // executable file.
 
-          Platform *platform_ptr = nullptr;
-          bool more_than_one_platforms = false;
-          for (const auto &the_platform_sp : platforms) {
-            if (platform_ptr) {
-              if (platform_ptr->GetName() != the_platform_sp->GetName()) {
-                more_than_one_platforms = true;
-                platform_ptr = nullptr;
-                break;
-              }
-            } else {
-              platform_ptr = the_platform_sp.get();
-            }
-          }
 
-          if (platform_ptr) {
-            // All platforms for all modules in the executable match, so we can
-            // select this platform
-            platform_sp = platforms.front();
-          } else if (!more_than_one_platforms) {
-            // No platforms claim to support this file
-            error.SetErrorString("No matching platforms found for this file, "
-                                 "specify one with the --platform option");
-            return error;
-          } else {
-            // More than one platform claims to support this file, so the
-            // --platform option must be specified
-            StreamString error_strm;
-            std::set<Platform *> platform_set;
-            error_strm.Printf(
-                "more than one platform supports this executable (");
-            for (const auto &the_platform_sp : platforms) {
-              if (platform_set.find(the_platform_sp.get()) ==
-                  platform_set.end()) {
-                if (!platform_set.empty())
-                  error_strm.PutCString(", ");
-                error_strm.PutCString(the_platform_sp->GetName().GetCString());
-                platform_set.insert(the_platform_sp.get());
-              }
-            }
-            error_strm.Printf(
-                "), use the --platform option to specify a platform");
-            error.SetErrorString(error_strm.GetString());
-            return error;
-          }
-        }
-      }
-    }
-  }
+          // select this platform.
+          // No platforms claim to support this file.
+          error.SetErrorString("no matching platforms found for this file");
+          // More than one platform claims to support this file.
+          error_strm.Printf("), specify an architecture to disambiguate");
 
   // If we have a valid architecture, make sure the current platform is
-  // compatible with that architecture
+  // compatible with that architecture.
   if (!prefer_platform_arch && arch.IsValid()) {
     if (!platform_sp->IsCompatibleArchitecture(arch, false, &platform_arch)) {
       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
+    // 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)) {
       platform_sp = Platform::GetPlatformForArchitecture(platform_arch,
                                                          &fixed_platform_arch);
       if (!is_dummy_target && platform_sp)
         debugger.GetPlatformList().SetSelectedPlatform(platform_sp);
     }
   }
 
   if (!platform_arch.IsValid())
     platform_arch = arch;
 
-  error = TargetList::CreateTargetInternal(
+  return TargetList::CreateTargetInternal(
       debugger, user_exe_path, platform_arch, load_dependent_files, platform_sp,
       target_sp, is_dummy_target);
-  return error;
 }
 
 lldb::TargetSP TargetList::GetDummyTarget(lldb_private::Debugger &debugger) {
   // FIXME: Maybe the dummy target should be per-Debugger
   if (!m_dummy_target_sp || !m_dummy_target_sp->IsValid()) {
     ArchSpec arch(Target::GetDefaultArchitecture());
     if (!arch.IsValid())
       arch = HostInfo::GetArchitecture();

lldb/test/API/commands/target/basic/TestTargetCommand.py

def do_target_command(self):
 
         self.runCmd("target select %d" % (base + 1))
         self.expect("thread backtrace", STOPPED_DUE_TO_BREAKPOINT,
                     substrs=['stop reason = breakpoint', 'b.c:%d' % self.line_b
                              ])
 
         self.runCmd("target list")
 
+    @no_debug_info_test
+    def test_target_create_invalid_arch(self):
+        exe = self.getBuildArtifact("a.out")
+        self.expect("target create {} --arch doesntexist".format(exe), error=True,
+                    patterns=["error: invalid triple 'doesntexist'"])
+
+    @no_debug_info_test
+    def test_target_create_platform(self):
+        self.buildB()
+        exe = self.getBuildArtifact("b.out")
+        self.expect("target create {} --platform host".format(exe))
+
+    @no_debug_info_test
+    def test_target_create_unsupported_platform(self):
+        yaml = os.path.join(self.getSourceDir(), "bogus.yaml")
+        exe = self.getBuildArtifact("bogus")
+        self.yaml2obj(yaml, exe)
+        self.expect("target create {}".format(exe), error=True,
+                    patterns=['error: no matching platforms found for this file'])
+
+    @no_debug_info_test
+    def test_target_create_invalid_platform(self):
+        self.buildB()
+        exe = self.getBuildArtifact("b.out")
+        self.expect("target create {} --platform doesntexist".format(exe), error=True,
+                    patterns=['error: unable to find a plug-in for the platform named "doesntexist"'])
+
     def do_target_variable_command(self, exe_name):
         """Exercise 'target variable' command before and after starting the inferior."""
         self.runCmd("file " + self.getBuildArtifact(exe_name),
                     CURRENT_EXECUTABLE_SET)
 
         self.expect(
             "target variable my_global_char",
             VARIABLES_DISPLAYED_CORRECTLY,

lldb/test/API/commands/target/basic/bogus.yaml

+--- !fat-mach-o
+FatHeader:
+  magic:           0xCAFEBABE
+  nfat_arch:       3
+FatArchs:
+  - cputype:         0x0000000C
+    cpusubtype:      0x00000009
+    offset:          0x0000000000004000
+    size:            200
+    align:           14
+  - cputype:         0x0000000C
+    cpusubtype:      0x0000000B
+    offset:          0x0000000000008000
+    size:            200
+    align:           14
+  - cputype:         0x0100000C
+    cpusubtype:      0x00000000
+    offset:          0x000000000000C000
+    size:            332
+    align:           14
+Slices:
+  - !mach-o
+    FileHeader:
+      magic:           0xFEEDFACE
+      # Bogus
+      cputype:         0x00000003
+      cpusubtype:      0x00000009
+      filetype:        0x00000001
+      ncmds:           4
+      sizeofcmds:      112
+      flags:           0x00002000
+    LoadCommands:
+      - cmd:             LC_SEGMENT
+        cmdsize:         56
+        segname:         ''
+        vmaddr:          0
+        vmsize:          0
+        fileoff:         0
+        filesize:        0
+        maxprot:         7
+        initprot:        7
+        nsects:          0
+        flags:           0
+      - cmd:             LC_SYMTAB
+        cmdsize:         24
+        symoff:          172
+        nsyms:           1
+        stroff:          184
+        strsize:         16
+      - cmd:             LC_VERSION_MIN_IPHONEOS
+        cmdsize:         16
+        version:         327680
+        sdk:             0
+      - cmd:             LC_DATA_IN_CODE
+        cmdsize:         16
+        dataoff:         172
+        datasize:        0
+    LinkEditData:
+      NameList:
+        - n_strx:          4
+          n_type:          0x01
+          n_sect:          0
+          n_desc:          512
+          n_value:         4
+      StringTable:
+        - ''
+        - ''
+        - ''
+        - ''
+        - _armv7_var
+        - ''
+  - !mach-o
+    FileHeader:
+      magic:           0xFEEDFACE
+      # Bogus
+      cputype:         0x00000002
+      cpusubtype:      0x0000000B
+      filetype:        0x00000001
+      ncmds:           4
+      sizeofcmds:      112
+      flags:           0x00002000
+    LoadCommands:
+      - cmd:             LC_SEGMENT
+        cmdsize:         56
+        segname:         ''
+        vmaddr:          0
+        vmsize:          0
+        fileoff:         0
+        filesize:        0
+        maxprot:         7
+        initprot:        7
+        nsects:          0
+        flags:           0
+      - cmd:             LC_SYMTAB
+        cmdsize:         24
+        symoff:          172
+        nsyms:           1
+        stroff:          184
+        strsize:         16
+      - cmd:             LC_VERSION_MIN_IPHONEOS
+        cmdsize:         16
+        version:         327680
+        sdk:             0
+      - cmd:             LC_DATA_IN_CODE
+        cmdsize:         16
+        dataoff:         172
+        datasize:        0
+    LinkEditData:
+      NameList:
+        - n_strx:          4
+          n_type:          0x01
+          n_sect:          0
+          n_desc:          512
+          n_value:         4
+      StringTable:
+        - ''
+        - ''
+        - ''
+        - ''
+        - _armv7s_var
+  - !mach-o
+    FileHeader:
+      magic:           0xFEEDFACF
+      # Bogus
+      cputype:         0x00000001
+      cpusubtype:      0x00000000
+      filetype:        0x00000001
+      ncmds:           4
+      sizeofcmds:      208
+      flags:           0x00002000
+      reserved:        0x00000000
+    LoadCommands:
+      - cmd:             LC_SEGMENT_64
+        cmdsize:         152
+        segname:         ''
+        vmaddr:          0
+        vmsize:          0
+        fileoff:         272
+        filesize:        0
+        maxprot:         7
+        initprot:        7
+        nsects:          1
+        flags:           0
+        Sections:
+          - sectname:        __text
+            segname:         __TEXT
+            addr:            0x0000000000000000
+            size:            0
+            offset:          0x00000110
+            align:           0
+            reloff:          0x00000000
+            nreloc:          0
+            flags:           0x80000400
+            reserved1:       0x00000000
+            reserved2:       0x00000000
+            reserved3:       0x00000000
+            content:         ''
+      - cmd:             LC_SYMTAB
+        cmdsize:         24
+        symoff:          276
+        nsyms:           2
+        stroff:          308
+        strsize:         24
+      - cmd:             LC_VERSION_MIN_IPHONEOS
+        cmdsize:         16
+        version:         327680
+        sdk:             0
+      - cmd:             LC_DATA_IN_CODE
+        cmdsize:         16
+        dataoff:         276
+        datasize:        0
+    LinkEditData:
+      NameList:
+        - n_strx:          15
+          n_type:          0x0E
+          n_sect:          1
+          n_desc:          0
+          n_value:         0
+        - n_strx:          4
+          n_type:          0x01
+          n_sect:          0
+          n_desc:          512
+          n_value:         4
+      StringTable:
+        - ''
+        - ''
+        - ''
+        - ''
+        - _arm64_var
+        - ltmp0
+        - ''
+        - ''
+        - ''
+...