[lldb-dap] Show load addresses in disassembly #136755

eronnen · 2025-04-22T20:02:22Z

Improves the lldb-dap disassembly by showing load addresses in disassembly, same as in a regular LLDB disassemble command by default.

Before:

After:

llvmbot · 2025-04-22T20:02:54Z

@llvm/pr-subscribers-lldb

Author: Ely Ronnen (eronnen)

Changes

Improves the lldb-dap disassembly by showing load addresses in disassembly, same as in a regular LLDB disassemble command by default.

Before:

After:

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

4 Files Affected:

(modified) lldb/include/lldb/API/SBFrame.h (+1)
(modified) lldb/include/lldb/API/SBInstructionList.h (+5-2)
(modified) lldb/source/API/SBInstructionList.cpp (+21-8)
(modified) lldb/tools/lldb-dap/Handler/SourceRequestHandler.cpp (+1-1)

diff --git a/lldb/include/lldb/API/SBFrame.h b/lldb/include/lldb/API/SBFrame.h
index 3635ee5a537ad..66db059d531a8 100644
--- a/lldb/include/lldb/API/SBFrame.h
+++ b/lldb/include/lldb/API/SBFrame.h
@@ -221,6 +221,7 @@ class LLDB_API SBFrame {
   friend class SBBlock;
   friend class SBExecutionContext;
   friend class SBInstruction;
+  friend class SBInstructionList;
   friend class SBThread;
   friend class SBValue;
 
diff --git a/lldb/include/lldb/API/SBInstructionList.h b/lldb/include/lldb/API/SBInstructionList.h
index 4c26ec9a294e0..b337c91721653 100644
--- a/lldb/include/lldb/API/SBInstructionList.h
+++ b/lldb/include/lldb/API/SBInstructionList.h
@@ -54,6 +54,10 @@ class LLDB_API SBInstructionList {
 
   bool GetDescription(lldb::SBStream &description);
 
+  // Writes assembly instructions to `description` with load addresses using
+  // `frame`.
+  bool GetDescription(lldb::SBStream &description, lldb::SBFrame &frame);
+
   bool DumpEmulationForAllInstructions(const char *triple);
 
 protected:
@@ -62,8 +66,7 @@ class LLDB_API SBInstructionList {
   friend class SBTarget;
 
   void SetDisassembler(const lldb::DisassemblerSP &opaque_sp);
-  bool GetDescription(lldb_private::Stream &description);
-
+  bool GetDescription(lldb_private::Stream &description, lldb::SBFrame *frame);
 
 private:
   lldb::DisassemblerSP m_opaque_sp;
diff --git a/lldb/source/API/SBInstructionList.cpp b/lldb/source/API/SBInstructionList.cpp
index c18204375dff1..79432e8c6c91a 100644
--- a/lldb/source/API/SBInstructionList.cpp
+++ b/lldb/source/API/SBInstructionList.cpp
@@ -15,8 +15,10 @@
 #include "lldb/Core/Module.h"
 #include "lldb/Host/StreamFile.h"
 #include "lldb/Symbol/SymbolContext.h"
+#include "lldb/Target/ExecutionContext.h"
 #include "lldb/Utility/Instrumentation.h"
 #include "lldb/Utility/Stream.h"
+#include <memory>
 
 using namespace lldb;
 using namespace lldb_private;
@@ -114,7 +116,7 @@ void SBInstructionList::Print(FILE *out) {
   if (out == nullptr)
     return;
   StreamFile stream(out, false);
-  GetDescription(stream);
+  GetDescription(stream, nullptr);
 }
 
 void SBInstructionList::Print(SBFile out) {
@@ -122,7 +124,7 @@ void SBInstructionList::Print(SBFile out) {
   if (!out.IsValid())
     return;
   StreamFile stream(out.m_opaque_sp);
-  GetDescription(stream);
+  GetDescription(stream, nullptr);
 }
 
 void SBInstructionList::Print(FileSP out_sp) {
@@ -130,15 +132,21 @@ void SBInstructionList::Print(FileSP out_sp) {
   if (!out_sp || !out_sp->IsValid())
     return;
   StreamFile stream(out_sp);
-  GetDescription(stream);
+  GetDescription(stream, nullptr);
 }
 
 bool SBInstructionList::GetDescription(lldb::SBStream &stream) {
   LLDB_INSTRUMENT_VA(this, stream);
-  return GetDescription(stream.ref());
+  return GetDescription(stream.ref(), nullptr);
 }
 
-bool SBInstructionList::GetDescription(Stream &sref) {
+bool SBInstructionList::GetDescription(lldb::SBStream &stream,
+                                       lldb::SBFrame &frame) {
+  LLDB_INSTRUMENT_VA(this, stream);
+  return GetDescription(stream.ref(), &frame);
+}
+
+bool SBInstructionList::GetDescription(Stream &sref, lldb::SBFrame *frame) {
 
   if (m_opaque_sp) {
     size_t num_instructions = GetSize();
@@ -148,10 +156,15 @@ bool SBInstructionList::GetDescription(Stream &sref) {
       const uint32_t max_opcode_byte_size =
           m_opaque_sp->GetInstructionList().GetMaxOpcocdeByteSize();
       FormatEntity::Entry format;
-      FormatEntity::Parse("${addr}: ", format);
+      FormatEntity::Parse("${addr-file-or-load}: ", format);
       SymbolContext sc;
       SymbolContext prev_sc;
 
+      std::shared_ptr<ExecutionContext> exec_ctx;
+      if (nullptr != frame) {
+        exec_ctx = std::make_shared<ExecutionContext>(frame->GetFrameSP());
+      }
+
       // Expected address of the next instruction. Used to print an empty line
       // for non-contiguous blocks of insns.
       std::optional<Address> next_addr;
@@ -172,8 +185,8 @@ bool SBInstructionList::GetDescription(Stream &sref) {
         if (next_addr && *next_addr != addr)
           sref.EOL();
         inst->Dump(&sref, max_opcode_byte_size, true, false,
-                   /*show_control_flow_kind=*/false, nullptr, &sc, &prev_sc,
-                   &format, 0);
+                   /*show_control_flow_kind=*/false, exec_ctx.get(), &sc,
+                   &prev_sc, &format, 0);
         sref.EOL();
         next_addr = addr;
         next_addr->Slide(inst->GetOpcode().GetByteSize());
diff --git a/lldb/tools/lldb-dap/Handler/SourceRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/SourceRequestHandler.cpp
index 1a7a13d9f267a..bd4fc981475f4 100644
--- a/lldb/tools/lldb-dap/Handler/SourceRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/SourceRequestHandler.cpp
@@ -43,7 +43,7 @@ SourceRequestHandler::Run(const protocol::SourceArguments &args) const {
 
   lldb::SBInstructionList insts = frame.GetSymbol().GetInstructions(dap.target);
   lldb::SBStream stream;
-  insts.GetDescription(stream);
+  insts.GetDescription(stream, frame);
 
   return protocol::SourceResponseBody{/*content=*/stream.GetData(),
                                       /*mimeType=*/"text/x-lldb.disassembly"};

JDevlieghere · 2025-04-22T22:35:09Z

lldb/include/lldb/API/SBInstructionList.h

@@ -54,6 +54,10 @@ class LLDB_API SBInstructionList {

  bool GetDescription(lldb::SBStream &description);

+  // Writes assembly instructions to `description` with load addresses using
+  // `frame`.
+  bool GetDescription(lldb::SBStream &description, lldb::SBFrame &frame);


We'd want this API to be as generic as possible. Since you only need the execution context, that means this should take an SBExecutionContext, which you can create from a frame or thread or process.

JDevlieghere · 2025-04-22T22:35:57Z

lldb/include/lldb/API/SBInstructionList.h

@@ -62,8 +66,7 @@ class LLDB_API SBInstructionList {
  friend class SBTarget;

  void SetDisassembler(const lldb::DisassemblerSP &opaque_sp);
-  bool GetDescription(lldb_private::Stream &description);
-
+  bool GetDescription(lldb_private::Stream &description, lldb::SBFrame *frame);


Let's give this a default argument so you don't need to pass nullptr everywhere.

Suggested change

bool GetDescription(lldb_private::Stream &description, lldb::SBFrame *frame);

bool GetDescription(lldb_private::Stream &description, lldb::SBExecutionContext *exe_ctx = nullptr);

eronnen · 2025-04-22T23:27:35Z

@JDevlieghere Thanks for the review! updated the API to receive SBExecutionContext

JDevlieghere · 2025-04-22T23:29:51Z

lldb/source/API/SBInstructionList.cpp

+}
+
+bool SBInstructionList::GetDescription(Stream &sref,
+                                       lldb::SBExecutionContext *exe_ctx) {


This could take the private type (e.g. ExecutionContext*).

JDevlieghere · 2025-04-22T23:31:49Z

lldb/include/lldb/API/SBInstructionList.h

-  bool GetDescription(lldb_private::Stream &description);
-
+  bool GetDescription(lldb_private::Stream &description,
+                      lldb::SBExecutionContext *exe_ctx = nullptr);


I guess this can now take the private type (i.e. ExecutionContext*). All the SB API objects are PIMPL objects so it feels a bit weird to take their address.

JDevlieghere · 2025-04-22T23:35:03Z

lldb/source/API/SBInstructionList.cpp

+      std::shared_ptr<ExecutionContext> exe_ctx_ptr;
+      if (nullptr != exe_ctx) {
+        exe_ctx_ptr = std::make_shared<ExecutionContext>(exe_ctx->get());
+      }


A few nits:

No braces around single-line ifs.

Do we actually need to wrap this in a shared pointer? The underlying object will already live long enough, right? Anyway the current code doesn't look safe unless the underlying object inherits from enable_shared_from_this.

eronnen · 2025-04-22T23:43:53Z

@JDevlieghere Nice catch, now that the internal GetDescription gets the private type there is no need for the shared_ptr even

JDevlieghere

LGTM!

eronnen · 2025-04-23T20:02:08Z

@JDevlieghere I don't have write access, fee free to merge this PR if possible (I don't know how the procedures of LLVM work in order to merge)

Improves the lldb-dap disassembly by showing load addresses in disassembly, same as in a regular LLDB `disassemble` command by default. Before: ![Screenshot From 2025-04-22 21-33-56](https://github.com/user-attachments/assets/c3febd48-8335-4932-a270-5a87f48122fe) After: ![Screenshot From 2025-04-22 21-54-51](https://github.com/user-attachments/assets/b2f44595-8ab2-4f28-aded-9233c53a589b)

[lldb-dap]: show load addresses in disassembly

c1c0728

eronnen requested a review from JDevlieghere as a code owner April 22, 2025 20:02

llvmbot added lldb lldb-dap labels Apr 22, 2025

JDevlieghere reviewed Apr 22, 2025

View reviewed changes

using SBExecutionContext parameter instead of SBFrame

cb03788

eronnen requested a review from JDevlieghere April 22, 2025 23:27

JDevlieghere reviewed Apr 22, 2025

View reviewed changes

cleaner SBInstructionList::GetDescription

70d9ce3

eronnen requested a review from JDevlieghere April 22, 2025 23:45

JDevlieghere approved these changes Apr 23, 2025

View reviewed changes

JDevlieghere merged commit 563ab56 into llvm:main Apr 23, 2025
10 checks passed

eronnen mentioned this pull request Apr 25, 2025

Request Commit Access For eronnen #137382

Closed

	bool GetDescription(lldb_private::Stream &description, lldb::SBFrame *frame);
	bool GetDescription(lldb_private::Stream &description, lldb::SBExecutionContext *exe_ctx = nullptr);

[lldb-dap] Show load addresses in disassembly #136755

[lldb-dap] Show load addresses in disassembly #136755

Uh oh!

Conversation

eronnen commented Apr 22, 2025

Uh oh!

llvmbot commented Apr 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eronnen commented Apr 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eronnen commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JDevlieghere left a comment

Choose a reason for hiding this comment

Uh oh!

eronnen commented Apr 23, 2025

Uh oh!

Uh oh!

Uh oh!

eronnen commented Apr 22, 2025 •

edited

Loading