Revert "[lldb-dap] Use structured types for stepInTargets request (#142439)" (#142891)

JDevlieghere · web-flow · commit d8ba707b0cbe · 2025-06-04T20:19:56.000-07:00
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
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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.h
diff --git a/lldb/unittests/DAP/ProtocolTypesTest.cpp b/lldb/unittests/DAP/ProtocolTypesTest.cpp
