Revert "[lldb-dap] Use structured types for stepInTargets request (#142439)" #142891
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
This reverts commit c9e1c52.
…)" This reverts commit 159de36.
…vm#142439)" This reverts commit 4b6c608.
|
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesThis reverts commit 4b6c608 and follow-up commits 159de36 and c9e1c52 because this breaks TestDAP_stepInTargets.py on Darwin. https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/as-lldb-cmake Patch is 21.83 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/142891.diff 12 Files Affected:
diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
index f1e3cab06ccde..6b41aef2bb5b8 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
@@ -153,7 +153,7 @@ def __init__(
self.recv_thread = threading.Thread(target=self._read_packet_thread)
self.process_event_body = None
self.exit_status: Optional[int] = None
- self.initialize_body: dict[str, Any] = {}
+ self.initialize_body = None
self.progress_events: list[Event] = []
self.reverse_requests = []
self.sequence = 1
@@ -300,9 +300,6 @@ def _handle_recv_packet(self, packet: Optional[ProtocolMessage]) -> bool:
elif event == "breakpoint":
# Breakpoint events are sent when a breakpoint is resolved
self._update_verified_breakpoints([body["breakpoint"]])
- elif event == "capabilities":
- # update the capabilities with new ones from the event.
- self.initialize_body.update(body["capabilities"])
elif packet_type == "response":
if packet["command"] == "disconnect":
@@ -497,7 +494,7 @@ def get_initialize_value(self, key):
"""
if self.initialize_body and key in self.initialize_body:
return self.initialize_body[key]
- raise ValueError(f"no value for key: {key} in {self.initialize_body}")
+ return None
def get_threads(self):
if self.threads is None:
diff --git a/lldb/test/API/tools/lldb-dap/stepInTargets/TestDAP_stepInTargets.py b/lldb/test/API/tools/lldb-dap/stepInTargets/TestDAP_stepInTargets.py
index af698074f3479..07acfe07c9ffc 100644
--- a/lldb/test/API/tools/lldb-dap/stepInTargets/TestDAP_stepInTargets.py
+++ b/lldb/test/API/tools/lldb-dap/stepInTargets/TestDAP_stepInTargets.py
@@ -78,49 +78,3 @@ def test_basic(self):
leaf_frame = self.dap_server.get_stackFrame()
self.assertIsNotNone(leaf_frame, "expect a leaf frame")
self.assertEqual(step_in_targets[1]["label"], leaf_frame["name"])
-
- @skipIf(archs=no_match(["x86", "x86_64"]))
- def test_supported_capability_x86_arch(self):
- program = self.getBuildArtifact("a.out")
- self.build_and_launch(program)
- source = "main.cpp"
- bp_lines = [line_number(source, "// set breakpoint here")]
- breakpoint_ids = self.set_source_breakpoints(source, bp_lines)
- self.assertEqual(
- len(breakpoint_ids), len(bp_lines), "expect correct number of breakpoints"
- )
- is_supported = self.dap_server.get_initialize_value(
- "supportsStepInTargetsRequest"
- )
-
- self.assertEqual(
- is_supported,
- True,
- f"expect capability `stepInTarget` is supported with architecture {self.getArchitecture()}",
- )
- # clear breakpoints.
- self.set_source_breakpoints(source, [])
- self.continue_to_exit()
-
- @skipIf(archs=["x86", "x86_64"])
- def test_supported_capability_other_archs(self):
- program = self.getBuildArtifact("a.out")
- self.build_and_launch(program)
- source = "main.cpp"
- bp_lines = [line_number(source, "// set breakpoint here")]
- breakpoint_ids = self.set_source_breakpoints(source, bp_lines)
- self.assertEqual(
- len(breakpoint_ids), len(bp_lines), "expect correct number of breakpoints"
- )
- is_supported = self.dap_server.get_initialize_value(
- "supportsStepInTargetsRequest"
- )
-
- self.assertEqual(
- is_supported,
- False,
- f"expect capability `stepInTarget` is not supported with architecture {self.getArchitecture()}",
- )
- # clear breakpoints.
- self.set_source_breakpoints(source, [])
- self.continue_to_exit()
diff --git a/lldb/tools/lldb-dap/EventHelper.cpp b/lldb/tools/lldb-dap/EventHelper.cpp
index 33bc7c2cbef11..c698084836e2f 100644
--- a/lldb/tools/lldb-dap/EventHelper.cpp
+++ b/lldb/tools/lldb-dap/EventHelper.cpp
@@ -33,24 +33,6 @@ static void SendThreadExitedEvent(DAP &dap, lldb::tid_t tid) {
dap.SendJSON(llvm::json::Value(std::move(event)));
}
-void SendTargetBasedCapabilities(DAP &dap) {
- if (!dap.target.IsValid())
- return;
-
- // FIXME: stepInTargets request is only supported by the x86
- // architecture remove when `lldb::InstructionControlFlowKind` is
- // supported by other architectures
- const llvm::StringRef target_triple = dap.target.GetTriple();
- if (target_triple.starts_with("x86"))
- return;
-
- protocol::Event event;
- event.event = "capabilities";
- event.body = llvm::json::Object{
- {"capabilities",
- llvm::json::Object{{"supportsStepInTargetsRequest", false}}}};
- dap.Send(event);
-}
// "ProcessEvent": {
// "allOf": [
// { "$ref": "#/definitions/Event" },
diff --git a/lldb/tools/lldb-dap/EventHelper.h b/lldb/tools/lldb-dap/EventHelper.h
index e648afbf67e59..90b009c73089e 100644
--- a/lldb/tools/lldb-dap/EventHelper.h
+++ b/lldb/tools/lldb-dap/EventHelper.h
@@ -16,8 +16,6 @@ struct DAP;
enum LaunchMethod { Launch, Attach, AttachForSuspendedLaunch };
-void SendTargetBasedCapabilities(DAP &dap);
-
void SendProcessEvent(DAP &dap, LaunchMethod launch_method);
void SendThreadStoppedEvent(DAP &dap);
diff --git a/lldb/tools/lldb-dap/Handler/ConfigurationDoneRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/ConfigurationDoneRequestHandler.cpp
index 7cbbbd7982462..1281857ef4b60 100644
--- a/lldb/tools/lldb-dap/Handler/ConfigurationDoneRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/ConfigurationDoneRequestHandler.cpp
@@ -30,7 +30,6 @@ llvm::Error
ConfigurationDoneRequestHandler::Run(const ConfigurationDoneArguments &) const {
dap.configuration_done = true;
- SendTargetBasedCapabilities(dap);
// Ensure any command scripts did not leave us in an unexpected state.
lldb::SBProcess process = dap.target.GetProcess();
if (!process.IsValid() ||
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.h b/lldb/tools/lldb-dap/Handler/RequestHandler.h
index 559929ffb21e8..3a965bcc87a5e 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.h
+++ b/lldb/tools/lldb-dap/Handler/RequestHandler.h
@@ -356,21 +356,7 @@ class StepInRequestHandler : public RequestHandler<protocol::StepInArguments,
llvm::Error Run(const protocol::StepInArguments &args) const override;
};
-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 {
+class StepInTargetsRequestHandler : 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 9295b6ceae36d..9b99791599f82 100644
--- a/lldb/tools/lldb-dap/Handler/StepInTargetsRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/StepInTargetsRequestHandler.cpp
@@ -7,85 +7,143 @@
//===----------------------------------------------------------------------===//
#include "DAP.h"
-#include "Protocol/ProtocolRequests.h"
+#include "EventHelper.h"
+#include "JSONUtils.h"
#include "RequestHandler.h"
#include "lldb/API/SBInstruction.h"
-#include "lldb/lldb-defines.h"
-using namespace lldb_dap::protocol;
namespace lldb_dap {
-// 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();
- 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) {
+// "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");
- 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;
+ 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;
+ }
- 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::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));
+ }
}
+ 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.";
}
- return body;
+ dap.SendJSON(llvm::json::Value(std::move(response)));
}
} // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp
index a7f28fb566d3d..4160077d419e1 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp
@@ -382,15 +382,6 @@ 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 570a0b6d39682..7c774e50d6e56 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h
@@ -523,21 +523,6 @@ 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 33db5f0f4b89b..3b297a0bd431f 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp
@@ -582,28 +582,6 @@ llvm::json::Value toJSON(const SteppingGranularity &SG) {
llvm_unreachable("unhandled stepping granularity.");
}
-bool fromJSON(const json::Value &Params, StepInTarget &SIT, json::Path P) {
- json::ObjectMapper O(Params, P);
- return O && O.map("id", SIT.id) && O.map("label", SIT.label) &&
- O.map("line", SIT.line) && O.map("column", SIT.column) &&
- O.map("endLine", SIT.endLine) && O.map("endColumn", SIT.endColumn);
-}
-
-llvm::json::Value toJSON(const StepInTarget &SIT) {
- json::Object target{{"id", SIT.id}, {"label", SIT.label}};
-
- if (SIT.line != LLDB_INVALID_LINE_NUMBER)
- target.insert({"line", SIT.line});
- if (SIT.column != LLDB_INVALID_COLUMN_NUMBER)
- target.insert({"column", SIT.column});
- if (SIT.endLine != LLDB_INVALID_LINE_NUMBER)
- target.insert({"endLine", SIT.endLine});
- if (SIT.endLine != LLDB_INVALID_COLUMN_NUMBER)
- 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 a9f67c280c921..f5e21c96fe17f 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h
@@ -24,7 +24,6 @@
#include "llvm/ADT/DenseSet.h"
#include "llvm/Support/JSON.h"
#include <cstdint>
-#include <limits>
#include <optional>
#include <string>
@@ -415,34 +414,6 @@ 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.
- lldb::addr_t id = LLDB_INVALID_ADDRESS;
-
- /// The name of the step-in target (shown in the UI).
- std::string label;
-
- /// The line of the step-in target.
- uint32_t line = LLDB_INVALID_LINE_NUMBER;
-
- /// Start position of the range covered by the step in target. It is measured
- /// in UTF-16 code units and the cli...
[truncated]
|
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/162/builds/23885 Here is the relevant piece of the build log for the reference |
|
#142831 should fix the failing test on macOS. Should I revert this revert and add that to my other PR or should I rebase my changes on HEAD and we can take another stab at updating the stepInTargets request? |
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.
This reverts commit 4b6c608 and follow-up commits 159de36 and c9e1c52 because this breaks TestDAP_stepInTargets.py on Darwin.
https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/as-lldb-cmake