Skip to content

Commit 97355fa

Browse files
JDevlieghererorth
authored andcommitted
Revert "[lldb-dap] Use structured types for stepInTargets request (llvm#142439)" (llvm#142891)
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
1 parent a383894 commit 97355fa

File tree

12 files changed

+131
-252
lines changed

12 files changed

+131
-252
lines changed

lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,7 @@ def __init__(
153153
self.recv_thread = threading.Thread(target=self._read_packet_thread)
154154
self.process_event_body = None
155155
self.exit_status: Optional[int] = None
156-
self.initialize_body: dict[str, Any] = {}
156+
self.initialize_body = None
157157
self.progress_events: list[Event] = []
158158
self.reverse_requests = []
159159
self.sequence = 1
@@ -300,9 +300,6 @@ def _handle_recv_packet(self, packet: Optional[ProtocolMessage]) -> bool:
300300
elif event == "breakpoint":
301301
# Breakpoint events are sent when a breakpoint is resolved
302302
self._update_verified_breakpoints([body["breakpoint"]])
303-
elif event == "capabilities":
304-
# update the capabilities with new ones from the event.
305-
self.initialize_body.update(body["capabilities"])
306303

307304
elif packet_type == "response":
308305
if packet["command"] == "disconnect":
@@ -497,7 +494,7 @@ def get_initialize_value(self, key):
497494
"""
498495
if self.initialize_body and key in self.initialize_body:
499496
return self.initialize_body[key]
500-
raise ValueError(f"no value for key: {key} in {self.initialize_body}")
497+
return None
501498

502499
def get_threads(self):
503500
if self.threads is None:

lldb/test/API/tools/lldb-dap/stepInTargets/TestDAP_stepInTargets.py

Lines changed: 0 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -78,49 +78,3 @@ def test_basic(self):
7878
leaf_frame = self.dap_server.get_stackFrame()
7979
self.assertIsNotNone(leaf_frame, "expect a leaf frame")
8080
self.assertEqual(step_in_targets[1]["label"], leaf_frame["name"])
81-
82-
@skipIf(archs=no_match(["x86", "x86_64"]))
83-
def test_supported_capability_x86_arch(self):
84-
program = self.getBuildArtifact("a.out")
85-
self.build_and_launch(program)
86-
source = "main.cpp"
87-
bp_lines = [line_number(source, "// set breakpoint here")]
88-
breakpoint_ids = self.set_source_breakpoints(source, bp_lines)
89-
self.assertEqual(
90-
len(breakpoint_ids), len(bp_lines), "expect correct number of breakpoints"
91-
)
92-
is_supported = self.dap_server.get_initialize_value(
93-
"supportsStepInTargetsRequest"
94-
)
95-
96-
self.assertEqual(
97-
is_supported,
98-
True,
99-
f"expect capability `stepInTarget` is supported with architecture {self.getArchitecture()}",
100-
)
101-
# clear breakpoints.
102-
self.set_source_breakpoints(source, [])
103-
self.continue_to_exit()
104-
105-
@skipIf(archs=["x86", "x86_64"])
106-
def test_supported_capability_other_archs(self):
107-
program = self.getBuildArtifact("a.out")
108-
self.build_and_launch(program)
109-
source = "main.cpp"
110-
bp_lines = [line_number(source, "// set breakpoint here")]
111-
breakpoint_ids = self.set_source_breakpoints(source, bp_lines)
112-
self.assertEqual(
113-
len(breakpoint_ids), len(bp_lines), "expect correct number of breakpoints"
114-
)
115-
is_supported = self.dap_server.get_initialize_value(
116-
"supportsStepInTargetsRequest"
117-
)
118-
119-
self.assertEqual(
120-
is_supported,
121-
False,
122-
f"expect capability `stepInTarget` is not supported with architecture {self.getArchitecture()}",
123-
)
124-
# clear breakpoints.
125-
self.set_source_breakpoints(source, [])
126-
self.continue_to_exit()

lldb/tools/lldb-dap/EventHelper.cpp

Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -33,24 +33,6 @@ static void SendThreadExitedEvent(DAP &dap, lldb::tid_t tid) {
3333
dap.SendJSON(llvm::json::Value(std::move(event)));
3434
}
3535

36-
void SendTargetBasedCapabilities(DAP &dap) {
37-
if (!dap.target.IsValid())
38-
return;
39-
40-
// FIXME: stepInTargets request is only supported by the x86
41-
// architecture remove when `lldb::InstructionControlFlowKind` is
42-
// supported by other architectures
43-
const llvm::StringRef target_triple = dap.target.GetTriple();
44-
if (target_triple.starts_with("x86"))
45-
return;
46-
47-
protocol::Event event;
48-
event.event = "capabilities";
49-
event.body = llvm::json::Object{
50-
{"capabilities",
51-
llvm::json::Object{{"supportsStepInTargetsRequest", false}}}};
52-
dap.Send(event);
53-
}
5436
// "ProcessEvent": {
5537
// "allOf": [
5638
// { "$ref": "#/definitions/Event" },

lldb/tools/lldb-dap/EventHelper.h

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,6 @@ struct DAP;
1616

1717
enum LaunchMethod { Launch, Attach, AttachForSuspendedLaunch };
1818

19-
void SendTargetBasedCapabilities(DAP &dap);
20-
2119
void SendProcessEvent(DAP &dap, LaunchMethod launch_method);
2220

2321
void SendThreadStoppedEvent(DAP &dap);

lldb/tools/lldb-dap/Handler/ConfigurationDoneRequestHandler.cpp

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@ llvm::Error
3030
ConfigurationDoneRequestHandler::Run(const ConfigurationDoneArguments &) const {
3131
dap.configuration_done = true;
3232

33-
SendTargetBasedCapabilities(dap);
3433
// Ensure any command scripts did not leave us in an unexpected state.
3534
lldb::SBProcess process = dap.target.GetProcess();
3635
if (!process.IsValid() ||

lldb/tools/lldb-dap/Handler/RequestHandler.h

Lines changed: 1 addition & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -356,21 +356,7 @@ class StepInRequestHandler : public RequestHandler<protocol::StepInArguments,
356356
llvm::Error Run(const protocol::StepInArguments &args) const override;
357357
};
358358

359-
class StepInTargetsRequestHandler
360-
: public RequestHandler<
361-
protocol::StepInTargetsArguments,
362-
llvm::Expected<protocol::StepInTargetsResponseBody>> {
363-
public:
364-
using RequestHandler::RequestHandler;
365-
static llvm::StringLiteral GetCommand() { return "stepInTargets"; }
366-
FeatureSet GetSupportedFeatures() const override {
367-
return {protocol::eAdapterFeatureStepInTargetsRequest};
368-
}
369-
llvm::Expected<protocol::StepInTargetsResponseBody>
370-
Run(const protocol::StepInTargetsArguments &args) const override;
371-
};
372-
373-
class StepInTargetsRequestHandler2 : public LegacyRequestHandler {
359+
class StepInTargetsRequestHandler : public LegacyRequestHandler {
374360
public:
375361
using LegacyRequestHandler::LegacyRequestHandler;
376362
static llvm::StringLiteral GetCommand() { return "stepInTargets"; }

lldb/tools/lldb-dap/Handler/StepInTargetsRequestHandler.cpp

Lines changed: 128 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -7,85 +7,143 @@
77
//===----------------------------------------------------------------------===//
88

99
#include "DAP.h"
10-
#include "Protocol/ProtocolRequests.h"
10+
#include "EventHelper.h"
11+
#include "JSONUtils.h"
1112
#include "RequestHandler.h"
1213
#include "lldb/API/SBInstruction.h"
13-
#include "lldb/lldb-defines.h"
1414

15-
using namespace lldb_dap::protocol;
1615
namespace lldb_dap {
1716

18-
// This request retrieves the possible step-in targets for the specified stack
19-
// frame.
20-
// These targets can be used in the `stepIn` request.
21-
// Clients should only call this request if the corresponding capability
22-
// `supportsStepInTargetsRequest` is true.
23-
llvm::Expected<StepInTargetsResponseBody>
24-
StepInTargetsRequestHandler::Run(const StepInTargetsArguments &args) const {
25-
dap.step_in_targets.clear();
26-
const lldb::SBFrame frame = dap.GetLLDBFrame(args.frameId);
27-
if (!frame.IsValid())
28-
return llvm::make_error<DAPError>("Failed to get frame for input frameId.");
29-
30-
lldb::SBAddress pc_addr = frame.GetPCAddress();
31-
lldb::SBAddress line_end_addr =
32-
pc_addr.GetLineEntry().GetSameLineContiguousAddressRangeEnd(true);
33-
lldb::SBInstructionList insts = dap.target.ReadInstructions(
34-
pc_addr, line_end_addr, /*flavor_string=*/nullptr);
35-
36-
if (!insts.IsValid())
37-
return llvm::make_error<DAPError>("Failed to get instructions for frame.");
38-
39-
StepInTargetsResponseBody body;
40-
const size_t num_insts = insts.GetSize();
41-
for (size_t i = 0; i < num_insts; ++i) {
42-
lldb::SBInstruction inst = insts.GetInstructionAtIndex(i);
43-
if (!inst.IsValid())
44-
break;
45-
46-
const lldb::addr_t inst_addr = inst.GetAddress().GetLoadAddress(dap.target);
47-
if (inst_addr == LLDB_INVALID_ADDRESS)
48-
break;
49-
50-
// Note: currently only x86/x64 supports flow kind.
51-
const lldb::InstructionControlFlowKind flow_kind =
52-
inst.GetControlFlowKind(dap.target);
53-
54-
if (flow_kind == lldb::eInstructionControlFlowKindCall) {
17+
// "StepInTargetsRequest": {
18+
// "allOf": [ { "$ref": "#/definitions/Request" }, {
19+
// "type": "object",
20+
// "description": "This request retrieves the possible step-in targets for
21+
// the specified stack frame.\nThese targets can be used in the `stepIn`
22+
// request.\nClients should only call this request if the corresponding
23+
// capability `supportsStepInTargetsRequest` is true.", "properties": {
24+
// "command": {
25+
// "type": "string",
26+
// "enum": [ "stepInTargets" ]
27+
// },
28+
// "arguments": {
29+
// "$ref": "#/definitions/StepInTargetsArguments"
30+
// }
31+
// },
32+
// "required": [ "command", "arguments" ]
33+
// }]
34+
// },
35+
// "StepInTargetsArguments": {
36+
// "type": "object",
37+
// "description": "Arguments for `stepInTargets` request.",
38+
// "properties": {
39+
// "frameId": {
40+
// "type": "integer",
41+
// "description": "The stack frame for which to retrieve the possible
42+
// step-in targets."
43+
// }
44+
// },
45+
// "required": [ "frameId" ]
46+
// },
47+
// "StepInTargetsResponse": {
48+
// "allOf": [ { "$ref": "#/definitions/Response" }, {
49+
// "type": "object",
50+
// "description": "Response to `stepInTargets` request.",
51+
// "properties": {
52+
// "body": {
53+
// "type": "object",
54+
// "properties": {
55+
// "targets": {
56+
// "type": "array",
57+
// "items": {
58+
// "$ref": "#/definitions/StepInTarget"
59+
// },
60+
// "description": "The possible step-in targets of the specified
61+
// source location."
62+
// }
63+
// },
64+
// "required": [ "targets" ]
65+
// }
66+
// },
67+
// "required": [ "body" ]
68+
// }]
69+
// }
70+
void StepInTargetsRequestHandler::operator()(
71+
const llvm::json::Object &request) const {
72+
llvm::json::Object response;
73+
FillResponse(request, response);
74+
const auto *arguments = request.getObject("arguments");
5575

56-
const llvm::StringRef call_operand_name = inst.GetOperands(dap.target);
57-
lldb::addr_t call_target_addr = LLDB_INVALID_ADDRESS;
58-
if (call_operand_name.getAsInteger(0, call_target_addr))
59-
continue;
60-
61-
const lldb::SBAddress call_target_load_addr =
62-
dap.target.ResolveLoadAddress(call_target_addr);
63-
if (!call_target_load_addr.IsValid())
64-
continue;
65-
66-
// The existing ThreadPlanStepInRange only accept step in target
67-
// function with debug info.
68-
lldb::SBSymbolContext sc = dap.target.ResolveSymbolContextForAddress(
69-
call_target_load_addr, lldb::eSymbolContextFunction);
70-
71-
// The existing ThreadPlanStepInRange only accept step in target
72-
// function with debug info.
73-
llvm::StringRef step_in_target_name;
74-
if (sc.IsValid() && sc.GetFunction().IsValid())
75-
step_in_target_name = sc.GetFunction().GetDisplayName();
76-
77-
// Skip call sites if we fail to resolve its symbol name.
78-
if (step_in_target_name.empty())
79-
continue;
76+
dap.step_in_targets.clear();
77+
lldb::SBFrame frame = dap.GetLLDBFrame(*arguments);
78+
if (frame.IsValid()) {
79+
lldb::SBAddress pc_addr = frame.GetPCAddress();
80+
lldb::SBAddress line_end_addr =
81+
pc_addr.GetLineEntry().GetSameLineContiguousAddressRangeEnd(true);
82+
lldb::SBInstructionList insts = dap.target.ReadInstructions(
83+
pc_addr, line_end_addr, /*flavor_string=*/nullptr);
84+
85+
if (!insts.IsValid()) {
86+
response["success"] = false;
87+
response["message"] = "Failed to get instructions for frame.";
88+
dap.SendJSON(llvm::json::Value(std::move(response)));
89+
return;
90+
}
8091

81-
StepInTarget target;
82-
target.id = inst_addr;
83-
target.label = step_in_target_name;
84-
dap.step_in_targets.try_emplace(inst_addr, step_in_target_name);
85-
body.targets.emplace_back(std::move(target));
92+
llvm::json::Array step_in_targets;
93+
const auto num_insts = insts.GetSize();
94+
for (size_t i = 0; i < num_insts; ++i) {
95+
lldb::SBInstruction inst = insts.GetInstructionAtIndex(i);
96+
if (!inst.IsValid())
97+
break;
98+
99+
lldb::addr_t inst_addr = inst.GetAddress().GetLoadAddress(dap.target);
100+
101+
// Note: currently only x86/x64 supports flow kind.
102+
lldb::InstructionControlFlowKind flow_kind =
103+
inst.GetControlFlowKind(dap.target);
104+
if (flow_kind == lldb::eInstructionControlFlowKindCall) {
105+
// Use call site instruction address as id which is easy to debug.
106+
llvm::json::Object step_in_target;
107+
step_in_target["id"] = inst_addr;
108+
109+
llvm::StringRef call_operand_name = inst.GetOperands(dap.target);
110+
lldb::addr_t call_target_addr;
111+
if (call_operand_name.getAsInteger(0, call_target_addr))
112+
continue;
113+
114+
lldb::SBAddress call_target_load_addr =
115+
dap.target.ResolveLoadAddress(call_target_addr);
116+
if (!call_target_load_addr.IsValid())
117+
continue;
118+
119+
// The existing ThreadPlanStepInRange only accept step in target
120+
// function with debug info.
121+
lldb::SBSymbolContext sc = dap.target.ResolveSymbolContextForAddress(
122+
call_target_load_addr, lldb::eSymbolContextFunction);
123+
124+
// The existing ThreadPlanStepInRange only accept step in target
125+
// function with debug info.
126+
std::string step_in_target_name;
127+
if (sc.IsValid() && sc.GetFunction().IsValid())
128+
step_in_target_name = sc.GetFunction().GetDisplayName();
129+
130+
// Skip call sites if we fail to resolve its symbol name.
131+
if (step_in_target_name.empty())
132+
continue;
133+
134+
dap.step_in_targets.try_emplace(inst_addr, step_in_target_name);
135+
step_in_target.try_emplace("label", step_in_target_name);
136+
step_in_targets.emplace_back(std::move(step_in_target));
137+
}
86138
}
139+
llvm::json::Object body;
140+
body.try_emplace("targets", std::move(step_in_targets));
141+
response.try_emplace("body", std::move(body));
142+
} else {
143+
response["success"] = llvm::json::Value(false);
144+
response["message"] = "Failed to get frame for input frameId.";
87145
}
88-
return body;
146+
dap.SendJSON(llvm::json::Value(std::move(response)));
89147
}
90148

91149
} // namespace lldb_dap

lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -382,15 +382,6 @@ bool fromJSON(const llvm::json::Value &Params, StepInArguments &SIA,
382382
OM.mapOptional("granularity", SIA.granularity);
383383
}
384384

385-
bool fromJSON(const llvm::json::Value &Params, StepInTargetsArguments &SITA,
386-
llvm::json::Path P) {
387-
json::ObjectMapper OM(Params, P);
388-
return OM && OM.map("frameId", SITA.frameId);
389-
}
390-
llvm::json::Value toJSON(const StepInTargetsResponseBody &SITR) {
391-
return llvm::json::Object{{"targets", SITR.targets}};
392-
}
393-
394385
bool fromJSON(const llvm::json::Value &Params, StepOutArguments &SOA,
395386
llvm::json::Path P) {
396387
json::ObjectMapper OM(Params, P);

lldb/tools/lldb-dap/Protocol/ProtocolRequests.h

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -523,21 +523,6 @@ bool fromJSON(const llvm::json::Value &, StepInArguments &, llvm::json::Path);
523523
/// body field is required.
524524
using StepInResponse = VoidResponse;
525525

526-
/// Arguments for `stepInTargets` request.
527-
struct StepInTargetsArguments {
528-
/// The stack frame for which to retrieve the possible step-in targets.
529-
uint64_t frameId = LLDB_INVALID_FRAME_ID;
530-
};
531-
bool fromJSON(const llvm::json::Value &, StepInTargetsArguments &,
532-
llvm::json::Path);
533-
534-
/// Response to `stepInTargets` request.
535-
struct StepInTargetsResponseBody {
536-
/// The possible step-in targets of the specified source location.
537-
std::vector<StepInTarget> targets;
538-
};
539-
llvm::json::Value toJSON(const StepInTargetsResponseBody &);
540-
541526
/// Arguments for `stepOut` request.
542527
struct StepOutArguments {
543528
/// Specifies the thread for which to resume execution for one step-out (of

0 commit comments

Comments
 (0)