[lldb-dap] Use structured types for stepInTargets request #142439
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
da-viper
requested review from
ashgti and
eronnen
and removed request for
JDevlieghere
|
@llvm/pr-subscribers-lldb Author: Ebuka Ezike (da-viper) ChangesFull diff: https://github.com/llvm/llvm-project/pull/142439.diff 7 Files Affected:
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.h b/lldb/tools/lldb-dap/Handler/RequestHandler.h
index 3a965bcc87a5e..559929ffb21e8 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.h
+++ b/lldb/tools/lldb-dap/Handler/RequestHandler.h
@@ -356,7 +356,21 @@ class StepInRequestHandler : public RequestHandler<protocol::StepInArguments,
llvm::Error Run(const protocol::StepInArguments &args) const override;
};
-class StepInTargetsRequestHandler : public LegacyRequestHandler {
+class StepInTargetsRequestHandler
+ : public RequestHandler<
+ protocol::StepInTargetsArguments,
+ llvm::Expected<protocol::StepInTargetsResponseBody>> {
+public:
+ using RequestHandler::RequestHandler;
+ static llvm::StringLiteral GetCommand() { return "stepInTargets"; }
+ FeatureSet GetSupportedFeatures() const override {
+ return {protocol::eAdapterFeatureStepInTargetsRequest};
+ }
+ llvm::Expected<protocol::StepInTargetsResponseBody>
+ Run(const protocol::StepInTargetsArguments &args) const override;
+};
+
+class StepInTargetsRequestHandler2 : public LegacyRequestHandler {
public:
using LegacyRequestHandler::LegacyRequestHandler;
static llvm::StringLiteral GetCommand() { return "stepInTargets"; }
diff --git a/lldb/tools/lldb-dap/Handler/StepInTargetsRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/StepInTargetsRequestHandler.cpp
index 9b99791599f82..1a76371be2d58 100644
--- a/lldb/tools/lldb-dap/Handler/StepInTargetsRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/StepInTargetsRequestHandler.cpp
@@ -7,143 +7,85 @@
//===----------------------------------------------------------------------===//
#include "DAP.h"
-#include "EventHelper.h"
-#include "JSONUtils.h"
+#include "Protocol/ProtocolRequests.h"
#include "RequestHandler.h"
#include "lldb/API/SBInstruction.h"
+#include "lldb/lldb-defines.h"
+using namespace lldb_dap::protocol;
namespace lldb_dap {
-// "StepInTargetsRequest": {
-// "allOf": [ { "$ref": "#/definitions/Request" }, {
-// "type": "object",
-// "description": "This request retrieves the possible step-in targets for
-// the specified stack frame.\nThese targets can be used in the `stepIn`
-// request.\nClients should only call this request if the corresponding
-// capability `supportsStepInTargetsRequest` is true.", "properties": {
-// "command": {
-// "type": "string",
-// "enum": [ "stepInTargets" ]
-// },
-// "arguments": {
-// "$ref": "#/definitions/StepInTargetsArguments"
-// }
-// },
-// "required": [ "command", "arguments" ]
-// }]
-// },
-// "StepInTargetsArguments": {
-// "type": "object",
-// "description": "Arguments for `stepInTargets` request.",
-// "properties": {
-// "frameId": {
-// "type": "integer",
-// "description": "The stack frame for which to retrieve the possible
-// step-in targets."
-// }
-// },
-// "required": [ "frameId" ]
-// },
-// "StepInTargetsResponse": {
-// "allOf": [ { "$ref": "#/definitions/Response" }, {
-// "type": "object",
-// "description": "Response to `stepInTargets` request.",
-// "properties": {
-// "body": {
-// "type": "object",
-// "properties": {
-// "targets": {
-// "type": "array",
-// "items": {
-// "$ref": "#/definitions/StepInTarget"
-// },
-// "description": "The possible step-in targets of the specified
-// source location."
-// }
-// },
-// "required": [ "targets" ]
-// }
-// },
-// "required": [ "body" ]
-// }]
-// }
-void StepInTargetsRequestHandler::operator()(
- const llvm::json::Object &request) const {
- llvm::json::Object response;
- FillResponse(request, response);
- const auto *arguments = request.getObject("arguments");
-
+// This request retrieves the possible step-in targets for the specified stack
+// frame.
+// These targets can be used in the `stepIn` request.
+// Clients should only call this request if the corresponding capability
+// `supportsStepInTargetsRequest` is true.
+llvm::Expected<StepInTargetsResponseBody>
+StepInTargetsRequestHandler::Run(const StepInTargetsArguments &args) const {
dap.step_in_targets.clear();
- lldb::SBFrame frame = dap.GetLLDBFrame(*arguments);
- if (frame.IsValid()) {
- lldb::SBAddress pc_addr = frame.GetPCAddress();
- lldb::SBAddress line_end_addr =
- pc_addr.GetLineEntry().GetSameLineContiguousAddressRangeEnd(true);
- lldb::SBInstructionList insts = dap.target.ReadInstructions(
- pc_addr, line_end_addr, /*flavor_string=*/nullptr);
-
- if (!insts.IsValid()) {
- response["success"] = false;
- response["message"] = "Failed to get instructions for frame.";
- dap.SendJSON(llvm::json::Value(std::move(response)));
- return;
- }
+ const lldb::SBFrame frame = dap.GetLLDBFrame(args.frameId);
+ if (!frame.IsValid())
+ return llvm::make_error<DAPError>("Failed to get frame for input frameId.");
+
+ lldb::SBAddress pc_addr = frame.GetPCAddress();
+ lldb::SBAddress line_end_addr =
+ pc_addr.GetLineEntry().GetSameLineContiguousAddressRangeEnd(true);
+ lldb::SBInstructionList insts = dap.target.ReadInstructions(
+ pc_addr, line_end_addr, /*flavor_string=*/nullptr);
+
+ if (!insts.IsValid())
+ return llvm::make_error<DAPError>("Failed to get instructions for frame.");
+
+ StepInTargetsResponseBody body;
+ const size_t num_insts = insts.GetSize();
+ for (size_t i = 0; i < num_insts; ++i) {
+ lldb::SBInstruction inst = insts.GetInstructionAtIndex(i);
+ if (!inst.IsValid())
+ break;
+
+ const lldb::addr_t inst_addr = inst.GetAddress().GetLoadAddress(dap.target);
+ if (inst_addr == LLDB_INVALID_ADDRESS)
+ break;
+
+ // Note: currently only x86/x64 supports flow kind.
+ const lldb::InstructionControlFlowKind flow_kind =
+ inst.GetControlFlowKind(dap.target);
+
+ if (flow_kind == lldb::eInstructionControlFlowKindCall) {
+
+ const llvm::StringRef call_operand_name = inst.GetOperands(dap.target);
+ lldb::addr_t call_target_addr = LLDB_INVALID_ADDRESS;
+ if (call_operand_name.getAsInteger(0, call_target_addr))
+ continue;
+
+ const lldb::SBAddress call_target_load_addr =
+ dap.target.ResolveLoadAddress(call_target_addr);
+ if (!call_target_load_addr.IsValid())
+ continue;
+
+ // The existing ThreadPlanStepInRange only accept step in target
+ // function with debug info.
+ lldb::SBSymbolContext sc = dap.target.ResolveSymbolContextForAddress(
+ call_target_load_addr, lldb::eSymbolContextFunction);
+
+ // The existing ThreadPlanStepInRange only accept step in target
+ // function with debug info.
+ llvm::StringRef step_in_target_name;
+ if (sc.IsValid() && sc.GetFunction().IsValid())
+ step_in_target_name = sc.GetFunction().GetDisplayName();
+
+ // Skip call sites if we fail to resolve its symbol name.
+ if (step_in_target_name.empty())
+ continue;
- llvm::json::Array step_in_targets;
- const auto num_insts = insts.GetSize();
- for (size_t i = 0; i < num_insts; ++i) {
- lldb::SBInstruction inst = insts.GetInstructionAtIndex(i);
- if (!inst.IsValid())
- break;
-
- lldb::addr_t inst_addr = inst.GetAddress().GetLoadAddress(dap.target);
-
- // Note: currently only x86/x64 supports flow kind.
- lldb::InstructionControlFlowKind flow_kind =
- inst.GetControlFlowKind(dap.target);
- if (flow_kind == lldb::eInstructionControlFlowKindCall) {
- // Use call site instruction address as id which is easy to debug.
- llvm::json::Object step_in_target;
- step_in_target["id"] = inst_addr;
-
- llvm::StringRef call_operand_name = inst.GetOperands(dap.target);
- lldb::addr_t call_target_addr;
- if (call_operand_name.getAsInteger(0, call_target_addr))
- continue;
-
- lldb::SBAddress call_target_load_addr =
- dap.target.ResolveLoadAddress(call_target_addr);
- if (!call_target_load_addr.IsValid())
- continue;
-
- // The existing ThreadPlanStepInRange only accept step in target
- // function with debug info.
- lldb::SBSymbolContext sc = dap.target.ResolveSymbolContextForAddress(
- call_target_load_addr, lldb::eSymbolContextFunction);
-
- // The existing ThreadPlanStepInRange only accept step in target
- // function with debug info.
- std::string step_in_target_name;
- if (sc.IsValid() && sc.GetFunction().IsValid())
- step_in_target_name = sc.GetFunction().GetDisplayName();
-
- // Skip call sites if we fail to resolve its symbol name.
- if (step_in_target_name.empty())
- continue;
-
- dap.step_in_targets.try_emplace(inst_addr, step_in_target_name);
- step_in_target.try_emplace("label", step_in_target_name);
- step_in_targets.emplace_back(std::move(step_in_target));
- }
+ StepInTarget target;
+ target.id = inst_addr;
+ target.label = step_in_target_name;
+ dap.step_in_targets.try_emplace(inst_addr, step_in_target_name);
+ body.targets.emplace_back(std::move(target));
}
- llvm::json::Object body;
- body.try_emplace("targets", std::move(step_in_targets));
- response.try_emplace("body", std::move(body));
- } else {
- response["success"] = llvm::json::Value(false);
- response["message"] = "Failed to get frame for input frameId.";
}
- dap.SendJSON(llvm::json::Value(std::move(response)));
-}
+ return body;
+};
} // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp
index 4160077d419e1..a7f28fb566d3d 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp
@@ -382,6 +382,15 @@ bool fromJSON(const llvm::json::Value &Params, StepInArguments &SIA,
OM.mapOptional("granularity", SIA.granularity);
}
+bool fromJSON(const llvm::json::Value &Params, StepInTargetsArguments &SITA,
+ llvm::json::Path P) {
+ json::ObjectMapper OM(Params, P);
+ return OM && OM.map("frameId", SITA.frameId);
+}
+llvm::json::Value toJSON(const StepInTargetsResponseBody &SITR) {
+ return llvm::json::Object{{"targets", SITR.targets}};
+}
+
bool fromJSON(const llvm::json::Value &Params, StepOutArguments &SOA,
llvm::json::Path P) {
json::ObjectMapper OM(Params, P);
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h
index 7c774e50d6e56..570a0b6d39682 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h
@@ -523,6 +523,21 @@ bool fromJSON(const llvm::json::Value &, StepInArguments &, llvm::json::Path);
/// body field is required.
using StepInResponse = VoidResponse;
+/// Arguments for `stepInTargets` request.
+struct StepInTargetsArguments {
+ /// The stack frame for which to retrieve the possible step-in targets.
+ uint64_t frameId = LLDB_INVALID_FRAME_ID;
+};
+bool fromJSON(const llvm::json::Value &, StepInTargetsArguments &,
+ llvm::json::Path);
+
+/// Response to `stepInTargets` request.
+struct StepInTargetsResponseBody {
+ /// The possible step-in targets of the specified source location.
+ std::vector<StepInTarget> targets;
+};
+llvm::json::Value toJSON(const StepInTargetsResponseBody &);
+
/// Arguments for `stepOut` request.
struct StepOutArguments {
/// Specifies the thread for which to resume execution for one step-out (of
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp
index 3b297a0bd431f..440613b201c42 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp
@@ -582,6 +582,33 @@ llvm::json::Value toJSON(const SteppingGranularity &SG) {
llvm_unreachable("unhandled stepping granularity.");
}
+bool fromJSON(const llvm::json::Value &Params, StepInTarget &SIT,
+ llvm::json::Path P) {
+ json::ObjectMapper O(Params, P);
+ return O && O.map("id", SIT.id) && O.map("label", SIT.label) &&
+ O.mapOptional("line", SIT.line) &&
+ O.mapOptional("column", SIT.column) &&
+ O.mapOptional("endLine", SIT.endLine) &&
+ O.mapOptional("endColumn", SIT.endColumn);
+}
+
+llvm::json::Value toJSON(const StepInTarget &SIT) {
+ json::Object target;
+
+ target.insert({"id", SIT.id});
+ target.insert({"label", SIT.label});
+ if (SIT.line)
+ target.insert({"line", SIT.line});
+ if (SIT.column)
+ target.insert({"column", SIT.column});
+ if (SIT.endLine)
+ target.insert({"endLine", SIT.endLine});
+ if (SIT.endColumn)
+ target.insert({"endColumn", SIT.endColumn});
+
+ return target;
+}
+
bool fromJSON(const llvm::json::Value &Params, ValueFormat &VF,
llvm::json::Path P) {
json::ObjectMapper O(Params, P);
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h
index f5e21c96fe17f..edaca049a6ffa 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h
@@ -24,6 +24,7 @@
#include "llvm/ADT/DenseSet.h"
#include "llvm/Support/JSON.h"
#include <cstdint>
+#include <limits>
#include <optional>
#include <string>
@@ -414,6 +415,34 @@ bool fromJSON(const llvm::json::Value &, SteppingGranularity &,
llvm::json::Path);
llvm::json::Value toJSON(const SteppingGranularity &);
+/// A `StepInTarget` can be used in the `stepIn` request and determines into
+/// which single target the `stepIn` request should step.
+struct StepInTarget {
+ /// Unique identifier for a step-in target.
+ uint64_t id = std::numeric_limits<uint64_t>::max();
+
+ /// The name of the step-in target (shown in the UI).
+ std::string label;
+
+ /// The line of the step-in target.
+ std::optional<uint64_t> line;
+
+ /// Start position of the range covered by the step in target. It is measured
+ /// in UTF-16 code units and the client capability `columnsStartAt1`
+ /// determines whether it is 0- or 1-based.
+ std::optional<uint64_t> column;
+
+ /// The end line of the range covered by the step-in target.
+ std::optional<uint64_t> endLine;
+
+ /// End position of the range covered by the step in target. It is measured in
+ /// UTF-16 code units and the client capability `columnsStartAt1` determines
+ /// whether it is 0- or 1-based.
+ std::optional<uint64_t> endColumn;
+};
+bool fromJSON(const llvm::json::Value &, StepInTarget &, llvm::json::Path);
+llvm::json::Value toJSON(const StepInTarget &);
+
/// Provides formatting information for a value.
struct ValueFormat {
/// Display the value in hex.
diff --git a/lldb/unittests/DAP/ProtocolTypesTest.cpp b/lldb/unittests/DAP/ProtocolTypesTest.cpp
index 41703f4a071fb..5949abcb717d6 100644
--- a/lldb/unittests/DAP/ProtocolTypesTest.cpp
+++ b/lldb/unittests/DAP/ProtocolTypesTest.cpp
@@ -602,3 +602,23 @@ TEST(ProtocolTypesTest, DisassembledInstruction) {
EXPECT_EQ(instruction.presentationHint,
deserialized_instruction->presentationHint);
}
+
+TEST(ProtocolTypesTest, StepInTarget) {
+ StepInTarget target;
+ target.id = 230;
+ target.label = "the_function_name";
+ target.line = 2;
+ target.column = 320;
+ target.endLine = 32;
+ target.endColumn = 23;
+
+ llvm::Expected<StepInTarget> deserialized_target = roundtrip(target);
+ ASSERT_THAT_EXPECTED(deserialized_target, llvm::Succeeded());
+
+ EXPECT_EQ(target.id, deserialized_target->id);
+ EXPECT_EQ(target.label, deserialized_target->label);
+ EXPECT_EQ(target.line, deserialized_target->line);
+ EXPECT_EQ(target.column, deserialized_target->column);
+ EXPECT_EQ(target.endLine, deserialized_target->endLine);
+ EXPECT_EQ(target.endColumn, deserialized_target->endColumn);
+}
\ No newline at end of file
|
JDevlieghere
reviewed
Jun 2, 2025
ashgti
reviewed
Jun 2, 2025
ashgti
reviewed
Jun 3, 2025
ashgti
reviewed
Jun 3, 2025
lldb/test/API/tools/lldb-dap/stepInTargets/TestDAP_stepInTargets.py
Outdated
Show resolved
Hide resolved
ashgti
reviewed
Jun 3, 2025
JDevlieghere
approved these changes
Jun 3, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/18803 Here is the relevant piece of the build log for the reference |
da-viper
added a commit
to da-viper/llvm-project
that referenced
this pull request
Jun 4, 2025
fixes the failed tests on arch64. complements llvm#142439
DavidSpickett
pushed a commit
that referenced
this pull request
Jun 4, 2025
fixes the failed tests on aarch64. complements #142439
|
This broke the macOS bot: https://ci.swift.org/view/all/job/llvm.org/view/LLDB/job/as-lldb-cmake/27030/ |
|
I'm working on a fix |
|
I was wondering why llvm build bot did not catch it ( I assume there is a build bot for macos ? ) |
|
Correct, we don't have macOS premerge testing (yet?) |
|
Ah sorry old news.
|
|
#142831 should fix this on non-intel platforms |
JDevlieghere
added a commit
to JDevlieghere/llvm-project
that referenced
this pull request
Jun 5, 2025
…vm#142439)" This reverts commit 4b6c608.
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.
No description provided.