lldb-server: add support for binary memory reads

Summary:
This commit adds support for binary memory reads ($x) to lldb-server. It also removes the "0x"
prefix from the $x client packet, to make it more compatible with the old $m packet. This allows
us to use almost the same code for handling both packet types. I have verified that debugserver
correctly handles $x packets even without the leading "0x". I have added a test which verifies
that the stub returns the same memory contents for both kinds of memory reads ($x and $m).

Reviewers: tberghammer, jasonmolenda

Subscribers: iancottrell, lldb-commits

Differential Revision: http://reviews.llvm.org/D13695

llvm-svn: 250295

6 files changed, 72 insertions, 14 deletions

diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
index 0df7714836c..b0d931ba196 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -113,7 +113,7 @@ GDBRemoteCommunicationServerLLGS::RegisterPacketHandlers()
     RegisterMemberFunctionHandler(StringExtractorGDBRemote::eServerPacketType_interrupt,
                                   &GDBRemoteCommunicationServerLLGS::Handle_interrupt);
     RegisterMemberFunctionHandler(StringExtractorGDBRemote::eServerPacketType_m,
-                                  &GDBRemoteCommunicationServerLLGS::Handle_m);
+                                  &GDBRemoteCommunicationServerLLGS::Handle_memory_read);
     RegisterMemberFunctionHandler(StringExtractorGDBRemote::eServerPacketType_M,
                                   &GDBRemoteCommunicationServerLLGS::Handle_M);
     RegisterMemberFunctionHandler(StringExtractorGDBRemote::eServerPacketType_p,
@@ -164,6 +164,8 @@ GDBRemoteCommunicationServerLLGS::RegisterPacketHandlers()
                                   &GDBRemoteCommunicationServerLLGS::Handle_vCont);
     RegisterMemberFunctionHandler(StringExtractorGDBRemote::eServerPacketType_vCont_actions,
                                   &GDBRemoteCommunicationServerLLGS::Handle_vCont_actions);
+    RegisterMemberFunctionHandler(StringExtractorGDBRemote::eServerPacketType_x,
+                                  &GDBRemoteCommunicationServerLLGS::Handle_memory_read);
     RegisterMemberFunctionHandler(StringExtractorGDBRemote::eServerPacketType_Z,
                                   &GDBRemoteCommunicationServerLLGS::Handle_Z);
     RegisterMemberFunctionHandler(StringExtractorGDBRemote::eServerPacketType_z,
@@ -1975,7 +1977,7 @@ GDBRemoteCommunicationServerLLGS::Handle_interrupt (StringExtractorGDBRemote &pa
 }
 
 GDBRemoteCommunication::PacketResult
-GDBRemoteCommunicationServerLLGS::Handle_m (StringExtractorGDBRemote &packet)
+GDBRemoteCommunicationServerLLGS::Handle_memory_read(StringExtractorGDBRemote &packet)
 {
     Log *log (GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS));
 
@@ -2008,7 +2010,7 @@ GDBRemoteCommunicationServerLLGS::Handle_m (StringExtractorGDBRemote &packet)
     {
         if (log)
             log->Printf ("GDBRemoteCommunicationServerLLGS::%s nothing to read: zero-length packet", __FUNCTION__);
-        return PacketResult::Success;
+        return SendOKResponse();
     }
 
     // Allocate the response buffer.
@@ -2035,8 +2037,16 @@ GDBRemoteCommunicationServerLLGS::Handle_m (StringExtractorGDBRemote &packet)
     }
 
     StreamGDBRemote response;
-    for (size_t i = 0; i < bytes_read; ++i)
-        response.PutHex8(buf[i]);
+    packet.SetFilePos(0);
+    char kind = packet.GetChar('?');
+    if (kind == 'x')
+        response.PutEscapedBytes(buf.data(), byte_count);
+    else
+    {
+        assert(kind == 'm');
+        for (size_t i = 0; i < bytes_read; ++i)
+            response.PutHex8(buf[i]);
+    }
 
     return SendPacketNoLock(response.GetData(), response.GetSize());
 }
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
index fab9f814d84..fe3f718274e 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
@@ -202,8 +202,9 @@ protected:
     PacketResult
     Handle_interrupt (StringExtractorGDBRemote &packet);
 
+    // Handles $m and $x packets.
     PacketResult
-    Handle_m (StringExtractorGDBRemote &packet);
+    Handle_memory_read (StringExtractorGDBRemote &packet);
 
     PacketResult
     Handle_M (StringExtractorGDBRemote &packet);
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index 895fc05aa36..960730b32b7 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -3043,14 +3043,8 @@ ProcessGDBRemote::DoReadMemory (addr_t addr, void *buf, size_t size, Error &erro
     char packet[64];
     int packet_len;
     bool binary_memory_read = m_gdb_comm.GetxPacketSupported();
-    if (binary_memory_read)
-    {
-        packet_len = ::snprintf (packet, sizeof(packet), "x0x%" PRIx64 ",0x%" PRIx64, (uint64_t)addr, (uint64_t)size);
-    }
-    else
-    {
-        packet_len = ::snprintf (packet, sizeof(packet), "m%" PRIx64 ",%" PRIx64, (uint64_t)addr, (uint64_t)size);
-    }
+    packet_len = ::snprintf(packet, sizeof(packet), "%c%" PRIx64 ",%" PRIx64,
+                            binary_memory_read ? 'x' : 'm', (uint64_t)addr, (uint64_t)size);
     assert (packet_len + 1 < (int)sizeof(packet));
     StringExtractorGDBRemote response;
     if (m_gdb_comm.SendPacketAndWaitForResponse(packet, packet_len, response, true) == GDBRemoteCommunication::PacketResult::Success)
diff --git a/lldb/source/Utility/StringExtractorGDBRemote.cpp b/lldb/source/Utility/StringExtractorGDBRemote.cpp
index 03daabca3df..7c09b7a5d04 100644
--- a/lldb/source/Utility/StringExtractorGDBRemote.cpp
+++ b/lldb/source/Utility/StringExtractorGDBRemote.cpp
@@ -310,6 +310,12 @@ StringExtractorGDBRemote::GetServerPacketType () const
       case 'S':
         return eServerPacketType_S;
 
+      case 'x':
+        return eServerPacketType_x;
+
+      case 'X':
+        return eServerPacketType_X;
+
       case 'T':
         return eServerPacketType_T;
 
diff --git a/lldb/source/Utility/StringExtractorGDBRemote.h b/lldb/source/Utility/StringExtractorGDBRemote.h
index da04a12d5fb..b981160b557 100644
--- a/lldb/source/Utility/StringExtractorGDBRemote.h
+++ b/lldb/source/Utility/StringExtractorGDBRemote.h
@@ -145,6 +145,8 @@ public:
         eServerPacketType_s,
         eServerPacketType_S,
         eServerPacketType_T,
+        eServerPacketType_x,
+        eServerPacketType_X,
         eServerPacketType_Z,
         eServerPacketType_z,
 
diff --git a/lldb/test/tools/lldb-server/TestGDBRemoteMemoryRead.py b/lldb/test/tools/lldb-server/TestGDBRemoteMemoryRead.py
new file mode 100644
index 00000000000..33baaa559ab
--- /dev/null
+++ b/lldb/test/tools/lldb-server/TestGDBRemoteMemoryRead.py
@@ -0,0 +1,45 @@
+"""
+Tests the binary ($x) and hex ($m) memory read packets of the remote stub
+"""
+
+import os
+import unittest2
+import lldb
+from lldbtest import *
+import lldbutil
+import binascii
+
+
+class MemoryReadTestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    @skipUnlessPlatform(getDarwinOSTriples()+["linux"])
+    def test_memory_read(self):
+        self.build()
+        exe = os.path.join (os.getcwd(), "a.out")
+
+        target = self.dbg.CreateTarget(exe)
+        lldbutil.run_break_set_by_symbol(self, "main")
+
+        process = target.LaunchSimple (None, None, self.get_process_working_directory())
+        self.assertTrue(process, PROCESS_IS_VALID)
+        self.assertEqual(process.GetState(), lldb.eStateStopped, "Process is stopped")
+
+        pc = process.GetSelectedThread().GetSelectedFrame().GetPC()
+        for size in [1, 2, 4, 8, 16, 32, 64, 128, 256, 512, 1024]:
+            error = lldb.SBError()
+            memory = process.ReadMemory(pc, size, error)
+            self.assertTrue(error.Success())
+            self.match("process plugin packet send x%x,%x" % (pc, size), ["response:", memory])
+            self.match("process plugin packet send m%x,%x" % (pc, size), ["response:", binascii.hexlify(memory)])
+
+        process.Continue()
+        self.assertEqual(process.GetState(), lldb.eStateExited, "Process exited")
+
+
+if __name__ == '__main__':
+    import atexit
+    lldb.SBDebugger.Initialize()
+    atexit.register(lambda: lldb.SBDebugger.Terminate())
+    unittest2.main()