[lldb-dap] show function name in the instruction comment.

[da-viper]

putting the function name is the dissassembly instruction messes up the alignment making it less readable. put it instead with the comment.

This also aligns the opcodes and instruction to the left matching the cli

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Ebuka Ezike (da-viper)

Changes

putting the function name is the dissassembly instruction messes up the alignment making it less readable. put it instead with the comment.

This also aligns the opcodes and instruction to the left matching the cli

---

Full diff: https://github.com/llvm/llvm-project/pull/144070.diff

1 Files Affected:

• (modified) lldb/tools/lldb-dap/Handler/DisassembleRequestHandler.cpp (+15-19)

diff --git a/lldb/tools/lldb-dap/Handler/DisassembleRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/DisassembleRequestHandler.cpp
index d5878d18289d6..85214b84b5c9c 100644
--- a/lldb/tools/lldb-dap/Handler/DisassembleRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/DisassembleRequestHandler.cpp
@@ -100,7 +100,7 @@ static DisassembledInstruction ConvertSBInstructionToDisassembledInstruction(
 
   const char *m = inst.GetMnemonic(target);
   const char *o = inst.GetOperands(target);
-  const char *c = inst.GetComment(target);
+  std::string c = inst.GetComment(target);
   auto d = inst.GetData(target);
 
   std::string bytes;
@@ -114,34 +114,30 @@ static DisassembledInstruction ConvertSBInstructionToDisassembledInstruction(
 
   DisassembledInstruction disassembled_inst;
   disassembled_inst.address = inst_addr;
-  disassembled_inst.instructionBytes =
-      bytes.size() > 0 ? bytes.substr(0, bytes.size() - 1) : "";
 
-  std::string instruction;
-  llvm::raw_string_ostream si(instruction);
+  if (!bytes.empty()) // remove last whitespace
+    bytes.pop_back();
+  disassembled_inst.instructionBytes = std::move(bytes);
+
+  llvm::raw_string_ostream si(disassembled_inst.instruction);
+  si << llvm::formatv("{0,-7} {1,-25}", m, o);
 
-  lldb::SBSymbol symbol = addr.GetSymbol();
   // Only add the symbol on the first line of the function.
-  if (symbol.IsValid() && symbol.GetStartAddress() == addr) {
-    // If we have a valid symbol, append it as a label prefix for the first
-    // instruction. This is so you can see the start of a function/callsite
-    // in the assembly, at the moment VS Code (1.80) does not visualize the
-    // symbol associated with the assembly instruction.
-    si << (symbol.GetMangledName() != nullptr ? symbol.GetMangledName()
-                                              : symbol.GetName())
-       << ": ";
+  // in the comment section
+  if (lldb::SBSymbol symbol = addr.GetSymbol();
+      symbol.GetStartAddress() == addr) {
+    const llvm::StringRef sym_display_name = symbol.GetDisplayName();
+    c.append(" ");
+    c.append(sym_display_name);
 
     if (resolve_symbols)
-      disassembled_inst.symbol = symbol.GetDisplayName();
+      disassembled_inst.symbol = sym_display_name;
   }
 
-  si << llvm::formatv("{0,7} {1,12}", m, o);
-  if (c && c[0]) {
+  if (!c.empty()) {
     si << " ; " << c;
   }
 
-  disassembled_inst.instruction = std::move(instruction);
-
   protocol::Source source = CreateSource(addr, target);
   lldb::SBLineEntry line_entry = GetLineEntryForAddress(target, addr);
 

[ashgti]

Looking at this locally, I think this is changing:

0x01234   ff ff ff ff   NSLog: pacibsp
...

into:

0x01234   ff ff ff ff   pacibsp ; NSLog
...

When other functions are called, they have the name in the comment, so you'd see

0x01234   ff ff ff ff   bl 0x04321 ; _NSLogv

as an example in arm64 where we're calling another function.

The name being in the comment, at least for me, makes it hard for me to tell where the function begins vs other functions that are being called.

I wonder if we should have some addition context? Like ; start of symbol <name> or something.

[labath]

Including the target name in the comment (without any extra explanation) is a pretty standard thing to do for disassemblers. For example, lldb's dissassemble command will give you:

    0x5555556e8cb1 <+2125>: callq  0x5555556ecf8e ; llvm::opt::InputArgList::~InputArgList at ArgList.h:442:3

and objdump produces things like:

  194a18:       48 8d 05 63 06 e9 ff    lea    -0x16f99d(%rip),%rax        # 25082 <(anonymous namespace)::OptionPrefixesTable+0x15e2>

.. so I don't think we need to make this output longer (the demangled name is long enough as it is)

[ashgti]

Ah, ok, if is already like this in other places, SGTM