llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-llvm-support Author: Ely Ronnen (eronnen) <details> <summary>Changes</summary> - 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 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] `````````` </details> https://github.com/llvm/llvm-project/pull/137448 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
