fixup! Address review comments

Author: felipepiovezan

lldb/test/API/macosx/debugserver-multimemread/TestDebugserverMultiMemRead.py

@@ -1,5 +1,5 @@
 """
-Tests the exit code/description coming from the debugserver.
+Tests debugserver support for MultiMemRead.
 """
 
 import lldb
@@ -17,12 +17,16 @@ def send_process_packet(self, packet_str):
         #  packet: <packet_str>
         #  response: <response>
         reply = self.res.GetOutput().split("\n")
-        reply[0] = reply[0].strip()
-        reply[1] = reply[1].strip()
+        packet = reply[0].strip()
+        response = reply[1].strip()
 
-        self.assertTrue(reply[0].startswith("packet: "), reply[0])
-        self.assertTrue(reply[1].startswith("response: "))
-        return reply[1][len("response: ") :]
+        self.assertTrue(packet.startswith("packet: "))
+        self.assertTrue(response.startswith("response: "))
+        return response[len("response: ") :]
+
+    def check_invalid_packet(self, packet_str):
+        reply = self.send_process_packet("packet_str")
+        self.assertEqual(reply, "E03")
 
     def test_packets(self):
         self.build()
@@ -36,30 +40,41 @@ def test_packets(self):
 
         mem_address_var = thread.frames[0].FindVariable("memory")
         self.assertTrue(mem_address_var)
-        mem_address = int(mem_address_var.GetValue(), 16)
+        mem_address = mem_address_var.GetValueAsUnsigned() + 42
 
-        reply = self.send_process_packet("MultiMemRead:")
-        self.assertEqual(reply, "E03")
-        reply = self.send_process_packet("MultiMemRead:ranges:")
-        self.assertEqual(reply, "E03")
-        reply = self.send_process_packet("MultiMemRead:ranges:10")
-        self.assertEqual(reply, "E03")
-        reply = self.send_process_packet("MultiMemRead:ranges:10,")
-        self.assertEqual(reply, "E03")
-        reply = self.send_process_packet("MultiMemRead:ranges:10,2")
-        self.assertEqual(reply, "E03")
-        reply = self.send_process_packet("MultiMemRead:ranges:10,2,")
-        self.assertEqual(reply, "E03")
+        # no ":"
+        self.check_invalid_packet("MultiMemRead")
+        # missing ranges
+        self.check_invalid_packet("MultiMemRead:")
+        # needs at least one range
+        self.check_invalid_packet("MultiMemRead:ranges:")
+        # needs at least one range
+        self.check_invalid_packet("MultiMemRead:ranges:,")
+        # a range is a pair of numbers
+        self.check_invalid_packet("MultiMemRead:ranges:10")
+        # a range is a pair of numbers
+        self.check_invalid_packet("MultiMemRead:ranges:10,")
+        # range list must end with ;
+        self.check_invalid_packet("MultiMemRead:ranges:10,2")
+        self.check_invalid_packet("MultiMemRead:ranges:10,2,")
+        self.check_invalid_packet("MultiMemRead:ranges:10,2,3")
+        # ranges are pairs of numbers.
+        self.check_invalid_packet("MultiMemRead:ranges:10,2,3;")
+        # unrecognized field
+        self.check_invalid_packet("MultiMemRead:ranges:10,2;blah:;")
+        # unrecognized field
+        self.check_invalid_packet("MultiMemRead:blah:;ranges:10,2;")
+
+        # This is a valid way of testing for MultiMemRead support
+        reply = self.send_process_packet("MultiMemRead:ranges:0,0;")
+        self.assertEqual(reply, "0;")
+        # Debugserver is permissive with trailing commas.
         reply = self.send_process_packet("MultiMemRead:ranges:10,2,;")
-        self.assertEqual(reply, "0;")  # Debugserver is permissive with trailing commas.
+        self.assertEqual(reply, "0;")
         reply = self.send_process_packet("MultiMemRead:ranges:10,2;")
         self.assertEqual(reply, "0;")
         reply = self.send_process_packet(f"MultiMemRead:ranges:{mem_address:x},0;")
         self.assertEqual(reply, "0;")
-        reply = self.send_process_packet(
-            f"MultiMemRead:ranges:{mem_address:x},0;options:;"
-        )
-        self.assertEqual(reply, "0;")
         reply = self.send_process_packet(f"MultiMemRead:ranges:{mem_address:x},2;")
         self.assertEqual(reply, "2;ab")
         reply = self.send_process_packet(
@@ -70,3 +85,14 @@ def test_packets(self):
             f"MultiMemRead:ranges:{mem_address:x},2,{mem_address+2:x},4,{mem_address+6:x},8;"
         )
         self.assertEqual(reply, "2,4,8;abcdefghijklmn")
+
+        # Test zero length in the middle.
+        reply = self.send_process_packet(
+            f"MultiMemRead:ranges:{mem_address:x},2,{mem_address+2:x},0,{mem_address+6:x},8;"
+        )
+        self.assertEqual(reply, "2,0,8;abghijklmn")
+        # Test zero length in the end.
+        reply = self.send_process_packet(
+            f"MultiMemRead:ranges:{mem_address:x},2,{mem_address+2:x},4,{mem_address+6:x},0;"
+        )
+        self.assertEqual(reply, "2,4,0;abcdef")

lldb/test/API/macosx/debugserver-multimemread/main.c

@@ -4,7 +4,11 @@
 int main(int argc, char **argv) {
   char *memory = malloc(1024);
   memset(memory, '-', 1024);
-  for (int i = 0; i < 50; i++)
-    memory[i] = 'a' + i;
+  // Write "interesting" characters at an offset from the memory filled with
+  // `-`. This way, if we read outside the range in either direction, we should
+  // find `-`s`.
+  int offset = 42;
+  for (int i = offset; i < offset + 14; i++)
+    memory[i] = 'a' + (i - offset);
   return 0; // break here
 }

lldb/tools/debugserver/source/RNBRemote.cpp

@@ -170,6 +170,20 @@ static uint64_t decode_uint64(const char *p, int base, char **end = nullptr,
   return addr;
 }
 
+/// Attempts to parse a prefix of `number_str` as a uint64_t. If
+/// successful, the number is returned and the prefix is dropped from
+/// `number_str`.
+static std::optional<uint64_t> extract_u64(std::string_view &number_str) {
+  char *str_end = nullptr;
+  errno = 0;
+  uint64_t number = strtoull(number_str.data(), &str_end, 16);
+  if (errno != 0)
+    return std::nullopt;
+  assert(str_end);
+  number_str.remove_prefix(str_end - number_str.data());
+  return number;
+}
+
 static void append_hex_value(std::ostream &ostrm, const void *buf,
                              size_t buf_size, bool swap) {
   int i;
@@ -204,6 +218,25 @@ static void append_hexified_string(std::ostream &ostrm,
   }
 }
 
+/// Returns true if `str` starts with `prefix`.
+static bool starts_with(std::string_view str, std::string_view prefix) {
+  return str.substr(0, prefix.size()) == prefix;
+}
+
+/// Splits `list_str` into multiple string_views separated by `,`.
+static std::vector<std::string_view>
+parse_comma_separated_list(std::string_view list_str) {
+  std::vector<std::string_view> list;
+  while (!list_str.empty()) {
+    auto pos = list_str.find(',');
+    list.push_back(list_str.substr(0, pos));
+    if (pos == list_str.npos)
+      break;
+    list_str.remove_prefix(pos + 1);
+  }
+  return list;
+}
+
 // from System.framework/Versions/B/PrivateHeaders/sys/codesign.h
 extern "C" {
 #define CS_OPS_STATUS 0       /* return status */
@@ -3155,42 +3188,6 @@ rnb_err_t RNBRemote::HandlePacket_m(const char *p) {
   return SendPacket(ostrm.str());
 }
 
-/// Returns true if `str` starts with `prefix`.
-static bool starts_with(std::string_view str, std::string_view prefix) {
-  return str.size() >= prefix.size() &&
-         str.compare(0, prefix.size(), prefix) == 0;
-}
-
-/// Attempts to parse a prefix of `number_str` as a number of type `T`. If
-/// successful, the number is returned and the prefix is dropped from
-/// `number_str`.
-template <typename T>
-static std::optional<T> extract_number(std::string_view &number_str) {
-  static_assert(std::is_integral<T>());
-  char *str_end = nullptr;
-  errno = 0;
-  nub_addr_t number = strtoull(number_str.data(), &str_end, 16);
-  if (errno != 0)
-    return std::nullopt;
-  assert(str_end);
-  number_str.remove_prefix(str_end - number_str.data());
-  return number;
-}
-
-/// Splits `list_str` into multiple string_views separated by `,`.
-static std::vector<std::string_view>
-parse_comma_separated_list(std::string_view list_str) {
-  std::vector<std::string_view> list;
-  while (!list_str.empty()) {
-    auto pos = list_str.find(',');
-    list.push_back(list_str.substr(0, pos));
-    if (pos == list_str.npos)
-      break;
-    list_str.remove_prefix(pos + 1);
-  }
-  return list;
-}
-
 rnb_err_t RNBRemote::HandlePacket_MultiMemRead(const char *p) {
   const std::string_view packet_name("MultiMemRead:");
   std::string_view packet(p);
@@ -3227,10 +3224,8 @@ rnb_err_t RNBRemote::HandlePacket_MultiMemRead(const char *p) {
         "MultiMemRead has an odd number of numbers for the ranges");
 
   for (unsigned idx = 0; idx < numbers_list.size(); idx += 2) {
-    std::optional<nub_addr_t> maybe_addr =
-        extract_number<nub_addr_t>(numbers_list[idx]);
-    std::optional<size_t> maybe_length =
-        extract_number<size_t>(numbers_list[idx + 1]);
+    std::optional<uint64_t> maybe_addr = extract_u64(numbers_list[idx]);
+    std::optional<uint64_t> maybe_length = extract_u64(numbers_list[idx + 1]);
     if (!maybe_addr || !maybe_length)
       return HandlePacket_ILLFORMED(__FILE__, __LINE__, packet.data(),
                                     "Invalid MultiMemRead range");
@@ -3248,28 +3243,14 @@ rnb_err_t RNBRemote::HandlePacket_MultiMemRead(const char *p) {
     return HandlePacket_ILLFORMED(__FILE__, __LINE__, p,
                                   "MultiMemRead has an empty range list");
 
-  // If 'options:' is present, ensure it's empty.
-  const std::string_view options_prefix("options:");
-  if (starts_with(packet, options_prefix)) {
-    packet.remove_prefix(options_prefix.size());
-    if (packet.empty())
-      return HandlePacket_ILLFORMED(
-          __FILE__, __LINE__, packet.data(),
-          "Too short 'options' in MultiMemRead packet");
-    if (packet[0] != ';')
-      return HandlePacket_ILLFORMED(__FILE__, __LINE__, packet.data(),
-                                    "Unimplemented 'options' in MultiMemRead");
-    packet.remove_prefix(1);
-  }
-
   if (!packet.empty())
-    return HandlePacket_ILLFORMED(__FILE__, __LINE__, p,
-                                  "Invalid MultiMemRead address_range");
+    return HandlePacket_ILLFORMED(
+        __FILE__, __LINE__, p, "MultiMemRead packet has unrecognized fields");
 
   std::vector<std::vector<uint8_t>> buffers;
   buffers.reserve(ranges.size());
   for (auto [base_addr, length] : ranges) {
-    buffers.emplace_back(length, '\0');
+    buffers.emplace_back(length, 0);
     nub_size_t bytes_read = DNBProcessMemoryRead(m_ctx.ProcessID(), base_addr,
                                                  length, buffers.back().data());
     buffers.back().resize(bytes_read);