Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

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

Authored by mib on May 31 2023, 5:10 PM.

Details

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 <ismail@bennani.ma>

Diff Detail

Event Timeline

mib created this revision.May 31 2023, 5:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2023, 5:10 PM
mib requested review of this revision.May 31 2023, 5:10 PM
mib added a comment.May 31 2023, 5:11 PM

I still need to add some tests.

bulbazord added inline comments.May 31 2023, 5:21 PM
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?

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).

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?

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 marked 3 inline comments as done.Jun 1 2023, 12:53 PM
mib added inline comments.
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 updated this revision to Diff 527580.Jun 1 2023, 1:03 PM
mib marked an inline comment as done.
mib retitled this revision from [lldb/Target] Add ability to set name to targets to [lldb/Target] Add ability to set a label to targets.
mib edited the summary of this revision. (Show Details)

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

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 updated this revision to Diff 527776.Jun 2 2023, 1:41 AM

Address @bulbazord comments:

  • Make Target::SetLabel return an llvm::Error to propagate error messages to both the CommandObject and the SBAPI callers.
This revision is now accepted and ready to land.Jun 2 2023, 10:45 AM
This revision was automatically updated to reflect the committed changes.
mib marked 2 inline comments as done.
haowei added a subscriber: haowei.Jun 6 2023, 4:32 PM

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 added a comment.Jun 6 2023, 5:43 PM

> 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.