[lldb-dap] Updating protocol memory references to lldb::addr_t. (#148037)

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.

Author: ashgti

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",

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.

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

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

@@ -182,22 +182,15 @@ static DisassembledInstruction ConvertSBInstructionToDisassembledInstruction(
 /// `supportsDisassembleRequest` is true.
 llvm::Expected<DisassembleResponseBody>
 DisassembleRequestHandler::Run(const DisassembleArguments &args) const {
-  std::optional<lldb::addr_t> addr_opt =
-      DecodeMemoryReference(args.memoryReference);
-  if (!addr_opt.has_value())
-    return llvm::make_error<DAPError>("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<DAPError>(
         "Memory reference not found in the current binary.");
 
   // 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<DAPError>(
         "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<DisassembledInstruction> instructions;
   size_t original_address_index = args.instructionCount;
   for (size_t i = 0; i < insts.GetSize(); ++i) {

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

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;

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

@@ -22,8 +22,7 @@ namespace lldb_dap {
 llvm::Expected<protocol::WriteMemoryResponseBody>
 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;

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"
@@ -338,26 +339,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);
@@ -498,7 +497,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) &&
@@ -512,29 +513,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<llvm::StringRef> memory_ref = rma_obj->getString(ref_key);
-  if (!memory_ref) {
-    P.field(ref_key).report("missing value");
-    return false;
-  }
-
-  const std::optional<lldb::addr_t> 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});
@@ -597,24 +583,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<llvm::StringRef> memory_ref = wma_obj->getString(ref_key);
-  if (!memory_ref) {
-    P.field(ref_key).report("missing value");
-    return false;
-  }
-
-  const std::optional<lldb::addr_t> 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);
 }