[lldb/Target] Add ability to set a label to targets

[mib]

Summary

This patch add the ability for the user to set a label for a target.

This can be very useful when debugging targets with the same executables
in the same session.

Labels can be set either at the target creation in the command
interpreter or at any time using the SBAPI.

Target labels show up in the target list output, following the target
index, and they also allow the user to switch targets using them.

rdar://105016191

Signed-off-by: Med Ismail Bennani <[e-mail]>

[mib]

I still need to add some tests.

[bulbazord]

lldb/source/Commands/CommandObjectTarget.cpp

315

nit:

523–524

Maybe we can rename target_idx_arg to something like target_identifier? Since it's not necessarily an index anymore.

550–555

nit:

Also, do we need the empty check? Or is it possible that target_idx_arg could be an empty string?

[jingham]

Make sure we do something sensible with "target select <NAME>" if the user has given the same name to two targets (or disallow doing that, one or the other).

[bulbazord]

In D151859#4385986, @jingham wrote:

Make sure we do something sensible with "target select <NAME>" if the user has given the same name to two targets (or disallow doing that, one or the other).

In addition to this, what happens if the user sets the target's name to a number? Something like 10. I don't know what format llvm::to_integer expects, but I wonder if it can extract 0xF as 15?

[jingham]

I'd also maybe call this the Target "Label" not the Name. We have a fairly strong use of Name for breakpoint names, and this doesn't have that character at all. Also, if they are Labels, I think it's legit for us to keep them unique, which I think is more sane than trying to handle two targets with the same label...

[mib]

lldb/source/Commands/CommandObjectTarget.cpp

550–555

We need to keep the empty check otherwise target select "" would select the first non-labeled target which would be wrong.

[mib]

Address @bulbazord & @jingham's comments:

• Rename name -> label
• Add test
• Add sanity check to avoid having integer only labels
• Add target index when label already exist for another target

[bulbazord]

I feel a little strange about having SBTarget::SetLabel give a label to a target that is anything other than what the user specified. In the test, you attempt to name 2 targets cat. The first one succeeds, the second one also succeeds but names it cat 2 (where 2 is the index of the target). I think we should probably give some kind of error instead of doing that.

lldb/source/API/SBTarget.cpp

1614–1620

What if this method returned an SBError that communicated the issue? Specifically, something like target labels cannot be integral values or something?

I see you doing that below in the lldb_private code.

lldb/source/Target/Target.cpp

2546

nit: I know I'm arguing against this implementation, but in case this does go in with this implementation, you can probably just use i instead of getting the index of the target again.

[mib]

Address @bulbazord comments:

• Make Target::SetLabel return an llvm::Error to propagate error messages to both the CommandObject and the SBAPI callers.

[bulbazord]

LGTM!

[haowei]

Hi

The lldb-shell :: Target/target-label.test is broken on our LLVM builder after this change was landed. Error message we saw:

Script:
--
: 'RUN: at line 1';   /b/s/w/ir/x/w/staging/llvm_build/bin/lldb --no-lldbinit -S /b/s/w/ir/x/w/staging/llvm_build/tools/lldb/test/Shell/lit-lldb-init-quiet -b -o 'settings set interpreter.stop-command-source-on-error false' -s /b/s/w/ir/x/w/llvm-llvm-project/lldb/test/Shell/Target/target-label.test 2>&1 | /b/s/w/ir/x/w/staging/llvm_build/bin/FileCheck /b/s/w/ir/x/w/llvm-llvm-project/lldb/test/Shell/Target/target-label.test
--
Exit Code: 1

Command Output (stderr):
--
/b/s/w/ir/x/w/llvm-llvm-project/lldb/test/Shell/Target/target-label.test:9:10: error: CHECK: expected string not found in input
# CHECK: * target #0: /bin/ls
         ^
<stdin>:10:26: note: scanning from here
* target #0 (ls): /bin/ls ( arch=x86_64-*-linux, platform=host )
                         ^
<stdin>:29:2: note: possible intended match here
 target #2: /bin/cat ( arch=x86_64-*-linux, platform=host )
 ^

Input file: <stdin>
Check file: /b/s/w/ir/x/w/llvm-llvm-project/lldb/test/Shell/Target/target-label.test

-dump-input=help explains the following input dump.

Input was:
<<<<<<
           .
           .
           .
           5: (lldb) command source -s 0 '/b/s/w/ir/x/w/llvm-llvm-project/lldb/test/Shell/Target/target-label.test' 
           6: Executing commands in '/b/s/w/ir/x/w/llvm-llvm-project/lldb/test/Shell/Target/target-label.test'. 
           7: (lldb) target create -l "ls" /bin/ls 
           8: (lldb) target list 
           9: Current targets: 
          10: * target #0 (ls): /bin/ls ( arch=x86_64-*-linux, platform=host ) 
check:9'0                              X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
          11: (lldb) script lldb.target.SetLabel("") 
check:9'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          12: error: there is no embedded script interpreter in this mode. 
check:9'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          13: (lldb) target list 
check:9'0     ~~~~~~~~~~~~~~~~~~~
          14: Current targets: 
check:9'0     ~~~~~~~~~~~~~~~~~
          15: * target #0 (ls): /bin/ls ( arch=x86_64-*-linux, platform=host ) 
check:9'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           .
           .
           .
          24: error: Cannot use integer as target label. 
check:9'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          25: (lldb) target select 0 
check:9'0     ~~~~~~~~~~~~~~~~~~~~~~~
          26: Current targets: 
check:9'0     ~~~~~~~~~~~~~~~~~
          27: * target #0 (ls): /bin/ls ( arch=x86_64-*-linux, platform=host ) 
check:9'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          28:  target #1 (cat): /bin/cat ( arch=x86_64-*-linux, platform=host ) 
check:9'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          29:  target #2: /bin/cat ( arch=x86_64-*-linux, platform=host ) 
check:9'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
check:9'1      ?                                                           possible intended match
          30:  target #3: /bin/cat ( arch=x86_64-*-linux, platform=host ) 
check:9'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          31: (lldb) target select cat 
check:9'0     ~~~~~~~~~~~~~~~~~~~~~~~~~
          32: Current targets: 
check:9'0     ~~~~~~~~~~~~~~~~~
          33:  target #0 (ls): /bin/ls ( arch=x86_64-*-linux, platform=host ) 
check:9'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          34: * target #1 (cat): /bin/cat ( arch=x86_64-*-linux, platform=host ) 
check:9'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           .
           .
           .
>>>>>>

--

Link to the builder: https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8779016248689544129/overview

Could you take a look? If it takes some time to fix, could you revert this change please?

[mib]

> Could you take a look? If it takes some time to fix, could you revert this change please?

@haowei bcfd85a25857294cb4a5cfa3f616f57a6911cda6 should fix the test failure. Let me know if there is another on.

lldb/include/lldb/API/SBTarget.h

public:
   lldb::ByteOrder GetByteOrder();
 
   uint32_t GetAddressByteSize();
 
   const char *GetTriple();
   
   const char *GetABIName();
 
+  const char *GetLabel() const;
+
+  SBError SetLabel(const char *label);
+
   /// Architecture data byte width accessor
   ///
   /// \return
   /// The size in 8-bit (host) bytes of a minimum addressable
   /// unit from the Architecture's data bus
   uint32_t GetDataByteSize();
 
   /// Architecture code byte width accessor

lldb/include/lldb/Target/Target.h

public:
   static FileSpecList GetDefaultDebugFileSearchPaths();
 
   static ArchSpec GetDefaultArchitecture();
 
   static void SetDefaultArchitecture(const ArchSpec &arch);
 
   bool IsDummyTarget() const { return m_is_dummy_target; }
 
+  const std::string &GetLabel() const { return m_label; }
+
+  /// Set a label for a target.
+  ///
+  /// The label cannot be used by another target or be only integral.
+  ///
+  /// \return
+  ///     The label for this target or an error if the label didn't match the
+  ///     requirements.
+  llvm::Error SetLabel(llvm::StringRef label);
+
   /// Find a binary on the system and return its Module,
   /// or return an existing Module that is already in the Target.
   ///
   /// Given a ModuleSpec, find a binary satisifying that specification,
   /// or identify a matching Module already present in the Target,
   /// and return a shared pointer to it.
   ///
   /// \param[in] module_spec
std::recursive_mutex m_mutex; ///< An API mutex that is used by the lldb::SB*
                                 /// classes make the SB interface thread safe
   /// When the private state thread calls SB API's - usually because it is
   /// running OS plugin or Python ThreadPlan code - it should not block on the
   /// API mutex that is held by the code that kicked off the sequence of events
   /// that led us to run the code.  We hand out this mutex instead when we
   /// detect that code is running on the private state thread.
   std::recursive_mutex m_private_mutex;
   Arch m_arch;
+  std::string m_label;
   ModuleList m_images; ///< The list of images for this process (shared
                        /// libraries and anything dynamically loaded).
   SectionLoadHistory m_section_load_history;
   BreakpointList m_breakpoint_list;
   BreakpointList m_internal_breakpoint_list;
   using BreakpointNameList =
       std::map<ConstString, std::unique_ptr<BreakpointName>>;
   BreakpointNameList m_breakpoint_names;

lldb/include/lldb/Target/TargetList.h

public:
   ///
   /// \return
   ///     Returns \b true if the target was successfully removed from
   ///     from this target list, \b false otherwise. The client will
   ///     be left with the last remaining shared pointer to the target
   ///     in \a target_sp which can then be properly released.
   bool DeleteTarget(lldb::TargetSP &target_sp);
 
-  int GetNumTargets() const;
+  size_t GetNumTargets() const;
 
   lldb::TargetSP GetTargetAtIndex(uint32_t index) const;
 
   uint32_t GetIndexOfTarget(lldb::TargetSP target_sp) const;
 
   /// Find the target that contains has an executable whose path
   /// matches \a exe_file_spec, and whose architecture matches
   /// \a arch_ptr if arch_ptr is not nullptr.

lldb/source/API/SBTarget.cpp

const char *SBTarget::GetABIName() {
   if (!target_sp)
     return nullptr;
 
   std::string abi_name(target_sp->GetABIName().str());
   ConstString const_name(abi_name.c_str());
   return const_name.GetCString();
 }
 
+const char *SBTarget::GetLabel() const {
+  LLDB_INSTRUMENT_VA(this);
+
+  TargetSP target_sp(GetSP());
+  if (!target_sp)
+    return nullptr;
+
+  return ConstString(target_sp->GetLabel().data()).AsCString();
+}
+
+SBError SBTarget::SetLabel(const char *label) {
+  LLDB_INSTRUMENT_VA(this, label);
+
+  TargetSP target_sp(GetSP());
+  if (!target_sp)
+    return Status("Couldn't get internal target object.");
+
+  return Status(target_sp->SetLabel(label));
+}
+
 uint32_t SBTarget::GetDataByteSize() {
   LLDB_INSTRUMENT_VA(this);
 
   TargetSP target_sp(GetSP());
   if (target_sp) {
     return target_sp->GetArchitecture().GetDataByteSize();
   }
   return 0;

lldb/source/Commands/CommandObjectTarget.cpp

static void DumpTargetInfo(uint32_t target_idx, Target *target,
   char exe_path[PATH_MAX];
   bool exe_valid = false;
   if (exe_module)
     exe_valid = exe_module->GetFileSpec().GetPath(exe_path, sizeof(exe_path));
 
   if (!exe_valid)
     ::strcpy(exe_path, "<none>");
 
-  strm.Printf("%starget #%u: %s", prefix_cstr ? prefix_cstr : "", target_idx,
-              exe_path);
+  std::string formatted_label = "";
+  const std::string &label = target->GetLabel();
+  if (!label.empty()) {
+    formatted_label = " (" + label + ")";
+  }
+
+  strm.Printf("%starget #%u%s: %s", prefix_cstr ? prefix_cstr : "", target_idx,
+              formatted_label.data(), exe_path);
 
   uint32_t properties = 0;
   if (target_arch.IsValid()) {
     strm.Printf("%sarch=", properties++ > 0 ? ", " : " ( ");
     target_arch.DumpTriple(strm.AsRawOstream());
     properties++;
   }
   PlatformSP platform_sp(target->GetPlatform());
public:
   CommandObjectTargetCreate(CommandInterpreter &interpreter)
       : CommandObjectParsed(
             interpreter, "target create",
             "Create a target using the argument as the main executable.",
             nullptr),
         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_label(LLDB_OPT_SET_1, false, "label", 'l', 0, eArgTypeName,
+                "Optional name for this target.", nullptr),
         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(
             LLDB_OPT_SET_1, false, "remote-file", 'r', 0, eArgTypeFilename,
             "Fullpath to the file on the remote host if debugging remotely.") {
CommandObjectTargetCreate(CommandInterpreter &interpreter)
     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_label, 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) {
           m_add_dependents.m_load_dependent_files, &m_platform_options,
           target_sp));
 
       if (!target_sp) {
         result.AppendError(error.AsCString());
         return false;
       }
 
+      const llvm::StringRef label =
+          m_label.GetOptionValue().GetCurrentValueAsRef();
+      if (!label.empty()) {
+        if (auto E = target_sp->SetLabel(label))
+          result.SetError(std::move(E));
+        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.
bool DoExecute(Args &command, CommandReturnObject &result) override {
     return result.Succeeded();
   }
 
 private:
   OptionGroupOptions m_option_group;
   OptionGroupArchitecture m_arch_option;
   OptionGroupPlatform m_platform_options;
   OptionGroupFile m_core_file;
+  OptionGroupString m_label;
   OptionGroupFile m_symbol_file;
   OptionGroupFile m_remote_file;
   OptionGroupDependents m_add_dependents;
 };
 
 #pragma mark CommandObjectTargetList
 
 class CommandObjectTargetList : public CommandObjectParsed {
CommandObjectTargetSelect(CommandInterpreter &interpreter)
     CommandArgumentData target_arg{eArgTypeTargetID, eArgRepeatPlain};
     m_arguments.push_back({target_arg});
   }
 
   ~CommandObjectTargetSelect() override = default;
 
 protected:
   bool DoExecute(Args &args, CommandReturnObject &result) override {
     if (args.GetArgumentCount() == 1) {
-      const char *target_idx_arg = args.GetArgumentAtIndex(0);
-      uint32_t target_idx;
-      if (llvm::to_integer(target_idx_arg, target_idx)) {
-        TargetList &target_list = GetDebugger().GetTargetList();
-        const uint32_t num_targets = target_list.GetNumTargets();
+      const char *target_identifier = args.GetArgumentAtIndex(0);
+      uint32_t target_idx = LLDB_INVALID_INDEX32;
+      if (llvm::to_integer(target_identifier, target_idx)) {
         if (target_idx < num_targets) {
           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 %u is out of range, valid target indexes are 0 - %u\n",
                 target_idx, num_targets - 1);
           } else {
             result.AppendErrorWithFormat(
                 "index %u is out of range since there are no active targets\n",
                 target_idx);
           }
         }
       } else {
+        for (size_t i = 0; i < num_targets; i++) {
+          if (TargetSP target_sp = target_list.GetTargetAtIndex(i)) {
+            const std::string &label = target_sp->GetLabel();
+            if (!label.empty() && label == target_identifier) {
+              target_idx = i;
+              break;
+            }
+          }
+        }
+
+        if (target_idx != LLDB_INVALID_INDEX32) {
+          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 {
           result.AppendErrorWithFormat("invalid index string value '%s'\n",
-                                     target_idx_arg);
+                                       target_identifier);
+        }
       }
     } else {
       result.AppendError(
           "'target select' takes a single argument: a target index\n");
     }
     return result.Succeeded();
   }
 };

 protected:
   bool DoExecute(Args &args, CommandReturnObject &result) override {
     const size_t argc = args.GetArgumentCount();
     std::vector<TargetSP> delete_target_list;
     TargetList &target_list = GetDebugger().GetTargetList();
     TargetSP target_sp;
 
     if (m_all_option.GetOptionValue()) {
-      for (int i = 0; i < target_list.GetNumTargets(); ++i)
+      for (size_t i = 0; i < target_list.GetNumTargets(); ++i)
         delete_target_list.push_back(target_list.GetTargetAtIndex(i));
     } else if (argc > 0) {
       const uint32_t num_targets = target_list.GetNumTargets();
       // Bail out if don't have any targets.
       if (num_targets == 0) {
         result.AppendError("no targets to delete");
         return false;
       }

lldb/source/Target/Target.cpp

 #include "lldb/Utility/Timer.h"
 
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SetVector.h"
 
 #include <memory>
 #include <mutex>
 #include <optional>
+#include <sstream>
 
 using namespace lldb;
 using namespace lldb_private;
 
 constexpr std::chrono::milliseconds EvaluateExpressionOptions::default_timeout;
 
 Target::Arch::Arch(const ArchSpec &spec)
     : m_spec(spec),

 
 void Target::SetDefaultArchitecture(const ArchSpec &arch) {
   LLDB_LOG(GetLog(LLDBLog::Target),
            "setting target's default architecture to  {0} ({1})",
            arch.GetArchitectureName(), arch.GetTriple().getTriple());
   Target::GetGlobalProperties().SetDefaultArchitecture(arch);
 }
 
+llvm::Error Target::SetLabel(llvm::StringRef label) {
+  size_t n = LLDB_INVALID_INDEX32;
+  if (llvm::to_integer(label, n))
+    return llvm::make_error<llvm::StringError>(
+        "Cannot use integer as target label.", llvm::inconvertibleErrorCode());
+  TargetList &targets = GetDebugger().GetTargetList();
+  for (size_t i = 0; i < targets.GetNumTargets(); i++) {
+    TargetSP target_sp = targets.GetTargetAtIndex(i);
+    if (target_sp && target_sp->GetLabel() == label) {
+        return llvm::make_error<llvm::StringError>(
+            llvm::formatv(
+                "Cannot use label '{0}' since it's set in target #{1}.", label,
+                i),
+            llvm::inconvertibleErrorCode());
+    }
+  }
+
+  m_label = label.str();
+  return llvm::Error::success();
+}
+
 Target *Target::GetTargetFromContexts(const ExecutionContext *exe_ctx_ptr,
                                       const SymbolContext *sc_ptr) {
   // The target can either exist in the "process" of ExecutionContext, or in
   // the "target_sp" member of SymbolContext. This accessor helper function
   // will get the target from one of these locations.
 
   Target *target = nullptr;
   if (sc_ptr != nullptr)

lldb/source/Target/TargetList.cpp

if (target_sp) {
         ++num_signals_sent;
         process->Signal(signo);
       }
     }
   }
   return num_signals_sent;
 }
 
-int TargetList::GetNumTargets() const {
+size_t TargetList::GetNumTargets() const {
   std::lock_guard<std::recursive_mutex> guard(m_target_list_mutex);
   return m_target_list.size();
 }
 
 lldb::TargetSP TargetList::GetTargetAtIndex(uint32_t idx) const {
   TargetSP target_sp;
   std::lock_guard<std::recursive_mutex> guard(m_target_list_mutex);
   if (idx < m_target_list.size())

lldb/test/Shell/Target/target-label.test

+# RUN: %lldb -b -o 'settings set interpreter.stop-command-source-on-error false' -s %s 2>&1 | FileCheck %s
+
+target create -l "ls" /bin/ls
+target list
+# CHECK: * target #0 (ls): /bin/ls
+
+script lldb.target.SetLabel("")
+target list
+# CHECK: * target #0: /bin/ls
+
+target create -l "cat" /bin/cat
+target list
+# CHECK: target #0: /bin/ls
+# CHECK-NEXT: * target #1 (cat): /bin/cat
+
+target create -l "cat" /bin/cat
+# CHECK: Cannot use label 'cat' since it's set in target #1.
+
+target create -l 42 /bin/cat
+# CHECK: error: Cannot use integer as target label.
+
+target select 0
+# CHECK: * target #0: /bin/ls
+# CHECK-NEXT: target #1 (cat): /bin/cat
+
+target select cat
+# CHECK: target #0: /bin/ls
+# CHECK-NEXT: * target #1 (cat): /bin/cat
+
+script lldb.target.GetLabel()
+# CHECK: 'cat'
+
+script lldb.debugger.GetTargetAtIndex(0).SetLabel('Not cat')
+# CHECK: success
+
+target list
+# CHECK: target #0 (Not cat): /bin/ls
+# CHECK-NEXT: * target #1 (cat): /bin/cat