[lldb] "target create" shouldn't save target if the command failed
authorTatyana Krasnukha <21970096+tkrasnukha@users.noreply.github.com>
Fri, 11 Dec 2020 08:09:39 +0000 (11:09 +0300)
committerTatyana Krasnukha <tatyana@synopsys.com>
Sat, 12 Dec 2020 13:40:58 +0000 (16:40 +0300)
TargetList::CreateTarget automatically adds created target to the list, however,
CommandObjectTargetCreate does some additional preparation after creating a target
and which can fail. The command should remove created target if it failed. Since
the function has many ways to return, scope guard does this work safely.

Changes to the TargetList make target adding and selection more transparent.

Other changes remove unnecessary SetSelectedTarget after CreateTarget.

Differential Revision: https://reviews.llvm.org/D93052

12 files changed:
lldb/include/lldb/Target/TargetList.h
lldb/source/API/SBDebugger.cpp
lldb/source/Commands/CommandObjectProcess.cpp
lldb/source/Commands/CommandObjectTarget.cpp
lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
lldb/source/Target/Platform.cpp
lldb/source/Target/TargetList.cpp
lldb/source/Target/TraceSessionFileParser.cpp
lldb/unittests/Process/ProcessEventDataTest.cpp
lldb/unittests/Thread/ThreadTest.cpp

index 94b25f6..903ca4b 100644 (file)
@@ -173,7 +173,9 @@ public:
 
   uint32_t SignalIfRunning(lldb::pid_t pid, int signo);
 
-  uint32_t SetSelectedTarget(Target *target);
+  void SetSelectedTarget(uint32_t index);
+
+  void SetSelectedTarget(const lldb::TargetSP &target);
 
   lldb::TargetSP GetSelectedTarget();
 
@@ -185,17 +187,21 @@ protected:
   uint32_t m_selected_target_idx;
 
 private:
-  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);
-
-  Status CreateTargetInternal(Debugger &debugger, llvm::StringRef user_exe_path,
-                              const ArchSpec &arch,
-                              LoadDependentFiles get_dependent_modules,
-                              lldb::PlatformSP &platform_sp,
-                              lldb::TargetSP &target_sp);
+  static 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);
+
+  static Status CreateTargetInternal(Debugger &debugger,
+                                     llvm::StringRef user_exe_path,
+                                     const ArchSpec &arch,
+                                     LoadDependentFiles get_dependent_modules,
+                                     lldb::PlatformSP &platform_sp,
+                                     lldb::TargetSP &target_sp);
+
+  void AddTargetInternal(lldb::TargetSP target_sp, bool do_select);
+
+  void SetSelectedTargetInternal(uint32_t index);
 
   TargetList(const TargetList &) = delete;
   const TargetList &operator=(const TargetList &) = delete;
index a5bf457..dc1cc91 100644 (file)
@@ -811,10 +811,8 @@ SBTarget SBDebugger::CreateTargetWithFileAndArch(const char *filename,
         add_dependent_modules ? eLoadDependentsYes : eLoadDependentsNo, nullptr,
         target_sp);
 
-    if (error.Success()) {
-      m_opaque_sp->GetTargetList().SetSelectedTarget(target_sp.get());
+    if (error.Success())
       sb_target.SetSP(target_sp);
-    }
   }
 
   LLDB_LOGF(log,
@@ -840,10 +838,8 @@ SBTarget SBDebugger::CreateTarget(const char *filename) {
         add_dependent_modules ? eLoadDependentsYes : eLoadDependentsNo, nullptr,
         target_sp);
 
-    if (error.Success()) {
-      m_opaque_sp->GetTargetList().SetSelectedTarget(target_sp.get());
+    if (error.Success())
       sb_target.SetSP(target_sp);
-    }
   }
   Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
   LLDB_LOGF(log,
@@ -998,7 +994,7 @@ void SBDebugger::SetSelectedTarget(SBTarget &sb_target) {
 
   TargetSP target_sp(sb_target.GetSP());
   if (m_opaque_sp) {
-    m_opaque_sp->GetTargetList().SetSelectedTarget(target_sp.get());
+    m_opaque_sp->GetTargetList().SetSelectedTarget(target_sp);
   }
   if (log) {
     SBStream sstr;
index 5ef0b87..1eef280 100644 (file)
@@ -364,7 +364,6 @@ protected:
         result.AppendError(error.AsCString("Error creating target"));
         return false;
       }
-      GetDebugger().GetTargetList().SetSelectedTarget(target);
     }
 
     // Record the old executable module, we want to issue a warning if the
index eba129c..c033493 100644 (file)
@@ -50,6 +50,7 @@
 #include "lldb/Utility/State.h"
 #include "lldb/Utility/Timer.h"
 
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/FormatAdapters.h"
 
@@ -318,123 +319,124 @@ protected:
           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 (remote_file) {
-          if (platform_sp) {
-            // I have a remote file.. two possible cases
-            if (file_spec && FileSystem::Instance().Exists(file_spec)) {
-              // if the remote file does not exist, push it there
-              if (!platform_sp->GetFileExists(remote_file)) {
-                Status err = platform_sp->PutFile(file_spec, remote_file);
-                if (err.Fail()) {
-                  result.AppendError(err.AsCString());
-                  result.SetStatus(eReturnStatusFailed);
-                  return false;
-                }
-              }
-            } else {
-              // there is no local file and we need one
-              // in order to make the remote ---> local transfer we need a
-              // platform
-              // TODO: if the user has passed in a --platform argument, use it
-              // to fetch the right platform
-              if (!platform_sp) {
-                result.AppendError(
-                    "unable to perform remote debugging without a platform");
+      if (!target_sp) {
+        result.AppendError(error.AsCString());
+        result.SetStatus(eReturnStatusFailed);
+        return false;
+      }
+
+      auto on_error = llvm::make_scope_exit(
+          [&target_list = debugger.GetTargetList(), &target_sp]() {
+            target_list.DeleteTarget(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 (remote_file) {
+        if (platform_sp) {
+          // I have a remote file.. two possible cases
+          if (file_spec && FileSystem::Instance().Exists(file_spec)) {
+            // if the remote file does not exist, push it there
+            if (!platform_sp->GetFileExists(remote_file)) {
+              Status err = platform_sp->PutFile(file_spec, remote_file);
+              if (err.Fail()) {
+                result.AppendError(err.AsCString());
                 result.SetStatus(eReturnStatusFailed);
                 return false;
               }
-              if (file_path) {
-                // copy the remote file to the local file
-                Status err = platform_sp->GetFile(remote_file, file_spec);
-                if (err.Fail()) {
-                  result.AppendError(err.AsCString());
-                  result.SetStatus(eReturnStatusFailed);
-                  return false;
-                }
-              } else {
-                // make up a local file
-                result.AppendError("remote --> local transfer without local "
-                                   "path is not implemented yet");
+            }
+          } else {
+            // there is no local file and we need one
+            // in order to make the remote ---> local transfer we need a
+            // platform
+            // TODO: if the user has passed in a --platform argument, use it
+            // to fetch the right platform
+            if (file_path) {
+              // copy the remote file to the local file
+              Status err = platform_sp->GetFile(remote_file, file_spec);
+              if (err.Fail()) {
+                result.AppendError(err.AsCString());
                 result.SetStatus(eReturnStatusFailed);
                 return false;
               }
+            } else {
+              // make up a local file
+              result.AppendError("remote --> local transfer without local "
+                                 "path is not implemented yet");
+              result.SetStatus(eReturnStatusFailed);
+              return false;
             }
-          } else {
-            result.AppendError("no platform found for target");
-            result.SetStatus(eReturnStatusFailed);
-            return false;
           }
+        } else {
+          result.AppendError("no platform found for target");
+          result.SetStatus(eReturnStatusFailed);
+          return false;
         }
+      }
 
-        if (symfile || remote_file) {
-          ModuleSP module_sp(target_sp->GetExecutableModule());
-          if (module_sp) {
-            if (symfile)
-              module_sp->SetSymbolFileFileSpec(symfile);
-            if (remote_file) {
-              std::string remote_path = remote_file.GetPath();
-              target_sp->SetArg0(remote_path.c_str());
-              module_sp->SetPlatformFileSpec(remote_file);
-            }
+      if (symfile || remote_file) {
+        ModuleSP module_sp(target_sp->GetExecutableModule());
+        if (module_sp) {
+          if (symfile)
+            module_sp->SetSymbolFileFileSpec(symfile);
+          if (remote_file) {
+            std::string remote_path = remote_file.GetPath();
+            target_sp->SetArg0(remote_path.c_str());
+            module_sp->SetPlatformFileSpec(remote_file);
           }
         }
+      }
 
-        debugger.GetTargetList().SetSelectedTarget(target_sp.get());
-        if (must_set_platform_path) {
-          ModuleSpec main_module_spec(file_spec);
-          ModuleSP module_sp =
-              target_sp->GetOrCreateModule(main_module_spec, true /* notify */);
-          if (module_sp)
-            module_sp->SetPlatformFileSpec(remote_file);
-        }
+      if (must_set_platform_path) {
+        ModuleSpec main_module_spec(file_spec);
+        ModuleSP module_sp =
+            target_sp->GetOrCreateModule(main_module_spec, true /* notify */);
+        if (module_sp)
+          module_sp->SetPlatformFileSpec(remote_file);
+      }
 
-        if (core_file) {
-          FileSpec core_file_dir;
-          core_file_dir.GetDirectory() = core_file.GetDirectory();
-          target_sp->AppendExecutableSearchPaths(core_file_dir);
+      if (core_file) {
+        FileSpec core_file_dir;
+        core_file_dir.GetDirectory() = core_file.GetDirectory();
+        target_sp->AppendExecutableSearchPaths(core_file_dir);
 
-          ProcessSP process_sp(target_sp->CreateProcess(
-              GetDebugger().GetListener(), llvm::StringRef(), &core_file,
-              false));
+        ProcessSP process_sp(target_sp->CreateProcess(
+            GetDebugger().GetListener(), llvm::StringRef(), &core_file, false));
 
-          if (process_sp) {
-            // Seems weird that we Launch a core file, but that is what we
-            // do!
-            error = process_sp->LoadCore();
+        if (process_sp) {
+          // Seems weird that we Launch a core file, but that is what we
+          // do!
+          error = process_sp->LoadCore();
 
-            if (error.Fail()) {
-              result.AppendError(
-                  error.AsCString("can't find plug-in for core file"));
-              result.SetStatus(eReturnStatusFailed);
-              return false;
-            } else {
-              result.AppendMessageWithFormatv("Core file '{0}' ({1}) was loaded.\n", core_file.GetPath(),
-                  target_sp->GetArchitecture().GetArchitectureName());
-              result.SetStatus(eReturnStatusSuccessFinishNoResult);
-            }
-          } else {
-            result.AppendErrorWithFormatv(
-                "Unable to find process plug-in for core file '{0}'\n",
-                core_file.GetPath());
+          if (error.Fail()) {
+            result.AppendError(
+                error.AsCString("can't find plug-in for core file"));
             result.SetStatus(eReturnStatusFailed);
+            return false;
+          } else {
+            result.AppendMessageWithFormatv(
+                "Core file '{0}' ({1}) was loaded.\n", core_file.GetPath(),
+                target_sp->GetArchitecture().GetArchitectureName());
+            result.SetStatus(eReturnStatusSuccessFinishNoResult);
+            on_error.release();
           }
         } else {
-          result.AppendMessageWithFormat(
-              "Current executable set to '%s' (%s).\n",
-              file_spec.GetPath().c_str(),
-              target_sp->GetArchitecture().GetArchitectureName());
-          result.SetStatus(eReturnStatusSuccessFinishNoResult);
+          result.AppendErrorWithFormatv(
+              "Unable to find process plug-in for core file '{0}'\n",
+              core_file.GetPath());
+          result.SetStatus(eReturnStatusFailed);
         }
       } else {
-        result.AppendError(error.AsCString());
-        result.SetStatus(eReturnStatusFailed);
+        result.AppendMessageWithFormat(
+            "Current executable set to '%s' (%s).\n",
+            file_spec.GetPath().c_str(),
+            target_sp->GetArchitecture().GetArchitectureName());
+        result.SetStatus(eReturnStatusSuccessFinishNoResult);
+        on_error.release();
       }
     } else {
       result.AppendErrorWithFormat("'%s' takes exactly one executable path "
@@ -442,6 +444,7 @@ protected:
                                    m_cmd_name.c_str());
       result.SetStatus(eReturnStatusFailed);
     }
+
     return result.Succeeded();
   }
 
@@ -507,18 +510,11 @@ protected:
         TargetList &target_list = GetDebugger().GetTargetList();
         const uint32_t num_targets = target_list.GetNumTargets();
         if (target_idx < num_targets) {
-          TargetSP target_sp(target_list.GetTargetAtIndex(target_idx));
-          if (target_sp) {
-            Stream &strm = result.GetOutputStream();
-            target_list.SetSelectedTarget(target_sp.get());
-            bool show_stopped_process_status = false;
-            DumpTargetList(target_list, show_stopped_process_status, strm);
-            result.SetStatus(eReturnStatusSuccessFinishResult);
-          } else {
-            result.AppendErrorWithFormat("target #%u is NULL in target list\n",
-                                         target_idx);
-            result.SetStatus(eReturnStatusFailed);
-          }
+          target_list.SetSelectedTarget(target_idx);
+          Stream &strm = result.GetOutputStream();
+          bool show_stopped_process_status = false;
+          DumpTargetList(target_list, show_stopped_process_status, strm);
+          result.SetStatus(eReturnStatusSuccessFinishResult);
         } else {
           if (num_targets > 0) {
             result.AppendErrorWithFormat(
index cad5a80..3628b0a 100644 (file)
@@ -377,7 +377,6 @@ lldb::ProcessSP PlatformPOSIX::Attach(ProcessAttachInfo &attach_info,
     }
 
     if (target && error.Success()) {
-      debugger.GetTargetList().SetSelectedTarget(target);
       if (log) {
         ModuleSP exe_module_sp = target->GetExecutableModule();
         LLDB_LOGF(log, "PlatformPOSIX::%s set selected target to %p %s",
@@ -462,9 +461,6 @@ PlatformPOSIX::DebugProcess(ProcessLaunchInfo &launch_info, Debugger &debugger,
     }
   }
 
-  // Mark target as currently selected target.
-  debugger.GetTargetList().SetSelectedTarget(target);
-
   // Now create the gdb-remote process.
   LLDB_LOG(log, "having target create process with gdb-remote plugin");
   process_sp =
index 0ef4dcb..3527fb1 100644 (file)
@@ -272,8 +272,6 @@ lldb::ProcessSP PlatformWindows::Attach(ProcessAttachInfo &attach_info,
   if (!target || error.Fail())
     return process_sp;
 
-  debugger.GetTargetList().SetSelectedTarget(target);
-
   const char *plugin_name = attach_info.GetProcessPluginName();
   process_sp = target->CreateProcess(
       attach_info.GetListenerForProcess(debugger), plugin_name, nullptr, false);
index b3774b9..a64ee26 100644 (file)
@@ -495,8 +495,6 @@ lldb::ProcessSP PlatformRemoteGDBServer::DebugProcess(
           error.Clear();
 
         if (target && error.Success()) {
-          debugger.GetTargetList().SetSelectedTarget(target);
-
           // The darwin always currently uses the GDB remote debugger plug-in
           // so even when debugging locally we are debugging remotely!
           process_sp = target->CreateProcess(launch_info.GetListener(),
@@ -581,8 +579,6 @@ lldb::ProcessSP PlatformRemoteGDBServer::Attach(
           error.Clear();
 
         if (target && error.Success()) {
-          debugger.GetTargetList().SetSelectedTarget(target);
-
           // The darwin always currently uses the GDB remote debugger plug-in
           // so even when debugging locally we are debugging remotely!
           process_sp =
index b5b673a..a77ecdd 100644 (file)
@@ -1831,8 +1831,6 @@ lldb::ProcessSP Platform::DoConnectProcess(llvm::StringRef connect_url,
   if (!target || error.Fail())
     return nullptr;
 
-  debugger.GetTargetList().SetSelectedTarget(target);
-
   lldb::ProcessSP process_sp =
       target->CreateProcess(debugger.GetListener(), plugin_name, nullptr, true);
 
index 2be32c5..bbef3b6 100644 (file)
@@ -48,9 +48,13 @@ Status TargetList::CreateTarget(Debugger &debugger,
                                 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);
+  auto result = TargetList::CreateTargetInternal(
+      debugger, user_exe_path, triple_str, load_dependent_files,
+      platform_options, target_sp);
+
+  if (target_sp && result.Success())
+    AddTargetInternal(target_sp, /*do_select*/ true);
+  return result;
 }
 
 Status TargetList::CreateTarget(Debugger &debugger,
@@ -58,8 +62,13 @@ Status TargetList::CreateTarget(Debugger &debugger,
                                 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);
+  auto result = TargetList::CreateTargetInternal(
+      debugger, user_exe_path, specified_arch, load_dependent_files,
+      platform_sp, target_sp);
+
+  if (target_sp && result.Success())
+    AddTargetInternal(target_sp, /*do_select*/ true);
+  return result;
 }
 
 Status TargetList::CreateTargetInternal(
@@ -388,9 +397,6 @@ Status TargetList::CreateTargetInternal(Debugger &debugger,
     target_sp->AppendExecutableSearchPaths(file_dir);
   }
 
-  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());
 
@@ -552,18 +558,29 @@ uint32_t TargetList::GetIndexOfTarget(lldb::TargetSP target_sp) const {
   return UINT32_MAX;
 }
 
-uint32_t TargetList::SetSelectedTarget(Target *target) {
+void TargetList::AddTargetInternal(TargetSP target_sp, bool do_select) {
+  lldbassert(std::find(m_target_list.begin(), m_target_list.end(), target_sp) ==
+                 m_target_list.end() &&
+             "target already exists it the list");
+  m_target_list.push_back(std::move(target_sp));
+  if (do_select)
+    SetSelectedTargetInternal(m_target_list.size() - 1);
+}
+
+void TargetList::SetSelectedTargetInternal(uint32_t index) {
+  lldbassert(!m_target_list.empty());
+  m_selected_target_idx = index < m_target_list.size() ? index : 0;
+}
+
+void TargetList::SetSelectedTarget(uint32_t index) {
   std::lock_guard<std::recursive_mutex> guard(m_target_list_mutex);
-  collection::const_iterator pos, begin = m_target_list.begin(),
-                                  end = m_target_list.end();
-  for (pos = begin; pos != end; ++pos) {
-    if (pos->get() == target) {
-      m_selected_target_idx = std::distance(begin, pos);
-      return m_selected_target_idx;
-    }
-  }
-  m_selected_target_idx = 0;
-  return m_selected_target_idx;
+  SetSelectedTargetInternal(index);
+}
+
+void TargetList::SetSelectedTarget(const TargetSP &target_sp) {
+  std::lock_guard<std::recursive_mutex> guard(m_target_list_mutex);
+  auto it = std::find(m_target_list.begin(), m_target_list.end(), target_sp);
+  SetSelectedTargetInternal(std::distance(m_target_list.begin(), it));
 }
 
 lldb::TargetSP TargetList::GetSelectedTarget() {
index 1cef818..713fadc 100644 (file)
@@ -123,8 +123,6 @@ TraceSessionFileParser::ParseProcess(const JSONProcess &process) {
   ParsedProcess parsed_process;
   parsed_process.target_sp = target_sp;
 
-  m_debugger.GetTargetList().SetSelectedTarget(target_sp.get());
-
   ProcessSP process_sp = target_sp->CreateProcess(
       /*listener*/ nullptr, "trace",
       /*crash_file*/ nullptr,
index f873e35..a21ea7b 100644 (file)
@@ -112,16 +112,11 @@ typedef std::shared_ptr<Process::ProcessEventData> ProcessEventDataSP;
 typedef std::shared_ptr<Event> EventSP;
 
 TargetSP CreateTarget(DebuggerSP &debugger_sp, ArchSpec &arch) {
-  Status error;
   PlatformSP platform_sp;
   TargetSP target_sp;
-  error = debugger_sp->GetTargetList().CreateTarget(
+  debugger_sp->GetTargetList().CreateTarget(
       *debugger_sp, "", arch, eLoadDependentsNo, platform_sp, target_sp);
 
-  if (target_sp) {
-    debugger_sp->GetTargetList().SetSelectedTarget(target_sp.get());
-  }
-
   return target_sp;
 }
 
index 9a79bbc..6b00072 100644 (file)
@@ -83,16 +83,11 @@ public:
 } // namespace
 
 TargetSP CreateTarget(DebuggerSP &debugger_sp, ArchSpec &arch) {
-  Status error;
   PlatformSP platform_sp;
   TargetSP target_sp;
-  error = debugger_sp->GetTargetList().CreateTarget(
+  debugger_sp->GetTargetList().CreateTarget(
       *debugger_sp, "", arch, eLoadDependentsNo, platform_sp, target_sp);
 
-  if (target_sp) {
-    debugger_sp->GetTargetList().SetSelectedTarget(target_sp.get());
-  }
-
   return target_sp;
 }