[TargetList] Simplify dummy target creation

[vsk]

Summary

Factor out dummy target creation from CreateTargetInternal.

This makes it impossible for dummy target creation to accidentally fail
due to too-strict checking in one of the CreateTargetInternal overloads.

Testing: check-lldb

rdar://70630655

[vsk]

Delete some dead code.

[JDevlieghere]

lldb/source/Target/TargetList.cpp

293

I wonder if we gain anything by making this lazy. Maybe it's time to address that FIXME and always have a single DummyTarget held onto by the Debugger?

[vsk]

lldb/source/Target/TargetList.cpp

293

You're right, that's even better.

[vsk]

Make the dummy target per-debugger.

[jingham]

lldb/source/Target/TargetList.cpp

293

I see no reason not to just make a dummy target when you make a debugger. That should be a very cheap operation, and the object itself should not be large.

I think for regularities sake it's better to have the TargetList have all the targets, including the Dummy target, rather than have the DummyTarget held specially by the debugger. But it would be easy for the Debugger to make a DummyTarget and inject it into the TargetList as part of its startup. We could even make the straight constructor of TargetList do this job. We copy the debugger TargetList to iterate over it and for similar purposes in a bunch of places, but that goes through the copy constructor so it wouldn't interfere with this.

[vsk]

lldb/source/Target/TargetList.cpp

293

So far, the policy has been to not include the dummy target in the list of targets (i.e, you can't find it via TargetList::GetTargetAtIndex or TargetList::GetIndexOfTarget). If, down the line, we need to support that we can add the functionality though.

[jingham]

Oh, my bad, apparently we were leaving the dummy target out of the TargetList. That's a little odd, but I probably did it so that's not so unusual... Ignore my comments...

[teemperor]

LGTM.

lldb/source/Core/Debugger.cpp

685 ↗

(On Diff #303252)

Maybe have a small comment here that summarizes the block: // Create dummy target?

[JDevlieghere]

lldb/source/Core/Debugger.cpp

692 ↗

(On Diff #303252)

Should we move all this into Target and have a private static TargetSP GetDummyTarget()?

[vsk]

lldb/source/Core/Debugger.cpp

685 ↗

(On Diff #303252)

I'll either fix this in a more substantial patch revision or before committing.

692 ↗

(On Diff #303252)

I personally find that having the target creation inlined in the Debugger constructor is nice: it precludes some other code from setting up a separate dummy target.

lldb/include/lldb/Target/TargetList.h

 //===-- TargetList.h --------------------------------------------*- C++ -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_TARGET_TARGETLIST_H
 #define LLDB_TARGET_TARGETLIST_H
 
 #include <mutex>
 #include <vector>
 
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/Broadcaster.h"
 
 namespace lldb_private {
 
 class TargetList : public Broadcaster {
 private:
   friend class Debugger;
 
protected:
   collection m_target_list;
   lldb::TargetSP m_dummy_target_sp;
   mutable std::recursive_mutex m_target_list_mutex;
   uint32_t m_selected_target_idx;
 
 private:
   lldb::TargetSP GetDummyTarget(lldb_private::Debugger &debugger);
 
-  Status CreateDummyTarget(Debugger &debugger,
-                           llvm::StringRef specified_arch_name,
-                           lldb::TargetSP &target_sp);
+  /// Create a dummy target. Guaranteed to succeed. May only be called once per
+  /// TargetList.
+  void CreateDummyTarget(Debugger &debugger, ArchSpec arch,
 
   Status CreateTargetInternal(Debugger &debugger, llvm::StringRef user_exe_path,
                               llvm::StringRef triple_str,
                               LoadDependentFiles load_dependent_files,
                               const OptionGroupPlatform *platform_options,
-                              lldb::TargetSP &target_sp, bool is_dummy_target);
+                              lldb::TargetSP &target_sp);
 
   Status CreateTargetInternal(Debugger &debugger, llvm::StringRef user_exe_path,
                               const ArchSpec &arch,
                               LoadDependentFiles get_dependent_modules,
                               lldb::PlatformSP &platform_sp,
-                              lldb::TargetSP &target_sp, bool is_dummy_target);
+                              lldb::TargetSP &target_sp);
 
   TargetList(const TargetList &) = delete;
   const TargetList &operator=(const TargetList &) = delete;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_TARGET_TARGETLIST_H

lldb/source/Target/TargetList.cpp

 //===-- TargetList.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 "lldb/Target/TargetList.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/OptionGroupPlatform.h"
 #include "lldb/Symbol/ObjectFile.h"

 
 Status TargetList::CreateTarget(Debugger &debugger,
                                 llvm::StringRef user_exe_path,
                                 llvm::StringRef triple_str,
                                 LoadDependentFiles load_dependent_files,
                                 const OptionGroupPlatform *platform_options,
                                 TargetSP &target_sp) {
   return CreateTargetInternal(debugger, user_exe_path, triple_str,
-                              load_dependent_files, platform_options, target_sp,
-                              false);
+                              load_dependent_files, platform_options,
+                              target_sp);
 }
 
 Status TargetList::CreateTarget(Debugger &debugger,
                                 llvm::StringRef user_exe_path,
                                 const ArchSpec &specified_arch,
                                 LoadDependentFiles load_dependent_files,
                                 PlatformSP &platform_sp, TargetSP &target_sp) {
   return CreateTargetInternal(debugger, user_exe_path, specified_arch,
-                              load_dependent_files, platform_sp, target_sp,
-                              false);
+                              load_dependent_files, platform_sp, target_sp);
+}
+
+void TargetList::CreateDummyTarget(Debugger &debugger, ArchSpec arch,
+                                   lldb::TargetSP &target_sp) {
+  assert(!m_dummy_target_sp && "attempted to create a second dummy target");
+  const bool is_dummy_target = true;
+  PlatformSP host_platform_sp(Platform::GetHostPlatform());
+  target_sp.reset(
+      new Target(debugger, arch, host_platform_sp, is_dummy_target));
+  // Don't put the dummy target in the target list, it's held separately.
+  m_dummy_target_sp = target_sp;
 }
 
 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) {
+    const OptionGroupPlatform *platform_options, TargetSP &target_sp) {
   Status error;
 
   // 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.
if (!user_exe_path.empty()) {
     }
   }
 
   // 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, nullptr)) {
       platform_sp = Platform::GetPlatformForArchitecture(arch, &platform_arch);
-      if (!is_dummy_target && platform_sp)
+      if (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, nullptr)) {
       platform_sp = Platform::GetPlatformForArchitecture(platform_arch,
                                                          &fixed_platform_arch);
-      if (!is_dummy_target && platform_sp)
+      if (platform_sp)
         debugger.GetPlatformList().SetSelectedPlatform(platform_sp);
     }
   }
 
   if (!platform_arch.IsValid())
     platform_arch = arch;
 
-  return TargetList::CreateTargetInternal(
-      debugger, user_exe_path, platform_arch, load_dependent_files, platform_sp,
-      target_sp, is_dummy_target);
+  return TargetList::CreateTargetInternal(debugger, user_exe_path,
+                                          platform_arch, load_dependent_files,
+                                          platform_sp, target_sp);
 }
 
 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();
-    Status err = CreateDummyTarget(
-        debugger, arch.GetTriple().getTriple().c_str(), m_dummy_target_sp);
+    assert(arch.IsValid() && "No valid default or host archspec");
+    CreateDummyTarget(debugger, arch, m_dummy_target_sp);
   }
 
   return m_dummy_target_sp;
 }
 
-Status TargetList::CreateDummyTarget(Debugger &debugger,
-                                     llvm::StringRef specified_arch_name,
-                                     lldb::TargetSP &target_sp) {
-  PlatformSP host_platform_sp(Platform::GetHostPlatform());
-  return CreateTargetInternal(
-      debugger, (const char *)nullptr, specified_arch_name, eLoadDependentsNo,
-      (const OptionGroupPlatform *)nullptr, target_sp, true);
-}
-
 Status TargetList::CreateTargetInternal(Debugger &debugger,
                                         llvm::StringRef user_exe_path,
                                         const ArchSpec &specified_arch,
                                         LoadDependentFiles load_dependent_files,
                                         lldb::PlatformSP &platform_sp,
-                                        lldb::TargetSP &target_sp,
-                                        bool is_dummy_target) {
+                                        lldb::TargetSP &target_sp) {
   static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
   Timer scoped_timer(
       func_cat, "TargetList::CreateTarget (file = '%s', arch = '%s')",
       user_exe_path.str().c_str(), specified_arch.GetArchitectureName());
   Status error;
+  const bool is_dummy_target = false;
 
   ArchSpec arch(specified_arch);
 
   if (arch.IsValid()) {
     if (!platform_sp ||
         !platform_sp->IsCompatibleArchitecture(arch, false, nullptr))
       platform_sp = Platform::GetPlatformForArchitecture(specified_arch, &arch);
   }
if (!user_exe_path.empty()) {
     }
   }
   if (file.GetDirectory()) {
     FileSpec file_dir;
     file_dir.GetDirectory() = file.GetDirectory();
     target_sp->AppendExecutableSearchPaths(file_dir);
   }
 
-  // Don't put the dummy target in the target list, it's held separately.
-  if (!is_dummy_target) {
-    std::lock_guard<std::recursive_mutex> guard(m_target_list_mutex);
-    m_selected_target_idx = m_target_list.size();
-    m_target_list.push_back(target_sp);
-    // Now prime this from the dummy target:
-    target_sp->PrimeFromDummyTarget(debugger.GetDummyTarget());
-  } else {
-    m_dummy_target_sp = target_sp;
-  }
 
   return error;
 }
 
 bool TargetList::DeleteTarget(TargetSP &target_sp) {
   std::lock_guard<std::recursive_mutex> guard(m_target_list_mutex);
   collection::iterator pos, end = m_target_list.end();