[lldb-dap] Reuse source object logics #141426
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: Ely Ronnen (eronnen) ChangesRefactor code revolving source objects such that most logics will be reused. The main change is to expose a single Other functions can use Patch is 22.05 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/141426.diff 12 Files Affected:
diff --git a/lldb/include/lldb/API/SBAddress.h b/lldb/include/lldb/API/SBAddress.h
index 430dad4862dbf..c61f02702f1ca 100644
--- a/lldb/include/lldb/API/SBAddress.h
+++ b/lldb/include/lldb/API/SBAddress.h
@@ -11,6 +11,7 @@
#include "lldb/API/SBDefines.h"
#include "lldb/API/SBModule.h"
+#include "lldb/API/SBTarget.h"
namespace lldb {
@@ -58,6 +59,9 @@ class LLDB_API SBAddress {
// "lldb::SBAddress SBTarget::ResolveLoadAddress (...)".
lldb::SBSymbolContext GetSymbolContext(uint32_t resolve_scope);
+ lldb::SBSymbolContext GetSymbolContext(const SBTarget &target,
+ uint32_t resolve_scope);
+
// The following functions grab individual objects for a given address and
// are less efficient if you want more than one symbol related objects. Use
// one of the following when you want multiple debug symbol related objects
@@ -122,6 +126,9 @@ class LLDB_API SBAddress {
void SetAddress(const lldb_private::Address &address);
+ void CalculateSymbolContext(lldb_private::SymbolContext &sc,
+ uint32_t resolve_scope);
+
private:
std::unique_ptr<lldb_private::Address> m_opaque_up;
};
diff --git a/lldb/source/API/SBAddress.cpp b/lldb/source/API/SBAddress.cpp
index e519f0bcc83c6..b6b9e238d3cbf 100644
--- a/lldb/source/API/SBAddress.cpp
+++ b/lldb/source/API/SBAddress.cpp
@@ -213,9 +213,18 @@ SBSymbolContext SBAddress::GetSymbolContext(uint32_t resolve_scope) {
LLDB_INSTRUMENT_VA(this, resolve_scope);
SBSymbolContext sb_sc;
- SymbolContextItem scope = static_cast<SymbolContextItem>(resolve_scope);
- if (m_opaque_up->IsValid())
- m_opaque_up->CalculateSymbolContext(&sb_sc.ref(), scope);
+ CalculateSymbolContext(sb_sc.ref(), resolve_scope);
+ return sb_sc;
+}
+
+SBSymbolContext SBAddress::GetSymbolContext(const SBTarget &target,
+ uint32_t resolve_scope) {
+ LLDB_INSTRUMENT_VA(this, target, resolve_scope);
+
+ SBSymbolContext sb_sc;
+ lldb_private::SymbolContext &sc = sb_sc.ref();
+ sc.target_sp = target.GetSP();
+ CalculateSymbolContext(sc, resolve_scope);
return sb_sc;
}
@@ -266,3 +275,10 @@ SBLineEntry SBAddress::GetLineEntry() {
}
return sb_line_entry;
}
+
+void SBAddress::CalculateSymbolContext(lldb_private::SymbolContext &sc,
+ uint32_t resolve_scope) {
+ SymbolContextItem scope = static_cast<SymbolContextItem>(resolve_scope);
+ if (m_opaque_up->IsValid())
+ m_opaque_up->CalculateSymbolContext(&sc, scope);
+}
diff --git a/lldb/source/Core/Module.cpp b/lldb/source/Core/Module.cpp
index 625c14e4a2153..90997dada3666 100644
--- a/lldb/source/Core/Module.cpp
+++ b/lldb/source/Core/Module.cpp
@@ -483,6 +483,9 @@ uint32_t Module::ResolveSymbolContextForAddress(
symfile->SetLoadDebugInfoEnabled();
resolved_flags |=
symfile->ResolveSymbolContext(so_addr, resolve_scope, sc);
+
+ if ((resolve_scope & eSymbolContextLineEntry) && sc.line_entry.IsValid())
+ sc.line_entry.ApplyFileMappings(sc.target_sp);
}
// Resolve the symbol if requested, but don't re-look it up if we've
diff --git a/lldb/tools/lldb-dap/Breakpoint.cpp b/lldb/tools/lldb-dap/Breakpoint.cpp
index 2d0fd9c9c3954..dea4dbadc0bda 100644
--- a/lldb/tools/lldb-dap/Breakpoint.cpp
+++ b/lldb/tools/lldb-dap/Breakpoint.cpp
@@ -9,12 +9,10 @@
#include "Breakpoint.h"
#include "DAP.h"
#include "JSONUtils.h"
-#include "LLDBUtils.h"
#include "lldb/API/SBAddress.h"
#include "lldb/API/SBBreakpointLocation.h"
#include "lldb/API/SBLineEntry.h"
#include "lldb/API/SBMutex.h"
-#include "lldb/lldb-enumerations.h"
#include "llvm/ADT/StringExtras.h"
#include <cstddef>
#include <cstdint>
@@ -66,17 +64,15 @@ protocol::Breakpoint Breakpoint::ToProtocolBreakpoint() {
"0x" + llvm::utohexstr(bp_addr.GetLoadAddress(m_bp.GetTarget()));
breakpoint.instructionReference = formatted_addr;
- lldb::StopDisassemblyType stop_disassembly_display =
- GetStopDisassemblyDisplay(m_dap.debugger);
- auto line_entry = bp_addr.GetLineEntry();
- if (!ShouldDisplayAssemblySource(line_entry, stop_disassembly_display)) {
+ auto source = CreateSource(bp_addr, m_dap.target);
+ if (!source.IsAssemblySource()) {
+ auto line_entry = bp_addr.GetLineEntry();
const auto line = line_entry.GetLine();
if (line != LLDB_INVALID_LINE_NUMBER)
breakpoint.line = line;
const auto column = line_entry.GetColumn();
if (column != LLDB_INVALID_COLUMN_NUMBER)
breakpoint.column = column;
- breakpoint.source = CreateSource(line_entry);
} else {
// Assembly breakpoint.
auto symbol = bp_addr.GetSymbol();
@@ -86,10 +82,10 @@ protocol::Breakpoint Breakpoint::ToProtocolBreakpoint() {
.ReadInstructions(symbol.GetStartAddress(), bp_addr, nullptr)
.GetSize() +
1;
-
- breakpoint.source = CreateAssemblySource(m_dap.target, bp_addr);
}
}
+
+ breakpoint.source = std::move(source);
}
return breakpoint;
diff --git a/lldb/tools/lldb-dap/Handler/DisassembleRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/DisassembleRequestHandler.cpp
index c9061ef19f17a..4c04612bf173a 100644
--- a/lldb/tools/lldb-dap/Handler/DisassembleRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/DisassembleRequestHandler.cpp
@@ -9,6 +9,7 @@
#include "DAP.h"
#include "EventHelper.h"
#include "JSONUtils.h"
+#include "LLDBUtils.h"
#include "Protocol/ProtocolRequests.h"
#include "Protocol/ProtocolTypes.h"
#include "RequestHandler.h"
@@ -138,15 +139,16 @@ static DisassembledInstruction ConvertSBInstructionToDisassembledInstruction(
disassembled_inst.instruction = std::move(instruction);
- auto line_entry = addr.GetLineEntry();
+ auto source = CreateSource(addr, target);
+ auto line_entry = GetLineEntryForAddress(target, addr);
+
// If the line number is 0 then the entry represents a compiler generated
// location.
-
- if (line_entry.GetStartAddress() == addr && line_entry.IsValid() &&
+ if (!source.IsAssemblySource() && line_entry.IsValid() &&
+ line_entry.GetStartAddress() == addr && line_entry.IsValid() &&
line_entry.GetFileSpec().IsValid() && line_entry.GetLine() != 0) {
- auto source = CreateSource(line_entry);
- disassembled_inst.location = std::move(source);
+ disassembled_inst.location = std::move(source);
const auto line = line_entry.GetLine();
if (line != 0 && line != LLDB_INVALID_LINE_NUMBER)
disassembled_inst.line = line;
@@ -155,7 +157,8 @@ static DisassembledInstruction ConvertSBInstructionToDisassembledInstruction(
if (column != 0 && column != LLDB_INVALID_COLUMN_NUMBER)
disassembled_inst.column = column;
- auto end_line_entry = line_entry.GetEndAddress().GetLineEntry();
+ lldb::SBAddress end_addr = line_entry.GetEndAddress();
+ auto end_line_entry = GetLineEntryForAddress(target, end_addr);
if (end_line_entry.IsValid() &&
end_line_entry.GetFileSpec() == line_entry.GetFileSpec()) {
const auto end_line = end_line_entry.GetLine();
diff --git a/lldb/tools/lldb-dap/Handler/LocationsRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/LocationsRequestHandler.cpp
index 15109d4b8e589..39ad6949990c7 100644
--- a/lldb/tools/lldb-dap/Handler/LocationsRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/LocationsRequestHandler.cpp
@@ -9,8 +9,11 @@
#include "DAP.h"
#include "EventHelper.h"
#include "JSONUtils.h"
+#include "LLDBUtils.h"
#include "RequestHandler.h"
+#include "lldb/API/SBAddress.h"
#include "lldb/API/SBDeclaration.h"
+#include "lldb/API/SBLineEntry.h"
namespace lldb_dap {
@@ -122,9 +125,9 @@ void LocationsRequestHandler::operator()(
return;
}
- lldb::addr_t addr = variable.GetValueAsAddress();
- lldb::SBLineEntry line_entry =
- dap.target.ResolveLoadAddress(addr).GetLineEntry();
+ lldb::addr_t raw_addr = variable.GetValueAsAddress();
+ lldb::SBAddress addr = dap.target.ResolveLoadAddress(raw_addr);
+ lldb::SBLineEntry line_entry = GetLineEntryForAddress(dap.target, addr);
if (!line_entry.IsValid()) {
response["success"] = false;
diff --git a/lldb/tools/lldb-dap/Handler/StackTraceRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/StackTraceRequestHandler.cpp
index ff326a47d9a48..4ea4cd1e517d4 100644
--- a/lldb/tools/lldb-dap/Handler/StackTraceRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/StackTraceRequestHandler.cpp
@@ -9,9 +9,7 @@
#include "DAP.h"
#include "EventHelper.h"
#include "JSONUtils.h"
-#include "LLDBUtils.h"
#include "RequestHandler.h"
-#include "lldb/lldb-enumerations.h"
namespace lldb_dap {
@@ -55,8 +53,6 @@ static bool FillStackFrames(DAP &dap, lldb::SBThread &thread,
llvm::json::Array &stack_frames, int64_t &offset,
const int64_t start_frame, const int64_t levels,
const bool include_all) {
- lldb::StopDisassemblyType stop_disassembly_display =
- GetStopDisassemblyDisplay(dap.debugger);
bool reached_end_of_stack = false;
for (int64_t i = start_frame;
static_cast<int64_t>(stack_frames.size()) < levels; i++) {
@@ -73,8 +69,7 @@ static bool FillStackFrames(DAP &dap, lldb::SBThread &thread,
break;
}
- stack_frames.emplace_back(
- CreateStackFrame(frame, frame_format, stop_disassembly_display));
+ stack_frames.emplace_back(CreateStackFrame(frame, frame_format));
}
if (include_all && reached_end_of_stack) {
diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp
index c9c6f4554c325..e0c6e19863896 100644
--- a/lldb/tools/lldb-dap/JSONUtils.cpp
+++ b/lldb/tools/lldb-dap/JSONUtils.cpp
@@ -12,6 +12,7 @@
#include "LLDBUtils.h"
#include "lldb/API/SBAddress.h"
#include "lldb/API/SBCompileUnit.h"
+#include "lldb/API/SBDebugger.h"
#include "lldb/API/SBDeclaration.h"
#include "lldb/API/SBEnvironment.h"
#include "lldb/API/SBError.h"
@@ -497,34 +498,33 @@ static std::string GetLoadAddressString(const lldb::addr_t addr) {
return result;
}
-protocol::Source CreateSource(const lldb::SBFileSpec &file) {
- protocol::Source source;
- if (file.IsValid()) {
- const char *name = file.GetFilename();
- if (name)
- source.name = name;
- char path[PATH_MAX] = "";
- if (file.GetPath(path, sizeof(path)) &&
- lldb::SBFileSpec::ResolvePath(path, path, PATH_MAX))
- source.path = path;
- }
- return source;
-}
+static bool ShouldDisplayAssemblySource(
+ lldb::SBAddress address,
+ lldb::StopDisassemblyType stop_disassembly_display) {
+ if (stop_disassembly_display == lldb::eStopDisassemblyTypeNever)
+ return false;
-protocol::Source CreateSource(const lldb::SBLineEntry &line_entry) {
- return CreateSource(line_entry.GetFileSpec());
-}
+ if (stop_disassembly_display == lldb::eStopDisassemblyTypeAlways)
+ return true;
-protocol::Source CreateSource(llvm::StringRef source_path) {
- protocol::Source source;
- llvm::StringRef name = llvm::sys::path::filename(source_path);
- source.name = name;
- source.path = source_path;
- return source;
+ // A line entry of 0 indicates the line is compiler generated i.e. no source
+ // file is associated with the frame.
+ auto line_entry = address.GetLineEntry();
+ auto file_spec = line_entry.GetFileSpec();
+ if (!file_spec.IsValid() || line_entry.GetLine() == 0 ||
+ line_entry.GetLine() == LLDB_INVALID_LINE_NUMBER)
+ return true;
+
+ if (stop_disassembly_display == lldb::eStopDisassemblyTypeNoSource &&
+ !file_spec.Exists()) {
+ return true;
+ }
+
+ return false;
}
-protocol::Source CreateAssemblySource(const lldb::SBTarget &target,
- lldb::SBAddress &address) {
+static protocol::Source CreateAssemblySource(const lldb::SBTarget &target,
+ lldb::SBAddress address) {
protocol::Source source;
auto symbol = address.GetSymbol();
@@ -558,28 +558,38 @@ protocol::Source CreateAssemblySource(const lldb::SBTarget &target,
return source;
}
-bool ShouldDisplayAssemblySource(
- const lldb::SBLineEntry &line_entry,
- lldb::StopDisassemblyType stop_disassembly_display) {
- if (stop_disassembly_display == lldb::eStopDisassemblyTypeNever)
- return false;
-
- if (stop_disassembly_display == lldb::eStopDisassemblyTypeAlways)
- return true;
-
- // A line entry of 0 indicates the line is compiler generated i.e. no source
- // file is associated with the frame.
- auto file_spec = line_entry.GetFileSpec();
- if (!file_spec.IsValid() || line_entry.GetLine() == 0 ||
- line_entry.GetLine() == LLDB_INVALID_LINE_NUMBER)
- return true;
+protocol::Source CreateSource(const lldb::SBFileSpec &file) {
+ protocol::Source source;
+ if (file.IsValid()) {
+ const char *name = file.GetFilename();
+ if (name)
+ source.name = name;
+ char path[PATH_MAX] = "";
+ if (file.GetPath(path, sizeof(path)) &&
+ lldb::SBFileSpec::ResolvePath(path, path, PATH_MAX))
+ source.path = path;
+ }
+ return source;
+}
- if (stop_disassembly_display == lldb::eStopDisassemblyTypeNoSource &&
- !file_spec.Exists()) {
- return true;
+protocol::Source CreateSource(lldb::SBAddress address, lldb::SBTarget &target) {
+ lldb::SBDebugger debugger = target.GetDebugger();
+ lldb::StopDisassemblyType stop_disassembly_display =
+ GetStopDisassemblyDisplay(debugger);
+ if (!ShouldDisplayAssemblySource(address, stop_disassembly_display)) {
+ lldb::SBLineEntry line_entry = GetLineEntryForAddress(target, address);
+ return CreateSource(line_entry.GetFileSpec());
}
- return false;
+ return CreateAssemblySource(target, address);
+}
+
+protocol::Source CreateSource(llvm::StringRef source_path) {
+ protocol::Source source;
+ llvm::StringRef name = llvm::sys::path::filename(source_path);
+ source.name = name;
+ source.path = source_path;
+ return source;
}
// "StackFrame": {
@@ -643,9 +653,8 @@ bool ShouldDisplayAssemblySource(
// },
// "required": [ "id", "name", "line", "column" ]
// }
-llvm::json::Value
-CreateStackFrame(lldb::SBFrame &frame, lldb::SBFormat &format,
- lldb::StopDisassemblyType stop_disassembly_display) {
+llvm::json::Value CreateStackFrame(lldb::SBFrame &frame,
+ lldb::SBFormat &format) {
llvm::json::Object object;
int64_t frame_id = MakeDAPFrameID(frame);
object.try_emplace("id", frame_id);
@@ -673,22 +682,18 @@ CreateStackFrame(lldb::SBFrame &frame, lldb::SBFormat &format,
EmplaceSafeString(object, "name", frame_name);
- auto line_entry = frame.GetLineEntry();
- if (!ShouldDisplayAssemblySource(line_entry, stop_disassembly_display)) {
- object.try_emplace("source", CreateSource(line_entry));
+ auto target = frame.GetThread().GetProcess().GetTarget();
+ auto source = CreateSource(frame.GetPCAddress(), target);
+ if (!source.IsAssemblySource()) {
+ // This is a normal source with a valid line entry.
+ auto line_entry = frame.GetLineEntry();
object.try_emplace("line", line_entry.GetLine());
auto column = line_entry.GetColumn();
object.try_emplace("column", column);
} else if (frame.GetSymbol().IsValid()) {
- // If no source is associated with the frame, use the DAPFrameID to track
- // the 'source' and generate assembly.
- auto frame_address = frame.GetPCAddress();
- object.try_emplace("source", CreateAssemblySource(
- frame.GetThread().GetProcess().GetTarget(),
- frame_address));
-
- // Calculate the line of the current PC from the start of the current
- // symbol.
+ // This is a source where the disassembly is used, but there is a valid
+ // symbol. Calculate the line of the current PC from the start of the
+ // current symbol.
lldb::SBTarget target = frame.GetThread().GetProcess().GetTarget();
lldb::SBInstructionList inst_list = target.ReadInstructions(
frame.GetSymbol().GetStartAddress(), frame.GetPCAddress(), nullptr);
@@ -699,14 +704,12 @@ CreateStackFrame(lldb::SBFrame &frame, lldb::SBFormat &format,
object.try_emplace("column", 1);
} else {
// No valid line entry or symbol.
- auto frame_address = frame.GetPCAddress();
- object.try_emplace("source", CreateAssemblySource(
- frame.GetThread().GetProcess().GetTarget(),
- frame_address));
object.try_emplace("line", 1);
object.try_emplace("column", 1);
}
+ object.try_emplace("source", std::move(source));
+
const auto pc = frame.GetPC();
if (pc != LLDB_INVALID_ADDRESS) {
std::string formatted_addr = "0x" + llvm::utohexstr(pc);
diff --git a/lldb/tools/lldb-dap/JSONUtils.h b/lldb/tools/lldb-dap/JSONUtils.h
index 5758c3d56ec90..3f9b3162ad027 100644
--- a/lldb/tools/lldb-dap/JSONUtils.h
+++ b/lldb/tools/lldb-dap/JSONUtils.h
@@ -11,7 +11,9 @@
#include "DAPForward.h"
#include "Protocol/ProtocolTypes.h"
+#include "lldb/API/SBAddress.h"
#include "lldb/API/SBCompileUnit.h"
+#include "lldb/API/SBDebugger.h"
#include "lldb/API/SBFileSpec.h"
#include "lldb/API/SBFormat.h"
#include "lldb/API/SBLineEntry.h"
@@ -250,14 +252,16 @@ protocol::Source CreateSource(const lldb::SBFileSpec &file);
/// Create a "Source" JSON object as described in the debug adapter definition.
///
-/// \param[in] line_entry
-/// The LLDB line table to use when populating out the "Source"
-/// object
+/// \param[in] address
+/// The address to use when populating out the "Source" object.
+///
+/// \param[in] target
+/// The target that has the address.
///
/// \return
/// A "Source" JSON object that follows the formal JSON
/// definition outlined by Microsoft.
-protocol::Source CreateSource(const lldb::SBLineEntry &line_entry);
+protocol::Source CreateSource(lldb::SBAddress address, lldb::SBTarget &target);
/// Create a "Source" object for a given source path.
///
@@ -269,35 +273,6 @@ protocol::Source CreateSource(const lldb::SBLineEntry &line_entry);
/// definition outlined by Microsoft.
protocol::Source CreateSource(llvm::StringRef source_path);
-/// Create a "Source" object for a given frame, using its assembly for source.
-///
-/// \param[in] target
-/// The relevant target.
-///
-/// \param[in] address
-/// The address to use when creating the "Source" object.
-///
-/// \return
-/// A "Source" JSON object that follows the formal JSON
-/// definition outlined by Microsoft.
-protocol::Source CreateAssemblySource(const lldb::SBTarget &target,
- lldb::SBAddress &address);
-
-/// Return true if the given line entry should be displayed as assembly.
-///
-/// \param[in] line_entry
-/// The LLDB line entry to check.
-///
-/// \param[in] stop_disassembly_display
-/// The value of the "stop-disassembly-display" setting.
-///
-/// \return
-/// True if the line entry should be displayed as assembly, false
-/// otherwise.
-bool ShouldDisplayAssemblySource(
- const lldb::SBLineEntry &line_entry,
- lldb::StopDisassemblyType stop_disassembly_display);
-
/// Create a "StackFrame" object for a LLDB frame object.
///
/// This function will fill in the following keys in the returned
@@ -316,14 +291,11 @@ bool ShouldDisplayAssemblySource(
/// The LLDB format to use when populating out the "StackFrame"
/// object.
///
-/// \param[in] stop_disassembly_display
-/// The value of the "stop-disassembly-display" setting.
-///
/// \return
/// A "StackFrame" JSON object with that follows the formal JSON
/// definition outlined by Microsoft.
-llvm::json::Value CreateStackFrame(lldb::SBFrame &frame, lldb::SBFormat &format,
- lldb::StopDisassemblyType);
+llvm::json::Value CreateStackFrame(lldb::SBFrame &frame,
+ lldb::SBFormat &format);
/// Create a "StackFrame" label object for a LLDB thread.
///
diff --git a/lldb/tools/lldb-dap/LLDBUtils.cpp b/lldb/tools/lldb-dap/LLDBUtils.cpp
index fe0bcda19b4cd..eb23a89be78da 100644
--- a/lldb/tools/lldb-dap/LLDBUtils.cpp
+++ b/lldb/tools/lldb-dap/LLDBUtils.cpp
@@ -252,4 +252,11 @@ std::string GetSBFileSpecPath(const lldb::SBFileSpec &file_spec) {...
[truncated]
|
eronnen
force-pushed
the
lldb-dap-reuse-source-object-logics
branch
from
4f53b56 to
43ec346
Compare
ashgti
reviewed
May 28, 2025
lldb/include/lldb/API/SBAddress.h
Outdated
Comment on lines
62
to
66
| lldb::SBSymbolContext GetSymbolContext(const SBTarget &target, | ||
| uint32_t resolve_scope); |
There was a problem hiding this comment.
Do we need a new API for this? Can the GetSymbolContext above work for our use case? Or should we call GetLineEntry directly? I think that is an existing function that may get the same data we're trying to resolve.
There was a problem hiding this comment.
The existing SBAdress::GetLineEntry and SBAddress::GetSymbolContext are not sufficient because they don't apply source maps (which fails the source maps tests), so I added a version that can use the target context
There was a problem hiding this comment.
You might put a comment to that effect before your new version.
There was a problem hiding this comment.
Added a small comment
There was a problem hiding this comment.
Does the SBTarget::ResolveSymbolContextForAddress()
llvm-project/lldb/include/lldb/API/SBTarget.h
Lines 587 to 588 in 711a177
There was a problem hiding this comment.
Looking at the SBTarget::ResolveSymbolContextForAddress() implementation it won't apply source maps correctly because it doesn't set the target to the symbol context at any point. in order to apply the source maps you must have sc.target_sp set:
if ((resolve_scope & eSymbolContextLineEntry) && sc.line_entry.IsValid())
sc.line_entry.ApplyFileMappings(sc.target_sp);There was a problem hiding this comment.
Should we fix SBTarget::ResolveSymbolContextForAddress to set the sc.target_sp instead of introducing a new API? Without reading the source, I would have expected the SBTarget API to handle the source mapping correctly.
There was a problem hiding this comment.
Yeah that seems more reasonable, I'll revert the new API in favor of using this function
lldb/tools/lldb-dap/JSONUtils.h
Outdated
| /// | ||
| /// \return | ||
| /// A "Source" JSON object that follows the formal JSON | ||
| /// definition outlined by Microsoft. | ||
| protocol::Source CreateSource(const lldb::SBLineEntry &line_entry); | ||
| protocol::Source CreateSource(lldb::SBAddress address, lldb::SBTarget &target); |
There was a problem hiding this comment.
Do we need to pass in the SBTarget? Isn't that set if we construct the SBAddress using the https://github.com/llvm/llvm-project/blob/892bfa93662329314573cf0a8f5160a9d5d9b34d/lldb/include/lldb/API/SBAddress.h#L25C1-L27C1 constructor?
There was a problem hiding this comment.
the SBTarget is not saved there, it's only used in the constructor to parse the load address
lldb/tools/lldb-dap/LLDBUtils.cpp
Outdated
Comment on lines
257
to
259
| lldb::SBSymbolContext sc = | ||
| address.GetSymbolContext(target, lldb::eSymbolContextLineEntry); | ||
| return sc.GetLineEntry(); |
There was a problem hiding this comment.
Is this the same as address.GetLineEntry()? Should we use that instead?
There was a problem hiding this comment.
Same reason as above, the current SBAdress::GetLineEntry doesn't apply source maps because it doesn't have the target information
ashgti
reviewed
May 28, 2025
lldb/include/lldb/API/SBAddress.h
Outdated
Comment on lines
62
to
66
| lldb::SBSymbolContext GetSymbolContext(const SBTarget &target, | ||
| uint32_t resolve_scope); |
There was a problem hiding this comment.
Does the SBTarget::ResolveSymbolContextForAddress()
llvm-project/lldb/include/lldb/API/SBTarget.h
Lines 587 to 588 in 711a177
There was a problem hiding this comment.
nit: Should this live in the lldb-dap top level folder? I could go either way on that, but it matches the existing helpers.
There was a problem hiding this comment.
Currently I don't think it can live in lldb-dap, because both setting the target and calculating the symbol context are private:
sc.target_sp = target.GetSP();
SymbolContextItem scope = static_cast<SymbolContextItem>(resolve_scope);
if (m_opaque_up->IsValid())
m_opaque_up->CalculateSymbolContext(&sc, scope);There was a problem hiding this comment.
I meant should the file be lldb/tools/lldb-dap/ProtocolUtils.{h,cpp}
There was a problem hiding this comment.
oh, yeah I can move it
eronnen
force-pushed
the
lldb-dap-reuse-source-object-logics
branch
from
5f1959a to
924c1a4
Compare
eronnen
force-pushed
the
lldb-dap-reuse-source-object-logics
branch
from
924c1a4 to
e44a0e3
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Comment on lines
144
to
145
| auto source = CreateSource(addr, target); | ||
| auto line_entry = GetLineEntryForAddress(target, addr); |
There was a problem hiding this comment.
Don't use auto if the type is not obvious from the RHS.
Suggested change
| auto source = CreateSource(addr, target); | |
| auto line_entry = GetLineEntryForAddress(target, addr); | |
| protocol::Source source = CreateSource(addr, target); | |
| SBLineEntry line_entry = GetLineEntryForAddress(target, addr); |
There was a problem hiding this comment.
I thought they were obvious from the RHS but changed :)
lldb/tools/lldb-dap/JSONUtils.cpp
Outdated
Comment on lines
565
to
567
| const char *name = file.GetFilename(); | ||
| if (name) | ||
| source.name = name; |
There was a problem hiding this comment.
Suggested change
| const char *name = file.GetFilename(); | |
| if (name) | |
| source.name = name; | |
| if (const char *name = file.GetFilename()) | |
| source.name = name; |
Comment on lines
13
to
106
| bool IsAssemblySource(const protocol::Source &source) { | ||
| return source.sourceReference.value_or(0) != 0; | ||
| } |
There was a problem hiding this comment.
Why does having a source reference means that the source is assembly?
There was a problem hiding this comment.
When creating the source object if there is source code available we set the path, otherwise we set sourceReference, which means we fall back to assembly.
According to the spec:
/**
* The source location of the breakpoints; either `source.path` or
* `source.sourceReference` must be specified.
*/Also added a comment to the function to make it clearer
lldb/tools/lldb-dap/JSONUtils.cpp
Outdated
Comment on lines
580
to
585
| if (!ShouldDisplayAssemblySource(address, stop_disassembly_display)) { | ||
| lldb::SBLineEntry line_entry = GetLineEntryForAddress(target, address); | ||
| return CreateSource(line_entry.GetFileSpec()); | ||
| } | ||
|
|
||
| return false; | ||
| return CreateAssemblySource(target, address); |
There was a problem hiding this comment.
I think this would read more naturally if you did:
if (ShouldDisplayAssemblySource(...))
return CreateAssemblySource(...);
...
return CreateSource(line_entry.GetFileSpec());
instead of the negation.
lldb/tools/lldb-dap/JSONUtils.cpp
Outdated
| if (!file_spec.IsValid() || line_entry.GetLine() == 0 || | ||
| line_entry.GetLine() == LLDB_INVALID_LINE_NUMBER) | ||
| return true; | ||
| protocol::Source CreateSource(const lldb::SBFileSpec &file) { |
There was a problem hiding this comment.
Should this now live in ProtocolUtils?
ashgti
approved these changes
May 30, 2025
There was a problem hiding this comment.
I meant should the file be lldb/tools/lldb-dap/ProtocolUtils.{h,cpp}
| } | ||
|
|
||
| static protocol::Source CreateAssemblySource(const lldb::SBTarget &target, | ||
| lldb::SBAddress address) { |
There was a problem hiding this comment.
Nit, can this be a const & as well?
There was a problem hiding this comment.
Currently not, because all of the functions in SBAddresss are not const. Technically they can be const but I'm not sure whether it's allowed to change the API
| return source; | ||
| } | ||
|
|
||
| protocol::Source CreateSource(lldb::SBAddress address, lldb::SBTarget &target) { |
There was a problem hiding this comment.
Nit: Can both address and target by const &?
There was a problem hiding this comment.
Currently no because SBTarget::ResolveSymbolContextForAddress is not const. It could be technically but I don't know if I can change the API
Comment on lines
109
to
112
| std::string result; | ||
| llvm::raw_string_ostream os(result); | ||
| os << llvm::format_hex(addr, 18); | ||
| return result; |
There was a problem hiding this comment.
Should we use llvm::utohexstr()? I think we use that in a few other places.
eronnen
force-pushed
the
lldb-dap-reuse-source-object-logics
branch
from
c0f1d3c to
92e8f22
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/197/builds/6086 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/141/builds/9140 Here is the relevant piece of the build log for the reference |
|
FYI, I am not a code owner of lldb-dap or lldb. I think its usually best to wait for one of the owners to sign off before submitting. See https://github.com/llvm/llvm-project/blob/main/lldb/Maintainers.md for the full list of active maintainers. |
DhruvSrivastavaX
pushed a commit
to DhruvSrivastavaX/lldb-for-aix
that referenced
this pull request
Jun 12, 2025
Refactor code revolving source objects such that most logics will be reused. The main change is to expose a single `CreateSource(addr, target)` that can return either a normal or an assembly source object, and call `ShouldDisplayAssemblySource()` only from this function instead of multiple places across the code. Other functions can use `source.IsAssemblySource()` in order to check which type the source is.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactor code revolving source objects such that most logics will be reused.
The main change is to expose a single
CreateSource(addr, target)that can return either a normal or an assembly source object, and callShouldDisplayAssemblySource()only from this function instead of multiple places across the code.Other functions can use
source.IsAssemblySource()in order to check which type the source is.