[lldb-dap] Fix source references #144364
Open
[lldb-dap] Fix source references #144364
+180
−74
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: Ebuka Ezike (da-viper) ChangesThe protocol expects that Make the I passed the Patch is 22.13 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/144364.diff 16 Files Affected:
diff --git a/lldb/test/API/tools/lldb-dap/breakpoint-assembly/TestDAP_breakpointAssembly.py b/lldb/test/API/tools/lldb-dap/breakpoint-assembly/TestDAP_breakpointAssembly.py
index 8bccc2bcf4156..674bfe4199b4a 100644
--- a/lldb/test/API/tools/lldb-dap/breakpoint-assembly/TestDAP_breakpointAssembly.py
+++ b/lldb/test/API/tools/lldb-dap/breakpoint-assembly/TestDAP_breakpointAssembly.py
@@ -67,19 +67,19 @@ def test_break_on_invalid_source_reference(self):
"Invalid sourceReference.",
)
- # Verify that setting a breakpoint on a source reference without a symbol also fails
+ # Verify that setting a breakpoint on a source reference that is not created fails
response = self.dap_server.request_setBreakpoints(
- Source(source_reference=0), [1]
+ Source(source_reference=200), [1]
)
self.assertIsNotNone(response)
breakpoints = response["body"]["breakpoints"]
self.assertEqual(len(breakpoints), 1)
- breakpoint = breakpoints[0]
+ break_point = breakpoints[0]
self.assertFalse(
- breakpoint["verified"], "Expected breakpoint to not be verified"
+ break_point["verified"], "Expected breakpoint to not be verified"
)
- self.assertIn("message", breakpoint, "Expected message to be present")
+ self.assertIn("message", break_point, "Expected message to be present")
self.assertEqual(
- breakpoint["message"],
- "Breakpoints in assembly without a valid symbol are not supported yet.",
+ break_point["message"],
+ "Invalid sourceReference.",
)
diff --git a/lldb/tools/lldb-dap/Breakpoint.cpp b/lldb/tools/lldb-dap/Breakpoint.cpp
index ef5646c4c3d16..e1b073405ebb2 100644
--- a/lldb/tools/lldb-dap/Breakpoint.cpp
+++ b/lldb/tools/lldb-dap/Breakpoint.cpp
@@ -64,8 +64,11 @@ protocol::Breakpoint Breakpoint::ToProtocolBreakpoint() {
"0x" + llvm::utohexstr(bp_addr.GetLoadAddress(m_bp.GetTarget()));
breakpoint.instructionReference = formatted_addr;
- auto source = CreateSource(bp_addr, m_dap.target);
- if (!IsAssemblySource(source)) {
+ std::optional<protocol::Source> source =
+ CreateSource(bp_addr, m_dap.target, [this](lldb::addr_t addr) {
+ return m_dap.CreateSourceReference(addr);
+ });
+ if (source && !IsAssemblySource(*source)) {
auto line_entry = bp_addr.GetLineEntry();
const auto line = line_entry.GetLine();
if (line != LLDB_INVALID_LINE_NUMBER)
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index 9fe8227cd2d6f..56bb5903080dc 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -510,6 +510,25 @@ DAP::SendFormattedOutput(OutputType o, const char *format, ...) {
o, llvm::StringRef(buffer, std::min<int>(actual_length, sizeof(buffer))));
}
+int32_t DAP::CreateSourceReference(lldb::addr_t address) {
+ auto iter = llvm::find(source_references, address);
+ if (iter != source_references.end())
+ return std::distance(source_references.begin(), iter) + 1;
+
+ source_references.emplace_back(address);
+ return static_cast<int32_t>(source_references.size());
+}
+
+std::optional<lldb::addr_t> DAP::GetSourceReferenceAddress(int32_t reference) {
+ if (reference <= LLDB_DAP_INVALID_SRC_REF)
+ return std::nullopt;
+
+ if (static_cast<size_t>(reference) > source_references.size())
+ return std::nullopt;
+
+ return source_references[reference - 1];
+}
+
ExceptionBreakpoint *DAP::GetExceptionBPFromStopReason(lldb::SBThread &thread) {
const auto num = thread.GetStopReasonDataCount();
// Check to see if have hit an exception breakpoint and change the
diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index 89bc827c1141f..2766c11b48963 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/lldb/tools/lldb-dap/DAP.h
@@ -105,6 +105,9 @@ struct DAP {
/// Map step in target id to list of function targets that user can choose.
llvm::DenseMap<lldb::addr_t, std::string> step_in_targets;
+ /// list of addresses mapped by sourceReference(index - 1)
+ std::vector<lldb::addr_t> source_references;
+
/// A copy of the last LaunchRequest so we can reuse its arguments if we get a
/// RestartRequest. Restarting an AttachRequest is not supported.
std::optional<protocol::LaunchRequestArguments> last_launch_request;
@@ -219,7 +222,9 @@ struct DAP {
void __attribute__((format(printf, 3, 4)))
SendFormattedOutput(OutputType o, const char *format, ...);
- static int64_t GetNextSourceReference();
+ int32_t CreateSourceReference(lldb::addr_t address);
+
+ std::optional<lldb::addr_t> GetSourceReferenceAddress(int32_t reference);
ExceptionBreakpoint *GetExceptionBPFromStopReason(lldb::SBThread &thread);
diff --git a/lldb/tools/lldb-dap/Handler/DisassembleRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/DisassembleRequestHandler.cpp
index d5878d18289d6..19863f3144018 100644
--- a/lldb/tools/lldb-dap/Handler/DisassembleRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/DisassembleRequestHandler.cpp
@@ -85,7 +85,8 @@ static lldb::SBAddress GetDisassembleStartAddress(lldb::SBTarget target,
}
static DisassembledInstruction ConvertSBInstructionToDisassembledInstruction(
- lldb::SBTarget &target, lldb::SBInstruction &inst, bool resolve_symbols) {
+ DAP &dap, lldb::SBInstruction &inst, bool resolve_symbols) {
+ lldb::SBTarget target = dap.target;
if (!inst.IsValid())
return GetInvalidInstruction();
@@ -142,14 +143,17 @@ static DisassembledInstruction ConvertSBInstructionToDisassembledInstruction(
disassembled_inst.instruction = std::move(instruction);
- protocol::Source source = CreateSource(addr, target);
+ std::optional<protocol::Source> source =
+ CreateSource(addr, target, [&dap](lldb::addr_t addr) {
+ return dap.CreateSourceReference(addr);
+ });
lldb::SBLineEntry line_entry = GetLineEntryForAddress(target, addr);
// If the line number is 0 then the entry represents a compiler generated
// location.
- if (!IsAssemblySource(source) && line_entry.GetStartAddress() == addr &&
- line_entry.IsValid() && line_entry.GetFileSpec().IsValid() &&
- line_entry.GetLine() != 0) {
+ if (source && !IsAssemblySource(*source) &&
+ line_entry.GetStartAddress() == addr && line_entry.IsValid() &&
+ line_entry.GetFileSpec().IsValid() && line_entry.GetLine() != 0) {
disassembled_inst.location = std::move(source);
const auto line = line_entry.GetLine();
@@ -225,7 +229,7 @@ DisassembleRequestHandler::Run(const DisassembleArguments &args) const {
original_address_index = i;
instructions.push_back(ConvertSBInstructionToDisassembledInstruction(
- dap.target, inst, resolve_symbols));
+ dap, inst, resolve_symbols));
}
// Check if we miss instructions at the beginning.
diff --git a/lldb/tools/lldb-dap/Handler/LocationsRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/LocationsRequestHandler.cpp
index 804bdcd622a2b..cf9b5a3dbd06b 100644
--- a/lldb/tools/lldb-dap/Handler/LocationsRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/LocationsRequestHandler.cpp
@@ -137,7 +137,16 @@ void LocationsRequestHandler::operator()(
return;
}
- body.try_emplace("source", CreateSource(line_entry.GetFileSpec()));
+ const std::optional<protocol::Source> source =
+ CreateSource(line_entry.GetFileSpec());
+ if (!source) {
+ response["success"] = false;
+ response["message"] = "Failed to resolve file path for location";
+ dap.SendJSON(llvm::json::Value(std::move(response)));
+ return;
+ }
+
+ body.try_emplace("source", *source);
if (int line = line_entry.GetLine())
body.try_emplace("line", line);
if (int column = line_entry.GetColumn())
@@ -152,7 +161,16 @@ void LocationsRequestHandler::operator()(
return;
}
- body.try_emplace("source", CreateSource(decl.GetFileSpec()));
+ const std::optional<protocol::Source> source =
+ CreateSource(decl.GetFileSpec());
+ if (!source) {
+ response["success"] = false;
+ response["message"] = "Failed to resolve file path for location";
+ dap.SendJSON(llvm::json::Value(std::move(response)));
+ return;
+ }
+
+ body.try_emplace("source", *source);
if (int line = decl.GetLine())
body.try_emplace("line", line);
if (int column = decl.GetColumn())
diff --git a/lldb/tools/lldb-dap/Handler/SourceRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/SourceRequestHandler.cpp
index 353d3365564f5..755ad206abe26 100644
--- a/lldb/tools/lldb-dap/Handler/SourceRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/SourceRequestHandler.cpp
@@ -29,34 +29,42 @@ namespace lldb_dap {
/// the source code for a given source reference.
llvm::Expected<protocol::SourceResponseBody>
SourceRequestHandler::Run(const protocol::SourceArguments &args) const {
- const auto source =
+
+ uint32_t source_ref =
args.source->sourceReference.value_or(args.sourceReference);
+ const std::optional<lldb::addr_t> source_addr_opt =
+ dap.GetSourceReferenceAddress(source_ref);
- if (!source)
+ if (!source_addr_opt)
return llvm::make_error<DAPError>(
- "invalid arguments, expected source.sourceReference to be set");
+ llvm::formatv("Unknown source reference {}", source_ref));
- lldb::SBAddress address(source, dap.target);
+ lldb::SBAddress address(*source_addr_opt, dap.target);
if (!address.IsValid())
return llvm::make_error<DAPError>("source not found");
lldb::SBSymbol symbol = address.GetSymbol();
-
- lldb::SBStream stream;
- lldb::SBExecutionContext exe_ctx(dap.target);
+ lldb::SBInstructionList insts;
if (symbol.IsValid()) {
- lldb::SBInstructionList insts = symbol.GetInstructions(dap.target);
- insts.GetDescription(stream, exe_ctx);
+ insts = symbol.GetInstructions(dap.target);
} else {
// No valid symbol, just return the disassembly.
- lldb::SBInstructionList insts = dap.target.ReadInstructions(
+ insts = dap.target.ReadInstructions(
address, dap.k_number_of_assembly_lines_for_nodebug);
- insts.GetDescription(stream, exe_ctx);
}
+ if (!insts || insts.GetSize() == 0)
+ return llvm::make_error<DAPError>(
+ llvm::formatv("no instruction source for address {}",
+ address.GetLoadAddress(dap.target)));
+
+ lldb::SBStream stream;
+ lldb::SBExecutionContext exe_ctx(dap.target);
+ insts.GetDescription(stream, exe_ctx);
return protocol::SourceResponseBody{/*content=*/stream.GetData(),
- /*mimeType=*/"text/x-lldb.disassembly"};
+ /*mimeType=*/
+ "text/x-lldb.disassembly"};
}
} // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/Handler/StackTraceRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/StackTraceRequestHandler.cpp
index 4ea4cd1e517d4..77ef952a1e343 100644
--- a/lldb/tools/lldb-dap/Handler/StackTraceRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/StackTraceRequestHandler.cpp
@@ -69,7 +69,7 @@ static bool FillStackFrames(DAP &dap, lldb::SBThread &thread,
break;
}
- stack_frames.emplace_back(CreateStackFrame(frame, frame_format));
+ stack_frames.emplace_back(CreateStackFrame(dap, 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 6cdde63e9796e..8091809f55791 100644
--- a/lldb/tools/lldb-dap/JSONUtils.cpp
+++ b/lldb/tools/lldb-dap/JSONUtils.cpp
@@ -552,7 +552,7 @@ CreateExceptionBreakpointFilter(const ExceptionBreakpoint &bp) {
// },
// "required": [ "id", "name", "line", "column" ]
// }
-llvm::json::Value CreateStackFrame(lldb::SBFrame &frame,
+llvm::json::Value CreateStackFrame(DAP &dap, lldb::SBFrame &frame,
lldb::SBFormat &format) {
llvm::json::Object object;
int64_t frame_id = MakeDAPFrameID(frame);
@@ -582,8 +582,11 @@ llvm::json::Value CreateStackFrame(lldb::SBFrame &frame,
EmplaceSafeString(object, "name", frame_name);
auto target = frame.GetThread().GetProcess().GetTarget();
- auto source = CreateSource(frame.GetPCAddress(), target);
- if (!IsAssemblySource(source)) {
+ std::optional<protocol::Source> source =
+ CreateSource(frame.GetPCAddress(), target, [&dap](lldb::addr_t addr) {
+ return dap.CreateSourceReference(addr);
+ });
+ if (source && !IsAssemblySource(*source)) {
// This is a normal source with a valid line entry.
auto line_entry = frame.GetLineEntry();
object.try_emplace("line", line_entry.GetLine());
@@ -607,7 +610,8 @@ llvm::json::Value CreateStackFrame(lldb::SBFrame &frame,
object.try_emplace("column", 1);
}
- object.try_emplace("source", std::move(source));
+ if (source)
+ object.try_emplace("source", std::move(source).value());
const auto pc = frame.GetPC();
if (pc != LLDB_INVALID_ADDRESS) {
diff --git a/lldb/tools/lldb-dap/JSONUtils.h b/lldb/tools/lldb-dap/JSONUtils.h
index 10dc46b94184f..dcf9453a72e90 100644
--- a/lldb/tools/lldb-dap/JSONUtils.h
+++ b/lldb/tools/lldb-dap/JSONUtils.h
@@ -246,6 +246,9 @@ CreateExceptionBreakpointFilter(const ExceptionBreakpoint &bp);
/// "line" - the source file line number as an integer
/// "column" - the source file column number as an integer
///
+/// \param[in] dap
+/// The DAP session associated with the stopped thread.
+///
/// \param[in] frame
/// The LLDB stack frame to use when populating out the "StackFrame"
/// object.
@@ -257,7 +260,7 @@ CreateExceptionBreakpointFilter(const ExceptionBreakpoint &bp);
/// \return
/// A "StackFrame" JSON object with that follows the formal JSON
/// definition outlined by Microsoft.
-llvm::json::Value CreateStackFrame(lldb::SBFrame &frame,
+llvm::json::Value CreateStackFrame(DAP &dap, lldb::SBFrame &frame,
lldb::SBFormat &format);
/// Create a "StackFrame" label object for a LLDB thread.
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h
index d199cc886b11c..0f7820a3ddebb 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h
@@ -468,7 +468,7 @@ struct SourceArguments {
/// The reference to the source. This is the same as `source.sourceReference`.
/// This is provided for backward compatibility since old clients do not
/// understand the `source` attribute.
- int64_t sourceReference;
+ int64_t sourceReference = LLDB_DAP_INVALID_SRC_REF;
};
bool fromJSON(const llvm::json::Value &, SourceArguments &, llvm::json::Path);
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp
index 085d53bb006ef..4736c6b6d290c 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp
@@ -65,7 +65,7 @@ llvm::json::Value toJSON(const Source &S) {
result.insert({"name", *S.name});
if (S.path)
result.insert({"path", *S.path});
- if (S.sourceReference)
+ if (S.sourceReference && (*S.sourceReference > LLDB_DAP_INVALID_SRC_REF))
result.insert({"sourceReference", *S.sourceReference});
if (S.presentationHint)
result.insert({"presentationHint", *S.presentationHint});
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h
index c7acfc482987b..4a2f940916134 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h
@@ -28,6 +28,7 @@
#include <string>
#define LLDB_DAP_INVALID_VARRERF UINT64_MAX
+#define LLDB_DAP_INVALID_SRC_REF 0
namespace lldb_dap::protocol {
@@ -309,7 +310,7 @@ struct Source {
/// `source` request (even if a path is specified). Since a `sourceReference`
/// is only valid for a session, it can not be used to persist a source. The
/// value should be less than or equal to 2147483647 (2^31-1).
- std::optional<int64_t> sourceReference;
+ std::optional<int32_t> sourceReference;
/// A hint for how to present the source in the UI. A value of `deemphasize`
/// can be used to indicate that the source is not available or that it is
diff --git a/lldb/tools/lldb-dap/ProtocolUtils.cpp b/lldb/tools/lldb-dap/ProtocolUtils.cpp
index 6e0adf5bc8b59..aa43b298339ba 100644
--- a/lldb/tools/lldb-dap/ProtocolUtils.cpp
+++ b/lldb/tools/lldb-dap/ProtocolUtils.cpp
@@ -46,21 +46,27 @@ static bool ShouldDisplayAssemblySource(
return false;
}
-static protocol::Source CreateAssemblySource(const lldb::SBTarget &target,
- lldb::SBAddress address) {
- protocol::Source source;
+static std::optional<protocol::Source> CreateAssemblySource(
+ const lldb::SBTarget &target, lldb::SBAddress address,
+ llvm::function_ref<uint32_t(lldb::addr_t)> create_reference) {
- auto symbol = address.GetSymbol();
+ lldb::SBSymbol symbol = address.GetSymbol();
+ lldb::addr_t load_addr = LLDB_INVALID_ADDRESS;
std::string name;
if (symbol.IsValid()) {
- source.sourceReference = symbol.GetStartAddress().GetLoadAddress(target);
+ load_addr = symbol.GetStartAddress().GetLoadAddress(target);
name = symbol.GetName();
} else {
- const auto load_addr = address.GetLoadAddress(target);
- source.sourceReference = load_addr;
+ load_addr = address.GetLoadAddress(target);
name = GetLoadAddressString(load_addr);
}
+ if (load_addr == LLDB_INVALID_ADDRESS) {
+ return std::nullopt;
+ }
+
+ protocol::Source source;
+ source.sourceReference = create_reference(load_addr);
lldb::SBModule module = address.GetModule();
if (module.IsValid()) {
lldb::SBFileSpec file_spec = module.GetFileSpec();
@@ -81,27 +87,33 @@ static protocol::Source CreateAssemblySource(const lldb::SBTarget &target,
return source;
}
-protocol::Source CreateSource(const lldb::SBFileSpec &file) {
+std::optional<protocol::Source> CreateSource(const lldb::SBFileSpec &file) {
+ if (!file.IsValid())
+ return std::nullopt;
+
protocol::Source source;
- if (file.IsValid()) {
- if (const char *name = file.GetFilename())
- source.name = name;
- char path[PATH_MAX] = "";
- if (file.GetPath(path, sizeof(path)) &&
- lldb::SBFileSpec::ResolvePath(path, path, PATH_MAX))
- source.path = path;
- }
+ if (const char *name = file.GetFilename())
+ 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;
}
-protocol::Source CreateSource(lldb::SBAddress address, lldb::SBTarget &target) {
+std::optional<protocol::Source>
+CreateSource(lldb::SBAddress address, lldb::SBTarget &target,
+ llvm::function_ref<int32_t(lldb::addr_t)> create_reference) {
lldb::SBDebugger debugger = target.GetDebugger();
lldb::StopDisassemblyType stop_disassembly_display =
GetStopDisassemblyDisplay(debugger);
if (ShouldDisplayAssemblySource(address, stop_disassembly_display))
- return CreateAssemblySource(target, address);
+ return CreateAssemblySource(target, address, create_reference);
lldb::SBLineEntry line_entry = GetLineEntryForAddress(target, address);
+ if (!line_entry.IsValid())
+ return std::nullopt;
+
return CreateSource(line_entry.GetFileSpec());
}
@@ -109,7 +121,8 @@ bool IsAssemblySource(const protocol::Source &source) {
// According to the specification, a source must have either `path` or
// `sourceReference` specified. We use `path` for sources with known source
// code, and `sourceReferences` when falling back to assembly.
- return source.sourceReference.value_or(0) != 0;
+ return source.sourceReference.value_or(LLDB_DAP_INVALID_SRC_REF) >
+ LLDB_DAP_INVALID_SRC_REF;
}
std::string GetLoadAddressString(const lldb::addr_t addr) {
diff --git a/lldb/tools/lldb-dap/ProtocolUtils.h b/lldb/tools/lldb-dap/ProtocolUtils.h
index 2b2ac9e8e35fd..bed5cc686335e 100644
--- a/lldb/tools/lldb-dap/ProtocolUtils.h
+++ b/lldb/tools/lldb-dap/ProtocolUtils.h
@@ -25,9 +25,9 @@ namespace lldb_dap {
/// The SBFileSpec to use when populating out the "Source" object
///
/// \return
-/// ...
[truncated]
|
ashgti
reviewed
Jun 16, 2025
lldb/tools/lldb-dap/ProtocolUtils.h
Outdated
| protocol::Source CreateSource(lldb::SBAddress address, lldb::SBTarget &target); | ||
| std::optional<protocol::Source> | ||
| CreateSource(lldb::SBAddress address, lldb::SBTarget &target, | ||
| llvm::function_ref<int32_t(lldb::addr_t)> create_reference); |
There was a problem hiding this comment.
Should we move this into the DAP? Something like optional<Source> DAP::ResolveSource(SBAddress, SBTarget); that creates or returns a source for the given address?
Or we could move the actual address info into the adapterData field and then we don't need to track the address in the DAP, we'd just need to have some way of hashing the address into a unique sourceReference.
There was a problem hiding this comment.
Should we move this into the DAP? Something like optional
DAP::ResolveSource(SBAddress, SBTarget); that creates or returns a source for the given address?
I am leaning towards this because there may be clients that do not support the optional source argument in SourceRequest
The protocol expects that `sourceReference` be less than `(2^31)-1`, but we currently represent memory address as source reference, this can be truncated either when sending through json or by the client. Instead, generate new source references based on the memory address. Make the `CreateSource` function return an optional source. # Conflicts: # lldb/tools/lldb-dap/Handler/DisassembleRequestHandler.cpp
da-viper
force-pushed
the
fix-source-reference
branch
from
3e13856 to
03a1099
Compare
|
To make sure I understand the problem: for assembly sources, we use the memory address (i.e. the PC) as the |
Yes this is the case, it may get truncated since the memory address is larger than the size of the sourceReference specified in the spec. |
ashgti
approved these changes
Jun 17, 2025
There was a problem hiding this comment.
I looked at the code some more and it seems like we return the address of the symbol if there is one, and only use the actual address (i.e. the pc) when there is none. I was trying to find the code that computes the line in that case (i.e. pc - symbol) and I couldn't immediately find it.
I think this PR is fine in the sense that it improves the status quo, but we probably want to take a more wholistic approach at this problem at some point. For example, if we don't have a symbol, it would be nice to have a source reference for a given range (say the next 64 or 128 instructions) and then as we step keep using that sourceReference but bump the "relative line number" in that artificial source file.
| @@ -406,6 +419,10 @@ struct DAP { | |||
| std::thread progress_event_thread; | |||
| /// @} | |||
|
|
|||
| /// list of addresses mapped by sourceReference(index - 1) | |||
There was a problem hiding this comment.
index - 1 seems wrong? That would mean that the address at index 0 has a source reference of -1?
Suggested change
| /// list of addresses mapped by sourceReference(index - 1) | |
| /// List of addresses mapped by sourceReference. |
| protocol::Source source; | ||
| std::optional<protocol::Source> CreateAssemblySource( | ||
| const lldb::SBTarget &target, lldb::SBAddress address, | ||
| llvm::function_ref<int32_t(lldb::addr_t)> create_reference) { |
There was a problem hiding this comment.
I think a callback is a sign that this isn't the right layering. Why not move this into DAP? That would match what we do for variables and variable references (though unlike Variables, we probably don't need a separate class for this)
There was a problem hiding this comment.
Will move it, did not want to since it was DAP.cpp was get quite large.
we use the address to create a source reference. llvm-project/lldb/tools/lldb-dap/DAP.cpp Lines 513 to 530 in 8b79cd7
Yeah makes more sense, |
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.
The protocol expects that
sourceReferencebe less than(2^31)-1, but we currently represent memory address as source reference, this can be truncated either when sending through json or by the client. Instead, generate new source references based on the memory address.Make the
CreateSourcefunction return an optional source.I passed the
create_referencefunction toCreateSourceto avoid the cyclic dependency ofProtocolUtilswithDAP