lldb-server: add support for binary memory reads

[labath]

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

[tberghammer]

Look good

[zturner]

Any tests for this?

[labath]

The new functionality gets picked up by the client and automatically used everywhere, so I'm not
really worried about testing it. A more important issue would be making sure that the legacy read
packet does not rot as it is now unused. I have added a test which makes sure both packets return
the same value (modulo hex-encoding), which should prevent that.

[labath]

Remove files which slipped in...

[tberghammer]

Looks good

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h

protected:
     Handle_H (StringExtractorGDBRemote &packet);
 
     PacketResult
     Handle_I (StringExtractorGDBRemote &packet);
 
     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);
 
     PacketResult
     Handle_qMemoryRegionInfoSupported (StringExtractorGDBRemote &packet);
 
     PacketResult

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

RegisterMemberFunctionHandler(StringExtractorGDBRemote::eServerPacketType_D,
                                   &GDBRemoteCommunicationServerLLGS::Handle_D);
     RegisterMemberFunctionHandler(StringExtractorGDBRemote::eServerPacketType_H,
                                   &GDBRemoteCommunicationServerLLGS::Handle_H);
     RegisterMemberFunctionHandler(StringExtractorGDBRemote::eServerPacketType_I,
                                   &GDBRemoteCommunicationServerLLGS::Handle_I);
     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,
                                   &GDBRemoteCommunicationServerLLGS::Handle_p);
     RegisterMemberFunctionHandler(StringExtractorGDBRemote::eServerPacketType_P,
                                   &GDBRemoteCommunicationServerLLGS::Handle_P);
     RegisterMemberFunctionHandler(StringExtractorGDBRemote::eServerPacketType_qC,
                                   &GDBRemoteCommunicationServerLLGS::Handle_qC);
GDBRemoteCommunicationServerLLGS::RegisterPacketHandlers()
     RegisterMemberFunctionHandler(StringExtractorGDBRemote::eServerPacketType_stop_reason,
                                   &GDBRemoteCommunicationServerLLGS::Handle_stop_reason); // ?
     RegisterMemberFunctionHandler(StringExtractorGDBRemote::eServerPacketType_vAttach,
                                   &GDBRemoteCommunicationServerLLGS::Handle_vAttach);
     RegisterMemberFunctionHandler(StringExtractorGDBRemote::eServerPacketType_vCont,
                                   &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,
                                   &GDBRemoteCommunicationServerLLGS::Handle_z);
 
     RegisterPacketHandler(StringExtractorGDBRemote::eServerPacketType_k,
                           [this](StringExtractorGDBRemote packet,
                                  Error &error,
GDBRemoteCommunicationServerLLGS::Handle_interrupt (StringExtractorGDBRemote &packet)
     if (log)
         log->Printf ("GDBRemoteCommunicationServerLLGS::%s stopped process %" PRIu64, __FUNCTION__, m_debugged_process_sp->GetID ());
 
     // No response required from stop all.
     return PacketResult::Success;
 }
 
 GDBRemoteCommunication::PacketResult
-GDBRemoteCommunicationServerLLGS::Handle_m (StringExtractorGDBRemote &packet)
+GDBRemoteCommunicationServerLLGS::Handle_memory_read(StringExtractorGDBRemote &packet)
 {
     Log *log (GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS));
 
     if (!m_debugged_process_sp || (m_debugged_process_sp->GetID () == LLDB_INVALID_PROCESS_ID))
     {
         if (log)
             log->Printf ("GDBRemoteCommunicationServerLLGS::%s failed, no process available", __FUNCTION__);
         return SendErrorResponse (0x15);
GDBRemoteCommunicationServerLLGS::Handle_memory_read(StringExtractorGDBRemote &packet)
     if (packet.GetBytesLeft() < 1)
         return SendIllFormedResponse(packet, "Length missing in m packet");
 
     const uint64_t byte_count = packet.GetHexMaxU64(false, 0);
     if (byte_count == 0)
     {
         if (log)
             log->Printf ("GDBRemoteCommunicationServerLLGS::%s nothing to read: zero-length packet", __FUNCTION__);
-        return PacketResult::Success;
+        return SendOKResponse();
     }
 
     // Allocate the response buffer.
     std::string buf(byte_count, '\0');
     if (buf.empty())
         return SendErrorResponse (0x78);
 
 
GDBRemoteCommunicationServerLLGS::Handle_memory_read(StringExtractorGDBRemote &packet)
     if (bytes_read == 0)
     {
         if (log)
             log->Printf ("GDBRemoteCommunicationServerLLGS::%s pid %" PRIu64 " mem 0x%" PRIx64 ": read 0 of %" PRIu64 " requested bytes", __FUNCTION__, m_debugged_process_sp->GetID (), read_addr, byte_count);
         return SendErrorResponse (0x08);
     }
 
     StreamGDBRemote response;
+    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());
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerLLGS::Handle_M (StringExtractorGDBRemote &packet)
 {
     Log *log (GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS));

lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

if (size > m_max_memory_size)
         // called multiple times in order to complete the task by 
         // lldb_private::Process so it is ok to do this.
         size = m_max_memory_size;
     }
 
     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)
     {
         if (response.IsNormalResponse())
         {
             error.Clear();
             if (binary_memory_read)

lldb/trunk/source/Utility/StringExtractorGDBRemote.h

enum ServerPacketType
         eServerPacketType_k,
         eServerPacketType_m,
         eServerPacketType_M,
         eServerPacketType_p,
         eServerPacketType_P,
         eServerPacketType_s,
         eServerPacketType_S,
         eServerPacketType_T,
+        eServerPacketType_x,
+        eServerPacketType_X,
         eServerPacketType_Z,
         eServerPacketType_z,
 
         eServerPacketType__M,
         eServerPacketType__m,
         eServerPacketType_notify, // '%' notification
     };
     

lldb/trunk/source/Utility/StringExtractorGDBRemote.cpp

case 'v':
 
       case 's':
         if (packet_size == 1) return eServerPacketType_s;
         break;
 
       case 'S':
         return eServerPacketType_S;
 
+      case 'x':
+        return eServerPacketType_x;
+
+      case 'X':
+        return eServerPacketType_X;
+
       case 'T':
         return eServerPacketType_T;
 
       case 'z':
         if (packet_cstr[1] >= '0' && packet_cstr[1] <= '4')
           return eServerPacketType_z;
         break;
 

lldb/trunk/test/tools/lldb-server/TestGDBRemoteMemoryRead.py

+"""
+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()