Skip to content

Conversation

@eronnen
Copy link
Contributor

@eronnen eronnen commented Apr 26, 2025

  • Migrate set breakpoint requests to use typed RequestHandler
    • SetBreakpointsRequestHandler
    • SetDataBreakpointsRequestHandler
    • SetFunctionBreakpointsRequestHandler
    • SetInstructionBreakpointsRequestHandler
    • DataBreakpointInfoRequestHandler
  • Decouple JSON from lldb-dap Breakpoint classes

@eronnen eronnen changed the title Lldb dap migrate set breakpoint requests [lldb-dap] migrate set breakpoint requests Apr 26, 2025
@github-actions
Copy link

github-actions bot commented Apr 26, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@eronnen eronnen force-pushed the lldb-dap-migrate-set-breakpoint-requests branch 10 times, most recently from 19b3a1b to dc8388b Compare April 27, 2025 10:07
@eronnen eronnen marked this pull request as ready for review April 27, 2025 10:08
@eronnen eronnen requested a review from JDevlieghere as a code owner April 27, 2025 10:08
@eronnen
Copy link
Contributor Author

eronnen commented Apr 27, 2025

CC @ashgti

@llvmbot
Copy link
Member

llvmbot commented Apr 27, 2025

@llvm/pr-subscribers-lldb

@llvm/pr-subscribers-llvm-support

Author: Ely Ronnen (eronnen)

Changes
  • Migrate set breakpoint requests to use typed RequestHandler
    • SetBreakpointsRequestHandler
    • SetDataBreakpointsRequestHandler
    • SetFunctionBreakpointsRequestHandler
    • SetInstructionBreakpointsRequestHandler
    • DataBreakpointInfoRequestHandler

Patch is 96.22 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/137448.diff

29 Files Affected:

  • (modified) lldb/tools/lldb-dap/Breakpoint.cpp (+13-9)
  • (modified) lldb/tools/lldb-dap/Breakpoint.h (+4-2)
  • (modified) lldb/tools/lldb-dap/BreakpointBase.cpp (+5-7)
  • (modified) lldb/tools/lldb-dap/BreakpointBase.h (+5-3)
  • (modified) lldb/tools/lldb-dap/DAP.cpp (+8-3)
  • (modified) lldb/tools/lldb-dap/DAP.h (+1)
  • (modified) lldb/tools/lldb-dap/FunctionBreakpoint.cpp (+4-4)
  • (modified) lldb/tools/lldb-dap/FunctionBreakpoint.h (+2-1)
  • (modified) lldb/tools/lldb-dap/Handler/DataBreakpointInfoRequestHandler.cpp (+40-135)
  • (modified) lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp (+1-1)
  • (modified) lldb/tools/lldb-dap/Handler/RequestHandler.h (+35-16)
  • (modified) lldb/tools/lldb-dap/Handler/SetBreakpointsRequestHandler.cpp (+38-142)
  • (modified) lldb/tools/lldb-dap/Handler/SetDataBreakpointsRequestHandler.cpp (+17-81)
  • (modified) lldb/tools/lldb-dap/Handler/SetFunctionBreakpointsRequestHandler.cpp (+16-99)
  • (modified) lldb/tools/lldb-dap/Handler/SetInstructionBreakpointsRequestHandler.cpp (+16-207)
  • (modified) lldb/tools/lldb-dap/Handler/TestGetTargetBreakpointsRequestHandler.cpp (+1-1)
  • (modified) lldb/tools/lldb-dap/InstructionBreakpoint.cpp (+7-8)
  • (modified) lldb/tools/lldb-dap/InstructionBreakpoint.h (+3-1)
  • (modified) lldb/tools/lldb-dap/JSONUtils.cpp (+12-149)
  • (modified) lldb/tools/lldb-dap/JSONUtils.h (+3-64)
  • (modified) lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp (+70)
  • (modified) lldb/tools/lldb-dap/Protocol/ProtocolRequests.h (+147)
  • (modified) lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp (+127)
  • (modified) lldb/tools/lldb-dap/Protocol/ProtocolTypes.h (+181)
  • (modified) lldb/tools/lldb-dap/SourceBreakpoint.cpp (+6-7)
  • (modified) lldb/tools/lldb-dap/SourceBreakpoint.h (+2-1)
  • (modified) lldb/tools/lldb-dap/Watchpoint.cpp (+14-12)
  • (modified) lldb/tools/lldb-dap/Watchpoint.h (+4-2)
  • (modified) llvm/include/llvm/Support/JSON.h (+8)
diff --git a/lldb/tools/lldb-dap/Breakpoint.cpp b/lldb/tools/lldb-dap/Breakpoint.cpp
index 5679fd545d53f..26d633d1d172e 100644
--- a/lldb/tools/lldb-dap/Breakpoint.cpp
+++ b/lldb/tools/lldb-dap/Breakpoint.cpp
@@ -14,7 +14,6 @@
 #include "lldb/API/SBLineEntry.h"
 #include "lldb/API/SBMutex.h"
 #include "llvm/ADT/StringExtras.h"
-#include "llvm/Support/JSON.h"
 #include <cstddef>
 #include <cstdint>
 #include <mutex>
@@ -30,13 +29,16 @@ void Breakpoint::SetHitCondition() {
     m_bp.SetIgnoreCount(hitCount - 1);
 }
 
-void Breakpoint::CreateJsonObject(llvm::json::Object &object) {
+protocol::Breakpoint Breakpoint::ToProtocolBreakpoint() {
+  protocol::Breakpoint breakpoint;
+
   // Each breakpoint location is treated as a separate breakpoint for VS code.
   // They don't have the notion of a single breakpoint with multiple locations.
   if (!m_bp.IsValid())
-    return;
-  object.try_emplace("verified", m_bp.GetNumResolvedLocations() > 0);
-  object.try_emplace("id", m_bp.GetID());
+    return breakpoint;
+
+  breakpoint.verified = m_bp.GetNumResolvedLocations() > 0;
+  breakpoint.id = m_bp.GetID();
   // VS Code DAP doesn't currently allow one breakpoint to have multiple
   // locations so we just report the first one. If we report all locations
   // then the IDE starts showing the wrong line numbers and locations for
@@ -60,16 +62,18 @@ void Breakpoint::CreateJsonObject(llvm::json::Object &object) {
   if (bp_addr.IsValid()) {
     std::string formatted_addr =
         "0x" + llvm::utohexstr(bp_addr.GetLoadAddress(m_bp.GetTarget()));
-    object.try_emplace("instructionReference", formatted_addr);
+    breakpoint.instructionReference = formatted_addr;
     auto line_entry = bp_addr.GetLineEntry();
     const auto line = line_entry.GetLine();
     if (line != UINT32_MAX)
-      object.try_emplace("line", line);
+      breakpoint.line = line;
     const auto column = line_entry.GetColumn();
     if (column != 0)
-      object.try_emplace("column", column);
-    object.try_emplace("source", CreateSource(line_entry));
+      breakpoint.column = column;
+    breakpoint.source = CreateSource(line_entry);
   }
+
+  return breakpoint;
 }
 
 bool Breakpoint::MatchesName(const char *name) {
diff --git a/lldb/tools/lldb-dap/Breakpoint.h b/lldb/tools/lldb-dap/Breakpoint.h
index 580017125af44..c4f1fa291f181 100644
--- a/lldb/tools/lldb-dap/Breakpoint.h
+++ b/lldb/tools/lldb-dap/Breakpoint.h
@@ -17,14 +17,16 @@ namespace lldb_dap {
 
 class Breakpoint : public BreakpointBase {
 public:
-  Breakpoint(DAP &d, const llvm::json::Object &obj) : BreakpointBase(d, obj) {}
+  Breakpoint(DAP &d, const std::optional<std::string> &condition,
+             const std::optional<std::string> &hit_condition)
+      : BreakpointBase(d, condition, hit_condition) {}
   Breakpoint(DAP &d, lldb::SBBreakpoint bp) : BreakpointBase(d), m_bp(bp) {}
 
   lldb::break_id_t GetID() const { return m_bp.GetID(); }
 
   void SetCondition() override;
   void SetHitCondition() override;
-  void CreateJsonObject(llvm::json::Object &object) override;
+  protocol::Breakpoint ToProtocolBreakpoint() override;
 
   bool MatchesName(const char *name);
   void SetBreakpoint();
diff --git a/lldb/tools/lldb-dap/BreakpointBase.cpp b/lldb/tools/lldb-dap/BreakpointBase.cpp
index 331ce8efee9bc..1a4da92e44d31 100644
--- a/lldb/tools/lldb-dap/BreakpointBase.cpp
+++ b/lldb/tools/lldb-dap/BreakpointBase.cpp
@@ -7,16 +7,14 @@
 //===----------------------------------------------------------------------===//
 
 #include "BreakpointBase.h"
-#include "JSONUtils.h"
-#include "llvm/ADT/StringRef.h"
 
 using namespace lldb_dap;
 
-BreakpointBase::BreakpointBase(DAP &d, const llvm::json::Object &obj)
-    : m_dap(d),
-      m_condition(std::string(GetString(obj, "condition").value_or(""))),
-      m_hit_condition(
-          std::string(GetString(obj, "hitCondition").value_or(""))) {}
+BreakpointBase::BreakpointBase(DAP &d,
+                               const std::optional<std::string> &condition,
+                               const std::optional<std::string> &hit_condition)
+    : m_dap(d), m_condition(condition.value_or("")),
+      m_hit_condition(hit_condition.value_or("")) {}
 
 void BreakpointBase::UpdateBreakpoint(const BreakpointBase &request_bp) {
   if (m_condition != request_bp.m_condition) {
diff --git a/lldb/tools/lldb-dap/BreakpointBase.h b/lldb/tools/lldb-dap/BreakpointBase.h
index 4c13326624831..e9cfc112675c3 100644
--- a/lldb/tools/lldb-dap/BreakpointBase.h
+++ b/lldb/tools/lldb-dap/BreakpointBase.h
@@ -10,7 +10,8 @@
 #define LLDB_TOOLS_LLDB_DAP_BREAKPOINTBASE_H
 
 #include "DAPForward.h"
-#include "llvm/ADT/StringRef.h"
+#include "Protocol/ProtocolTypes.h"
+#include <optional>
 #include <string>
 
 namespace lldb_dap {
@@ -18,12 +19,13 @@ namespace lldb_dap {
 class BreakpointBase {
 public:
   explicit BreakpointBase(DAP &d) : m_dap(d) {}
-  BreakpointBase(DAP &d, const llvm::json::Object &obj);
+  BreakpointBase(DAP &d, const std::optional<std::string> &condition,
+                 const std::optional<std::string> &hit_condition);
   virtual ~BreakpointBase() = default;
 
   virtual void SetCondition() = 0;
   virtual void SetHitCondition() = 0;
-  virtual void CreateJsonObject(llvm::json::Object &object) = 0;
+  virtual protocol::Breakpoint ToProtocolBreakpoint() = 0;
 
   void UpdateBreakpoint(const BreakpointBase &request_bp);
 
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index 55d49667b6398..50e5fd9dd6147 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -45,6 +45,7 @@
 #include <chrono>
 #include <condition_variable>
 #include <cstdarg>
+#include <cstdint>
 #include <cstdio>
 #include <fstream>
 #include <future>
@@ -514,9 +515,7 @@ lldb::SBThread DAP::GetLLDBThread(const llvm::json::Object &arguments) {
   return target.GetProcess().GetThreadByID(tid);
 }
 
-lldb::SBFrame DAP::GetLLDBFrame(const llvm::json::Object &arguments) {
-  const uint64_t frame_id =
-      GetInteger<uint64_t>(arguments, "frameId").value_or(UINT64_MAX);
+lldb::SBFrame DAP::GetLLDBFrame(uint64_t frame_id) {
   lldb::SBProcess process = target.GetProcess();
   // Upper 32 bits is the thread index ID
   lldb::SBThread thread =
@@ -525,6 +524,12 @@ lldb::SBFrame DAP::GetLLDBFrame(const llvm::json::Object &arguments) {
   return thread.GetFrameAtIndex(GetLLDBFrameID(frame_id));
 }
 
+lldb::SBFrame DAP::GetLLDBFrame(const llvm::json::Object &arguments) {
+  const auto frame_id =
+      GetInteger<uint64_t>(arguments, "frameId").value_or(UINT64_MAX);
+  return GetLLDBFrame(frame_id);
+}
+
 llvm::json::Value DAP::CreateTopLevelScopes() {
   llvm::json::Array scopes;
   scopes.emplace_back(
diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index 88eedb0860cf1..d785f071414db 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/lldb/tools/lldb-dap/DAP.h
@@ -275,6 +275,7 @@ struct DAP {
   lldb::SBThread GetLLDBThread(lldb::tid_t id);
   lldb::SBThread GetLLDBThread(const llvm::json::Object &arguments);
 
+  lldb::SBFrame GetLLDBFrame(uint64_t frame_id);
   lldb::SBFrame GetLLDBFrame(const llvm::json::Object &arguments);
 
   llvm::json::Value CreateTopLevelScopes();
diff --git a/lldb/tools/lldb-dap/FunctionBreakpoint.cpp b/lldb/tools/lldb-dap/FunctionBreakpoint.cpp
index d87723f7557bd..1ea9cddb9f689 100644
--- a/lldb/tools/lldb-dap/FunctionBreakpoint.cpp
+++ b/lldb/tools/lldb-dap/FunctionBreakpoint.cpp
@@ -8,15 +8,15 @@
 
 #include "FunctionBreakpoint.h"
 #include "DAP.h"
-#include "JSONUtils.h"
 #include "lldb/API/SBMutex.h"
 #include <mutex>
 
 namespace lldb_dap {
 
-FunctionBreakpoint::FunctionBreakpoint(DAP &d, const llvm::json::Object &obj)
-    : Breakpoint(d, obj),
-      m_function_name(std::string(GetString(obj, "name").value_or(""))) {}
+FunctionBreakpoint::FunctionBreakpoint(
+    DAP &d, const protocol::FunctionBreakpoint &breakpoint)
+    : Breakpoint(d, breakpoint.condition, breakpoint.hitCondition),
+      m_function_name(breakpoint.name) {}
 
 void FunctionBreakpoint::SetBreakpoint() {
   lldb::SBMutex lock = m_dap.GetAPIMutex();
diff --git a/lldb/tools/lldb-dap/FunctionBreakpoint.h b/lldb/tools/lldb-dap/FunctionBreakpoint.h
index 7100360cd7ec1..76fbdb3e886a4 100644
--- a/lldb/tools/lldb-dap/FunctionBreakpoint.h
+++ b/lldb/tools/lldb-dap/FunctionBreakpoint.h
@@ -11,12 +11,13 @@
 
 #include "Breakpoint.h"
 #include "DAPForward.h"
+#include "Protocol/ProtocolTypes.h"
 
 namespace lldb_dap {
 
 class FunctionBreakpoint : public Breakpoint {
 public:
-  FunctionBreakpoint(DAP &dap, const llvm::json::Object &obj);
+  FunctionBreakpoint(DAP &dap, const protocol::FunctionBreakpoint &breakpoint);
 
   /// Set this breakpoint in LLDB as a new breakpoint.
   void SetBreakpoint();
diff --git a/lldb/tools/lldb-dap/Handler/DataBreakpointInfoRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/DataBreakpointInfoRequestHandler.cpp
index 4d920f8556254..8cb25d0603449 100644
--- a/lldb/tools/lldb-dap/Handler/DataBreakpointInfoRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/DataBreakpointInfoRequestHandler.cpp
@@ -8,144 +8,50 @@
 
 #include "DAP.h"
 #include "EventHelper.h"
-#include "JSONUtils.h"
+#include "Protocol/ProtocolTypes.h"
 #include "RequestHandler.h"
 #include "lldb/API/SBMemoryRegionInfo.h"
 #include "llvm/ADT/StringExtras.h"
+#include <optional>
 
 namespace lldb_dap {
 
-// "DataBreakpointInfoRequest": {
-//   "allOf": [ { "$ref": "#/definitions/Request" }, {
-//     "type": "object",
-//     "description": "Obtains information on a possible data breakpoint that
-//     could be set on an expression or variable.\nClients should only call this
-//     request if the corresponding capability `supportsDataBreakpoints` is
-//     true.", "properties": {
-//       "command": {
-//         "type": "string",
-//         "enum": [ "dataBreakpointInfo" ]
-//       },
-//       "arguments": {
-//         "$ref": "#/definitions/DataBreakpointInfoArguments"
-//       }
-//     },
-//     "required": [ "command", "arguments"  ]
-//   }]
-// },
-// "DataBreakpointInfoArguments": {
-//   "type": "object",
-//   "description": "Arguments for `dataBreakpointInfo` request.",
-//   "properties": {
-//     "variablesReference": {
-//       "type": "integer",
-//       "description": "Reference to the variable container if the data
-//       breakpoint is requested for a child of the container. The
-//       `variablesReference` must have been obtained in the current suspended
-//       state. See 'Lifetime of Object References' in the Overview section for
-//       details."
-//     },
-//     "name": {
-//       "type": "string",
-//       "description": "The name of the variable's child to obtain data
-//       breakpoint information for.\nIf `variablesReference` isn't specified,
-//       this can be an expression."
-//     },
-//     "frameId": {
-//       "type": "integer",
-//       "description": "When `name` is an expression, evaluate it in the scope
-//       of this stack frame. If not specified, the expression is evaluated in
-//       the global scope. When `variablesReference` is specified, this property
-//       has no effect."
-//     }
-//   },
-//   "required": [ "name" ]
-// },
-// "DataBreakpointInfoResponse": {
-//   "allOf": [ { "$ref": "#/definitions/Response" }, {
-//     "type": "object",
-//     "description": "Response to `dataBreakpointInfo` request.",
-//     "properties": {
-//       "body": {
-//         "type": "object",
-//         "properties": {
-//           "dataId": {
-//             "type": [ "string", "null" ],
-//             "description": "An identifier for the data on which a data
-//             breakpoint can be registered with the `setDataBreakpoints`
-//             request or null if no data breakpoint is available. If a
-//             `variablesReference` or `frameId` is passed, the `dataId` is
-//             valid in the current suspended state, otherwise it's valid
-//             indefinitely. See 'Lifetime of Object References' in the Overview
-//             section for details. Breakpoints set using the `dataId` in the
-//             `setDataBreakpoints` request may outlive the lifetime of the
-//             associated `dataId`."
-//           },
-//           "description": {
-//             "type": "string",
-//             "description": "UI string that describes on what data the
-//             breakpoint is set on or why a data breakpoint is not available."
-//           },
-//           "accessTypes": {
-//             "type": "array",
-//             "items": {
-//               "$ref": "#/definitions/DataBreakpointAccessType"
-//             },
-//             "description": "Attribute lists the available access types for a
-//             potential data breakpoint. A UI client could surface this
-//             information."
-//           },
-//           "canPersist": {
-//             "type": "boolean",
-//             "description": "Attribute indicates that a potential data
-//             breakpoint could be persisted across sessions."
-//           }
-//         },
-//         "required": [ "dataId", "description" ]
-//       }
-//     },
-//     "required": [ "body" ]
-//   }]
-// }
-void DataBreakpointInfoRequestHandler::operator()(
-    const llvm::json::Object &request) const {
-  llvm::json::Object response;
-  FillResponse(request, response);
-  llvm::json::Object body;
-  lldb::SBError error;
-  llvm::json::Array accessTypes{"read", "write", "readWrite"};
-  const auto *arguments = request.getObject("arguments");
-  const auto variablesReference =
-      GetInteger<uint64_t>(arguments, "variablesReference").value_or(0);
-  llvm::StringRef name = GetString(arguments, "name").value_or("");
-  lldb::SBFrame frame = dap.GetLLDBFrame(*arguments);
-  lldb::SBValue variable = dap.variables.FindVariable(variablesReference, name);
+/// Obtains information on a possible data breakpoint that could be set on an
+/// expression or variable. Clients should only call this request if the
+/// corresponding capability supportsDataBreakpoints is true.
+llvm::Expected<protocol::DataBreakpointInfoResponseBody>
+DataBreakpointInfoRequestHandler::Run(
+    const protocol::DataBreakpointInfoArguments &args) const {
+  protocol::DataBreakpointInfoResponseBody response;
+  lldb::SBFrame frame = dap.GetLLDBFrame(args.frameId.value_or(UINT64_MAX));
+  lldb::SBValue variable = dap.variables.FindVariable(
+      args.variablesReference.value_or(0), args.name);
   std::string addr, size;
 
+  bool is_data_ok = true;
   if (variable.IsValid()) {
     lldb::addr_t load_addr = variable.GetLoadAddress();
     size_t byte_size = variable.GetByteSize();
     if (load_addr == LLDB_INVALID_ADDRESS) {
-      body.try_emplace("dataId", nullptr);
-      body.try_emplace("description",
-                       "does not exist in memory, its location is " +
-                           std::string(variable.GetLocation()));
+      is_data_ok = false;
+      response.description = "does not exist in memory, its location is " +
+                             std::string(variable.GetLocation());
     } else if (byte_size == 0) {
-      body.try_emplace("dataId", nullptr);
-      body.try_emplace("description", "variable size is 0");
+      is_data_ok = false;
+      response.description = "variable size is 0";
     } else {
       addr = llvm::utohexstr(load_addr);
       size = llvm::utostr(byte_size);
     }
-  } else if (variablesReference == 0 && frame.IsValid()) {
-    lldb::SBValue value = frame.EvaluateExpression(name.data());
+  } else if (args.variablesReference.value_or(0) == 0 && frame.IsValid()) {
+    lldb::SBValue value = frame.EvaluateExpression(args.name.c_str());
     if (value.GetError().Fail()) {
       lldb::SBError error = value.GetError();
       const char *error_cstr = error.GetCString();
-      body.try_emplace("dataId", nullptr);
-      body.try_emplace("description", error_cstr && error_cstr[0]
-                                          ? std::string(error_cstr)
-                                          : "evaluation failed");
+      is_data_ok = false;
+      response.description = error_cstr && error_cstr[0]
+                                 ? std::string(error_cstr)
+                                 : "evaluation failed";
     } else {
       uint64_t load_addr = value.GetValueAsUnsigned();
       lldb::SBData data = value.GetPointeeData();
@@ -159,32 +65,31 @@ void DataBreakpointInfoRequestHandler::operator()(
         // request if SBProcess::GetMemoryRegionInfo returns error.
         if (err.Success()) {
           if (!(region.IsReadable() || region.IsWritable())) {
-            body.try_emplace("dataId", nullptr);
-            body.try_emplace("description",
-                             "memory region for address " + addr +
-                                 " has no read or write permissions");
+            is_data_ok = false;
+            response.description = "memory region for address " + addr +
+                                   " has no read or write permissions";
           }
         }
       } else {
-        body.try_emplace("dataId", nullptr);
-        body.try_emplace("description",
-                         "unable to get byte size for expression: " +
-                             name.str());
+        is_data_ok = false;
+        response.description =
+            "unable to get byte size for expression: " + args.name;
       }
     }
   } else {
-    body.try_emplace("dataId", nullptr);
-    body.try_emplace("description", "variable not found: " + name.str());
+    is_data_ok = false;
+    response.description = "variable not found: " + args.name;
   }
 
-  if (!body.getObject("dataId")) {
-    body.try_emplace("dataId", addr + "/" + size);
-    body.try_emplace("accessTypes", std::move(accessTypes));
-    body.try_emplace("description",
-                     size + " bytes at " + addr + " " + name.str());
+  if (is_data_ok) {
+    response.dataId = addr + "/" + size;
+    response.accessTypes = {protocol::eDataBreakpointAccessTypeRead,
+                            protocol::eDataBreakpointAccessTypeWrite,
+                            protocol::eDataBreakpointAccessTypeReadWrite};
+    response.description = size + " bytes at " + addr + " " + args.name;
   }
-  response.try_emplace("body", std::move(body));
-  dap.SendJSON(llvm::json::Value(std::move(response)));
+
+  return response;
 }
 
 } // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp
index 7d8ac676ba935..9614d2876b2d4 100644
--- a/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp
@@ -212,7 +212,7 @@ static void EventThreadFunction(DAP &dap) {
             // avoids sending paths that should be source mapped. Note that
             // CreateBreakpoint doesn't apply source mapping and certain
             // implementation ignore the source part of this event anyway.
-            llvm::json::Value source_bp = CreateBreakpoint(&bp);
+            llvm::json::Value source_bp = bp.ToProtocolBreakpoint();
             source_bp.getAsObject()->erase("source");
 
             llvm::json::Object body;
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.h b/lldb/tools/lldb-dap/Handler/RequestHandler.h
index fa3d76ed4a125..279e70dbbf555 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.h
+++ b/lldb/tools/lldb-dap/Handler/RequestHandler.h
@@ -15,7 +15,6 @@
 #include "Protocol/ProtocolBase.h"
 #include "Protocol/ProtocolRequests.h"
 #include "Protocol/ProtocolTypes.h"
-#include "lldb/API/SBError.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
@@ -339,15 +338,19 @@ class StepOutRequestHandler : public RequestHandler<protocol::StepOutArguments,
   llvm::Error Run(const protocol::StepOutArguments &args) const override;
 };
 
-class SetBreakpointsRequestHandler : public LegacyRequestHandler {
+class SetBreakpointsRequestHandler
+    : public RequestHandler<
+          protocol::SetBreakpointsArguments,
+          llvm::Expected<protocol::SetBreakpointsResponseBody>> {
 public:
-  using LegacyRequestHandler::LegacyRequestHandler;
+  using RequestHandler::RequestHandler;
   static llvm::StringLiteral GetCommand() { return "setBreakpoints"; }
   FeatureSet GetSupportedFeatures() const override {
     return {protocol::eAdapterFeatureConditionalBreakpoints...
[truncated]

@JDevlieghere JDevlieghere requested a review from ashgti April 27, 2025 18:10
O.map("sourceReference", S.sourceReference);
}

static llvm::json::Value ToString(PresentationHint hint) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toJSON?

Then on line 67 you don't need to add ToString, it would be handled by the existing overloads.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

Comment on lines 330 to 338
/// A machine-readable explanation of why a breakpoint may not be verified.
enum class Reason : unsigned {
/// Indicates a breakpoint might be verified in the future, but
/// the adapter cannot verify it in the current state.
eBreakpointReasonPending,
/// Indicates a breakpoint was not able to be verified, and the
/// adapter does not believe it can be verified without intervention.
eBreakpointReasonFailed,
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style nit, in lldb enums are usually not nested like this, how about moving this out of the Breakpoint struct? You could use BreakpointReason as well, thats already repeated in the enum names.

Copy link
Contributor Author

@eronnen eronnen Apr 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed


/// If true, the breakpoint could be set (but not necessarily at the desired
/// location).
bool verified;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we default this to false?

In the Breakpoint.cpp if the SBBreakpiont.IsValid() returns false we return the Breakpoint without initializing this value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

/// request
struct SourceBreakpoint {
/// The source line of the breakpoint or logpoint.
uint32_t line;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we default this to LLDB_INVALID_LINE_NUMBER?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

@eronnen eronnen requested a review from ashgti April 27, 2025 23:25
add all breakpoint requests JSON types

decouple JSON from DAP Breakpoint classes

forgor exception breakpoint response

migrating breakpoint requests

migrate breakpoints requests handler implementations

remove CreateBreakpoint

data breakpoint

requests serialization

fix

GetFrame

fix

data breakpoint

return exception breakpoints list

restore exception breakpoints

add default values

extract breakpoint reason type
@eronnen eronnen force-pushed the lldb-dap-migrate-set-breakpoint-requests branch from 8a23802 to a436d40 Compare May 9, 2025 19:21
@eronnen eronnen merged commit 8630c22 into llvm:main May 9, 2025
9 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants