From 66ffdc4cb80191dc87f137efbd22685fe30d3a8e Mon Sep 17 00:00:00 2001 From: John Harrison Date: Thu, 10 Jul 2025 11:38:47 -0700 Subject: [PATCH] [lldb-dap] Updating protocol memory references to `lldb::addr_t`. This updates all the existing memory reference fields to use `lldb::addr_t` directly. A few places were using `std::string` and decoding the value in the request handler and other places had unique ways of parsing addresses. This unifies all of these references with the `DecodeMemoryReference` helper in JSONUtils.h. Additionally, for the types I updated, I tried to simplify the POD types some and moved default values out of RequestHandlers and into the protocol POD types. --- .../test/tools/lldb-dap/dap_server.py | 10 ++- .../test/tools/lldb-dap/lldbdap_testcase.py | 2 +- .../lldb-dap/variables/TestDAP_variables.py | 6 +- .../Handler/DisassembleRequestHandler.cpp | 15 +--- .../Handler/ReadMemoryRequestHandler.cpp | 2 +- .../Handler/SetVariableRequestHandler.cpp | 5 +- .../Handler/WriteMemoryRequestHandler.cpp | 7 +- .../lldb-dap/Protocol/ProtocolRequests.cpp | 74 ++++++------------- .../lldb-dap/Protocol/ProtocolRequests.h | 35 +++++---- .../tools/lldb-dap/Protocol/ProtocolTypes.cpp | 17 +---- lldb/unittests/DAP/ProtocolTypesTest.cpp | 24 +++--- 11 files changed, 76 insertions(+), 121 deletions(-) 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 68f58bf1349a7..964d685e61161 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 @@ -831,13 +831,17 @@ def request_readMemory(self, memoryReference, offset, count): } return self.send_recv(command_dict) - def request_writeMemory(self, memoryReference, data, offset=0, allowPartial=True): + def request_writeMemory(self, memoryReference, data, offset=0, allowPartial=False): args_dict = { "memoryReference": memoryReference, - "offset": offset, - "allowPartial": allowPartial, "data": data, } + + if offset: + args_dict["offset"] = offset + if allowPartial: + args_dict["allowPartial"] = allowPartial + command_dict = { "command": "writeMemory", "type": "request", diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py index 00b01d4bdb1c5..d823126e3e2fd 100644 --- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py +++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py @@ -508,7 +508,7 @@ def getBuiltinDebugServerTool(self): self.assertIsNotNone(server_tool, "debugserver not found.") return server_tool - def writeMemory(self, memoryReference, data=None, offset=None, allowPartial=None): + def writeMemory(self, memoryReference, data=None, offset=0, allowPartial=False): # This function accepts data in decimal and hexadecimal format, # converts it to a Base64 string, and send it to the DAP, # which expects Base64 encoded data. diff --git a/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py b/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py index 69d4eab0b6fcd..a3a4bdaaf40a6 100644 --- a/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py +++ b/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py @@ -141,8 +141,7 @@ def do_test_scopes_variables_setVariable_evaluate( self, enableAutoVariableSummaries: bool ): """ - Tests the "scopes", "variables", "setVariable", and "evaluate" - packets. + Tests the "scopes", "variables", "setVariable", and "evaluate" packets. """ program = self.getBuildArtifact("a.out") self.build_and_launch( @@ -304,7 +303,6 @@ def do_test_scopes_variables_setVariable_evaluate( verify_response = { "type": "int", "value": str(variable_value), - "variablesReference": 0, } for key, value in verify_response.items(): self.assertEqual(value, response["body"][key]) @@ -723,7 +721,7 @@ def test_indexedVariables_with_raw_child_for_synthetics(self): self.do_test_indexedVariables(enableSyntheticChildDebugging=True) @skipIfWindows - @skipIfAsan # FIXME this fails with a non-asan issue on green dragon. + @skipIfAsan # FIXME this fails with a non-asan issue on green dragon. def test_registers(self): """ Test that registers whose byte size is the size of a pointer on diff --git a/lldb/tools/lldb-dap/Handler/DisassembleRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/DisassembleRequestHandler.cpp index f66c87fa9893d..9542f091b055e 100644 --- a/lldb/tools/lldb-dap/Handler/DisassembleRequestHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/DisassembleRequestHandler.cpp @@ -182,14 +182,7 @@ static DisassembledInstruction ConvertSBInstructionToDisassembledInstruction( /// `supportsDisassembleRequest` is true. llvm::Expected DisassembleRequestHandler::Run(const DisassembleArguments &args) const { - std::optional addr_opt = - DecodeMemoryReference(args.memoryReference); - if (!addr_opt.has_value()) - return llvm::make_error("Malformed memory reference: " + - args.memoryReference); - - lldb::addr_t addr_ptr = *addr_opt; - addr_ptr += args.offset.value_or(0); + const lldb::addr_t addr_ptr = args.memoryReference + args.offset; lldb::SBAddress addr(addr_ptr, dap.target); if (!addr.IsValid()) return llvm::make_error( @@ -197,7 +190,7 @@ DisassembleRequestHandler::Run(const DisassembleArguments &args) const { // Offset (in instructions) to be applied after the byte offset (if any) // before disassembling. Can be negative. - int64_t instruction_offset = args.instructionOffset.value_or(0); + const int64_t instruction_offset = args.instructionOffset; // Calculate a sufficient address to start disassembling from. lldb::SBAddress disassemble_start_addr = @@ -212,8 +205,8 @@ DisassembleRequestHandler::Run(const DisassembleArguments &args) const { return llvm::make_error( "Unexpected error while disassembling instructions."); - // Conver the found instructions to the DAP format. - const bool resolve_symbols = args.resolveSymbols.value_or(false); + // Convert the found instructions to the DAP format. + const bool resolve_symbols = args.resolveSymbols; std::vector instructions; size_t original_address_index = args.instructionCount; for (size_t i = 0; i < insts.GetSize(); ++i) { diff --git a/lldb/tools/lldb-dap/Handler/ReadMemoryRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/ReadMemoryRequestHandler.cpp index 7065b6a24b554..374dc4516aa2d 100644 --- a/lldb/tools/lldb-dap/Handler/ReadMemoryRequestHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/ReadMemoryRequestHandler.cpp @@ -37,7 +37,7 @@ ReadMemoryRequestHandler::Run(const protocol::ReadMemoryArguments &args) const { const size_t memory_count = dap.target.GetProcess().ReadMemory( raw_address, buffer.data(), buffer.size(), error); - response.address = "0x" + llvm::utohexstr(raw_address); + response.address = raw_address; // reading memory may fail for multiple reasons. memory not readable, // reading out of memory range and gaps in memory. return from diff --git a/lldb/tools/lldb-dap/Handler/SetVariableRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/SetVariableRequestHandler.cpp index 5b8ff563968f0..d07c0d6c9afa8 100644 --- a/lldb/tools/lldb-dap/Handler/SetVariableRequestHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/SetVariableRequestHandler.cpp @@ -68,14 +68,11 @@ SetVariableRequestHandler::Run(const SetVariableArguments &args) const { body.indexedVariables = variable.GetNumChildren(); else body.namedVariables = variable.GetNumChildren(); - - } else { - body.variablesReference = 0; } if (const lldb::addr_t addr = variable.GetLoadAddress(); addr != LLDB_INVALID_ADDRESS) - body.memoryReference = EncodeMemoryReference(addr); + body.memoryReference = addr; if (ValuePointsToCode(variable)) body.valueLocationReference = new_var_ref; diff --git a/lldb/tools/lldb-dap/Handler/WriteMemoryRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/WriteMemoryRequestHandler.cpp index 7bd1c9f91ff47..313f59dceab24 100644 --- a/lldb/tools/lldb-dap/Handler/WriteMemoryRequestHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/WriteMemoryRequestHandler.cpp @@ -22,8 +22,7 @@ namespace lldb_dap { llvm::Expected WriteMemoryRequestHandler::Run( const protocol::WriteMemoryArguments &args) const { - const lldb::addr_t address = args.memoryReference + args.offset.value_or(0); - ; + const lldb::addr_t address = args.memoryReference + args.offset; lldb::SBProcess process = dap.target.GetProcess(); if (!lldb::SBDebugger::StateIsStoppedState(process.GetState())) @@ -54,7 +53,7 @@ WriteMemoryRequestHandler::Run( // If 'allowPartial' is false or missing, a debug adapter should attempt to // verify the region is writable before writing, and fail the response if it // is not. - if (!args.allowPartial.value_or(false)) { + if (!args.allowPartial) { // Start checking from the initial write address. lldb::addr_t start_address = address; // Compute the end of the write range. @@ -74,7 +73,7 @@ WriteMemoryRequestHandler::Run( "Memory 0x" + llvm::utohexstr(args.memoryReference) + " region is not writable"); } - // If the current region covers the full requested range, stop futher + // If the current region covers the full requested range, stop further // iterations. if (end_address <= region_info.GetRegionEnd()) { break; diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp index e840677cdebd5..cc29c1c8c9620 100644 --- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp +++ b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp @@ -8,6 +8,7 @@ #include "Protocol/ProtocolRequests.h" #include "JSONUtils.h" +#include "lldb/lldb-defines.h" #include "llvm/ADT/DenseMap.h" #include "llvm/ADT/StringMap.h" #include "llvm/ADT/StringRef.h" @@ -311,26 +312,24 @@ bool fromJSON(const json::Value &Params, SetVariableArguments &SVA, json::Value toJSON(const SetVariableResponseBody &SVR) { json::Object Body{{"value", SVR.value}}; - if (SVR.type.has_value()) - Body.insert({"type", SVR.type}); - if (SVR.variablesReference.has_value()) + if (!SVR.type.empty()) + Body.insert({"type", SVR.type}); + if (SVR.variablesReference) Body.insert({"variablesReference", SVR.variablesReference}); - - if (SVR.namedVariables.has_value()) + if (SVR.namedVariables) Body.insert({"namedVariables", SVR.namedVariables}); - - if (SVR.indexedVariables.has_value()) + if (SVR.indexedVariables) Body.insert({"indexedVariables", SVR.indexedVariables}); - - if (SVR.memoryReference.has_value()) - Body.insert({"memoryReference", SVR.memoryReference}); - - if (SVR.valueLocationReference.has_value()) + if (SVR.memoryReference != LLDB_INVALID_ADDRESS) + Body.insert( + {"memoryReference", EncodeMemoryReference(SVR.memoryReference)}); + if (SVR.valueLocationReference) Body.insert({"valueLocationReference", SVR.valueLocationReference}); return json::Value(std::move(Body)); } + bool fromJSON(const json::Value &Params, ScopesArguments &SCA, json::Path P) { json::ObjectMapper O(Params, P); return O && O.map("frameId", SCA.frameId); @@ -471,7 +470,9 @@ json::Value toJSON(const ThreadsResponseBody &TR) { bool fromJSON(const llvm::json::Value &Params, DisassembleArguments &DA, llvm::json::Path P) { json::ObjectMapper O(Params, P); - return O && O.map("memoryReference", DA.memoryReference) && + return O && + DecodeMemoryReference(Params, "memoryReference", DA.memoryReference, P, + /*required=*/true) && O.mapOptional("offset", DA.offset) && O.mapOptional("instructionOffset", DA.instructionOffset) && O.map("instructionCount", DA.instructionCount) && @@ -485,29 +486,14 @@ json::Value toJSON(const DisassembleResponseBody &DRB) { bool fromJSON(const json::Value &Params, ReadMemoryArguments &RMA, json::Path P) { json::ObjectMapper O(Params, P); - - const json::Object *rma_obj = Params.getAsObject(); - constexpr llvm::StringRef ref_key = "memoryReference"; - const std::optional memory_ref = rma_obj->getString(ref_key); - if (!memory_ref) { - P.field(ref_key).report("missing value"); - return false; - } - - const std::optional addr_opt = - DecodeMemoryReference(*memory_ref); - if (!addr_opt) { - P.field(ref_key).report("Malformed memory reference"); - return false; - } - - RMA.memoryReference = *addr_opt; - - return O && O.map("count", RMA.count) && O.mapOptional("offset", RMA.offset); + return O && + DecodeMemoryReference(Params, "memoryReference", RMA.memoryReference, + P, /*required=*/true) && + O.map("count", RMA.count) && O.mapOptional("offset", RMA.offset); } json::Value toJSON(const ReadMemoryResponseBody &RMR) { - json::Object result{{"address", RMR.address}}; + json::Object result{{"address", EncodeMemoryReference(RMR.address)}}; if (RMR.unreadableBytes != 0) result.insert({"unreadableBytes", RMR.unreadableBytes}); @@ -570,24 +556,10 @@ bool fromJSON(const json::Value &Params, WriteMemoryArguments &WMA, json::Path P) { json::ObjectMapper O(Params, P); - const json::Object *wma_obj = Params.getAsObject(); - constexpr llvm::StringRef ref_key = "memoryReference"; - const std::optional memory_ref = wma_obj->getString(ref_key); - if (!memory_ref) { - P.field(ref_key).report("missing value"); - return false; - } - - const std::optional addr_opt = - DecodeMemoryReference(*memory_ref); - if (!addr_opt) { - P.field(ref_key).report("Malformed memory reference"); - return false; - } - - WMA.memoryReference = *addr_opt; - - return O && O.mapOptional("allowPartial", WMA.allowPartial) && + return O && + DecodeMemoryReference(Params, "memoryReference", WMA.memoryReference, + P, /*required=*/true) && + O.mapOptional("allowPartial", WMA.allowPartial) && O.mapOptional("offset", WMA.offset) && O.map("data", WMA.data); } diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h index 63f56fe36f148..0cfd9606b7969 100644 --- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h +++ b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h @@ -398,13 +398,12 @@ bool fromJSON(const llvm::json::Value &, SetVariableArguments &, /// Response to `setVariable` request. struct SetVariableResponseBody { - /// The new value of the variable. std::string value; /// The type of the new value. Typically shown in the UI when hovering over /// the value. - std::optional type; + std::string type; /// If `variablesReference` is > 0, the new value is structured and its /// children can be retrieved by passing `variablesReference` to the @@ -414,26 +413,26 @@ struct SetVariableResponseBody { /// If this property is included in the response, any `variablesReference` /// previously associated with the updated variable, and those of its /// children, are no longer valid. - std::optional variablesReference; + uint64_t variablesReference = 0; /// The number of named child variables. /// The client can use this information to present the variables in a paged /// UI and fetch them in chunks. /// The value should be less than or equal to 2147483647 (2^31-1). - std::optional namedVariables; + uint32_t namedVariables = 0; /// The number of indexed child variables. /// The client can use this information to present the variables in a paged /// UI and fetch them in chunks. /// The value should be less than or equal to 2147483647 (2^31-1). - std::optional indexedVariables; + uint32_t indexedVariables = 0; /// A memory reference to a location appropriate for this result. /// For pointer type eval results, this is generally a reference to the /// memory address contained in the pointer. /// This attribute may be returned by a debug adapter if corresponding /// capability `supportsMemoryReferences` is true. - std::optional memoryReference; + lldb::addr_t memoryReference = LLDB_INVALID_ADDRESS; /// A reference that allows the client to request the location where the new /// value is declared. For example, if the new value is function pointer, the @@ -442,7 +441,7 @@ struct SetVariableResponseBody { /// /// This reference shares the same lifetime as the `variablesReference`. See /// 'Lifetime of Object References' in the Overview section for details. - std::optional valueLocationReference; + uint64_t valueLocationReference = 0; }; llvm::json::Value toJSON(const SetVariableResponseBody &); @@ -805,26 +804,26 @@ llvm::json::Value toJSON(const SetExceptionBreakpointsResponseBody &); struct DisassembleArguments { /// Memory reference to the base location containing the instructions to /// disassemble. - std::string memoryReference; + lldb::addr_t memoryReference = LLDB_INVALID_ADDRESS; /// Offset (in bytes) to be applied to the reference location before /// disassembling. Can be negative. - std::optional offset; + int64_t offset = 0; /// Offset (in instructions) to be applied after the byte offset (if any) /// before disassembling. Can be negative. - std::optional instructionOffset; + int64_t instructionOffset = 0; /// Number of instructions to disassemble starting at the specified location /// and offset. /// An adapter must return exactly this number of instructions - any /// unavailable instructions should be replaced with an implementation-defined /// 'invalid instruction' value. - uint32_t instructionCount; + uint32_t instructionCount = 0; /// If true, the adapter should attempt to resolve memory addresses and other /// values to symbolic names. - std::optional resolveSymbols; + bool resolveSymbols = false; }; bool fromJSON(const llvm::json::Value &, DisassembleArguments &, llvm::json::Path); @@ -842,14 +841,14 @@ llvm::json::Value toJSON(const DisassembleResponseBody &); /// Arguments for `readMemory` request. struct ReadMemoryArguments { /// Memory reference to the base location from which data should be read. - lldb::addr_t memoryReference; + lldb::addr_t memoryReference = LLDB_INVALID_ADDRESS; /// Offset (in bytes) to be applied to the reference location before reading /// data. Can be negative. int64_t offset = 0; /// Number of bytes to read at the specified location and offset. - uint64_t count; + uint64_t count = 0; }; bool fromJSON(const llvm::json::Value &, ReadMemoryArguments &, llvm::json::Path); @@ -859,7 +858,7 @@ struct ReadMemoryResponseBody { /// The address of the first byte of data returned. /// Treated as a hex value if prefixed with `0x`, or as a decimal value /// otherwise. - std::string address; + lldb::addr_t address = LLDB_INVALID_ADDRESS; /// The number of unreadable bytes encountered after the last successfully /// read byte. @@ -947,11 +946,11 @@ llvm::json::Value toJSON(const VariablesResponseBody &); /// Arguments for `writeMemory` request. struct WriteMemoryArguments { /// Memory reference to the base location to which data should be written. - lldb::addr_t memoryReference; + lldb::addr_t memoryReference = LLDB_INVALID_ADDRESS; /// Offset (in bytes) to be applied to the reference location before writing /// data. Can be negative. - std::optional offset; + int64_t offset = 0; /// Property to control partial writes. If true, the debug adapter should /// attempt to write memory even if the entire memory region is not writable. @@ -960,7 +959,7 @@ struct WriteMemoryArguments { /// the response via the `offset` and `bytesWritten` properties. /// If false or missing, a debug adapter should attempt to verify the region /// is writable before writing, and fail the response if it is not. - std::optional allowPartial; + bool allowPartial = false; /// Bytes to write, encoded using base64. std::string data; diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp index 785830c693104..fe8bb5252cc23 100644 --- a/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp +++ b/lldb/tools/lldb-dap/Protocol/ProtocolTypes.cpp @@ -875,19 +875,10 @@ llvm::json::Value toJSON(const DisassembledInstruction::PresentationHint &PH) { bool fromJSON(const llvm::json::Value &Params, DisassembledInstruction &DI, llvm::json::Path P) { llvm::json::ObjectMapper O(Params, P); - std::string raw_address; - if (!O || !O.map("address", raw_address)) - return false; - - std::optional address = DecodeMemoryReference(raw_address); - if (!address) { - P.field("address").report("expected string encoded uint64_t"); - return false; - } - - DI.address = *address; - - return O.map("instruction", DI.instruction) && + return O && + DecodeMemoryReference(Params, "address", DI.address, P, + /*required=*/true) && + O.map("instruction", DI.instruction) && O.mapOptional("instructionBytes", DI.instructionBytes) && O.mapOptional("symbol", DI.symbol) && O.mapOptional("location", DI.location) && diff --git a/lldb/unittests/DAP/ProtocolTypesTest.cpp b/lldb/unittests/DAP/ProtocolTypesTest.cpp index 8add315f47036..3a1b9402b40c9 100644 --- a/lldb/unittests/DAP/ProtocolTypesTest.cpp +++ b/lldb/unittests/DAP/ProtocolTypesTest.cpp @@ -605,15 +605,17 @@ TEST(ProtocolTypesTest, DisassembledInstruction) { parse(R"({"address":1})", "disassemblyInstruction"), FailedWithMessage("expected string at disassemblyInstruction.address")); - EXPECT_THAT_EXPECTED(parse(R"({"address":"-1"})", - "disassemblyInstruction"), - FailedWithMessage("expected string encoded uint64_t at " - "disassemblyInstruction.address")); - EXPECT_THAT_EXPECTED(parse( - R"({"address":"0xfffffffffffffffffffffffffff"})", - "disassemblyInstruction"), - FailedWithMessage("expected string encoded uint64_t at " - "disassemblyInstruction.address")); + EXPECT_THAT_EXPECTED( + parse(R"({"address":"-1"})", + "disassemblyInstruction"), + FailedWithMessage( + "malformed memory reference at disassemblyInstruction.address")); + EXPECT_THAT_EXPECTED( + parse( + R"({"address":"0xfffffffffffffffffffffffffff"})", + "disassemblyInstruction"), + FailedWithMessage( + "malformed memory reference at disassemblyInstruction.address")); } TEST(ProtocolTypesTest, Thread) { @@ -780,7 +782,7 @@ TEST(ProtocolTypesTest, ReadMemoryArguments) { TEST(ProtocolTypesTest, ReadMemoryResponseBody) { ReadMemoryResponseBody response; - response.address = "0xdeadbeef"; + response.address = 0xdeadbeef; const std::string data_str = "hello world!"; std::transform(data_str.begin(), data_str.end(), std::back_inserter(response.data), @@ -788,7 +790,7 @@ TEST(ProtocolTypesTest, ReadMemoryResponseBody) { response.unreadableBytes = 1; Expected expected = json::parse( - R"({ "address": "0xdeadbeef", "data": "aGVsbG8gd29ybGQh", "unreadableBytes": 1})"); + R"({ "address": "0xDEADBEEF", "data": "aGVsbG8gd29ybGQh", "unreadableBytes": 1})"); ASSERT_THAT_EXPECTED(expected, llvm::Succeeded()); EXPECT_EQ(pp(*expected), pp(response)); }