Move packet construction from GDBRemoteRegisterContext go the communication class

[labath]

Summary

When saving/restoring registers the GDBRemoteRegisterContext class was manually constructing
the register save/restore packets. This creates appropriate helper functions in
GDBRemoteCommunicationClient, and switches the class to use those. It also removes what a
duplicate packet send in some of those functions, a thing that I can only attribute to a bad
merge artefact.

I also add a test framework for testing gdb-remote client functionality and add tests for the new
functions I introduced. I'd like to be able to test the register context changes in isolation as
well, but currently there doesn't seem to be a way to reasonably construct a standalone register
context object, so we'll have to rely on the end-to-end tests to verify that.

[labath]

source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp

601

The location of the duplicate Send. Also similar two cases in WriteAllRegisterValues.

[labath]

Fix indentation

[clayborg]

Looks good. I know there are many other functions that we should port down into the client code.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h

ReadRegister(lldb::tid_t tid,
                  uint32_t reg_num,   // Must be the eRegisterKindProcessPlugin register number, to be sent to the remote
                  StringExtractorGDBRemote &response);
 
     bool
     ReadAllRegisters (lldb::tid_t tid,
                       StringExtractorGDBRemote &response);
 
     bool
-    SaveRegisterState (lldb::tid_t tid, uint32_t &save_id);
-    
+    WriteRegister(lldb::tid_t tid, uint32_t reg_num, // eRegisterKindProcessPlugin register number
+                  llvm::StringRef data);
+
+    bool
+    WriteAllRegisters(lldb::tid_t tid, llvm::StringRef data);
+
+    bool
+    SaveRegisterState(lldb::tid_t tid, uint32_t &save_id);
     bool
     RestoreRegisterState (lldb::tid_t tid, uint32_t save_id);
 
     const char *
     GetGDBServerProgramName();
     
     uint32_t
     GetGDBServerProgramVersion();

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

if (lock)
         }
     }
     else if (Log *log = ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet(GDBR_LOG_PROCESS | GDBR_LOG_PACKETS))
     {
         log->Printf("GDBRemoteCommunicationClient::%s: Didn't get sequence mutex for g packet.", __FUNCTION__);
     }
     return false;
 }
+
+bool
+GDBRemoteCommunicationClient::WriteRegister(lldb::tid_t tid, uint32_t reg_num, llvm::StringRef data)
+{
+    Lock lock(*this, false);
+    if (!lock)
+    {
+        if (Log *log = ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet(GDBR_LOG_PROCESS | GDBR_LOG_PACKETS))
+            log->Printf("GDBRemoteCommunicationClient::%s: Didn't get sequence mutex for P packet.", __FUNCTION__);
+        return false;
+    }
+
+    const bool thread_suffix_supported = GetThreadSuffixSupported();
+    if (!thread_suffix_supported && !SetCurrentThread(tid))
+        return false;
+
+    StreamString packet;
+    packet.Printf("P%x=", reg_num);
+    packet.PutBytesAsRawHex8(data.data(), data.size(), endian::InlHostByteOrder(), endian::InlHostByteOrder());
+    if (thread_suffix_supported)
+        packet.Printf(";thread:%4.4" PRIx64 ";", tid);
+    StringExtractorGDBRemote response;
+    return SendPacketAndWaitForResponse(packet.GetData(), packet.GetSize(), response, false) == PacketResult::Success &&
+           response.IsOKResponse();
+}
+
+bool
+GDBRemoteCommunicationClient::WriteAllRegisters(lldb::tid_t tid, llvm::StringRef data)
+{
+    Lock lock(*this, false);
+    if (!lock)
+    {
+        if (Log *log = ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet(GDBR_LOG_PROCESS | GDBR_LOG_PACKETS))
+            log->Printf("GDBRemoteCommunicationClient::%s: Didn't get sequence mutex for G packet.", __FUNCTION__);
+        return false;
+    }
+
+    const bool thread_suffix_supported = GetThreadSuffixSupported();
+    if (!thread_suffix_supported && !SetCurrentThread(tid))
+        return false;
+
+    StreamString packet;
+    packet.PutChar('G');
+    packet.Write(data.data(), data.size());
+    if (thread_suffix_supported)
+        packet.Printf(";thread:%4.4" PRIx64 ";", tid);
+    StringExtractorGDBRemote response;
+    return SendPacketAndWaitForResponse(packet.GetData(), packet.GetSize(), response, false) == PacketResult::Success &&
+           response.IsOKResponse();
+}
+
 bool
 GDBRemoteCommunicationClient::SaveRegisterState (lldb::tid_t tid, uint32_t &save_id)
 {
     save_id = 0; // Set to invalid save ID
     if (m_supports_QSaveRegisterState == eLazyBoolNo)
         return false;
     
     m_supports_QSaveRegisterState = eLazyBoolYes;

source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp

GDBRemoteRegisterContext::ReadAllRegisterValues (lldb::DataBufferSP &data_sp)
     StringExtractorGDBRemote response;
 
     const bool use_g_packet = gdb_comm.AvoidGPackets ((ProcessGDBRemote *)process) == false;
 
     GDBRemoteClientBase::Lock lock(gdb_comm, false);
     if (lock)
     {
         SyncThreadState(process);
-        
-        char packet[32];
-        const bool thread_suffix_supported = gdb_comm.GetThreadSuffixSupported();
-        ProcessSP process_sp (m_thread.GetProcess());
-        if (thread_suffix_supported || static_cast<ProcessGDBRemote *>(process_sp.get())->GetGDBRemote().SetCurrentThread(m_thread.GetProtocolID()))
-        {
-            int packet_len = 0;
-            if (thread_suffix_supported)
-                packet_len = ::snprintf (packet, sizeof(packet), "g;thread:%4.4" PRIx64, m_thread.GetProtocolID());
-            else
-                packet_len = ::snprintf (packet, sizeof(packet), "g");
-            assert (packet_len < ((int)sizeof(packet) - 1));
-
-            if (use_g_packet && gdb_comm.SendPacketAndWaitForResponse(packet, packet_len, response, false) == GDBRemoteCommunication::PacketResult::Success)
-            {
-                int packet_len = 0;
-                if (thread_suffix_supported)
-                    packet_len = ::snprintf (packet, sizeof(packet), "g;thread:%4.4" PRIx64, m_thread.GetProtocolID());
-                else
-                    packet_len = ::snprintf (packet, sizeof(packet), "g");
-                assert (packet_len < ((int)sizeof(packet) - 1));
-    
-                if (gdb_comm.SendPacketAndWaitForResponse(packet, packet_len, response, false) == GDBRemoteCommunication::PacketResult::Success)
-                {
-                    if (response.IsErrorResponse())
-                        return false;
-    
-                    std::string &response_str = response.GetStringRef();
-                    if (isxdigit(response_str[0]))
-                    {
-                        response_str.insert(0, 1, 'G');
-                        if (thread_suffix_supported)
-                        {
-                            char thread_id_cstr[64];
-                            ::snprintf (thread_id_cstr, sizeof(thread_id_cstr), ";thread:%4.4" PRIx64 ";", m_thread.GetProtocolID());
-                            response_str.append (thread_id_cstr);
-                        }
-                        data_sp.reset (new DataBufferHeap (response_str.c_str(), response_str.size()));
-                        return true;
-                    }
-                }
-            }
-            else
-            {
-                // For the use_g_packet == false case, we're going to read each register 
-                // individually and store them as binary data in a buffer instead of as ascii
-                // characters.
-                const RegisterInfo *reg_info;
+        if (use_g_packet && gdb_comm.ReadAllRegisters(m_thread.GetProtocolID(), response))
+            if (!isxdigit(response_str[0]))
+                return false;
+
+            data_sp.reset(new DataBufferHeap(response_str.c_str(), response_str.size()));
+            // For the use_g_packet == false case, we're going to read each register
 
-                // data_sp will take ownership of this DataBufferHeap pointer soon.
-                DataBufferSP reg_ctx(new DataBufferHeap(m_reg_info.GetRegisterDataByteSize(), 0));
 
-                for (uint32_t i = 0; (reg_info = GetRegisterInfoAtIndex (i)) != NULL; i++)
-                {
-                    if (reg_info->value_regs) // skip registers that are slices of real registers
-                        continue;
-                    ReadRegisterBytes (reg_info, m_reg_data);
-                    // ReadRegisterBytes saves the contents of the register in to the m_reg_data buffer
-                }
-                memcpy (reg_ctx->GetBytes(), m_reg_data.GetDataStart(), m_reg_info.GetRegisterDataByteSize());
+            for (uint32_t i = 0; (reg_info = GetRegisterInfoAtIndex(i)) != NULL; i++)
+                ReadRegisterBytes(reg_info, m_reg_data);
+            memcpy(reg_ctx->GetBytes(), m_reg_data.GetDataStart(), m_reg_info.GetRegisterDataByteSize());
 
-                data_sp = reg_ctx;
-                return true;
-            }
-        }
-    }
     else
     {
 
         Log *log (ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet (GDBR_LOG_THREAD | GDBR_LOG_PACKETS));
         if (log)
         {
             if (log->GetVerbose())
             {
GDBRemoteRegisterContext::WriteAllRegisterValues (const lldb::DataBufferSP &data_sp)
     GDBRemoteCommunicationClient &gdb_comm (((ProcessGDBRemote *)process)->GetGDBRemote());
 
     const bool use_g_packet = gdb_comm.AvoidGPackets ((ProcessGDBRemote *)process) == false;
 
     StringExtractorGDBRemote response;
     GDBRemoteClientBase::Lock lock(gdb_comm, false);
     if (lock)
     {
-        const bool thread_suffix_supported = gdb_comm.GetThreadSuffixSupported();
-        ProcessSP process_sp (m_thread.GetProcess());
-        if (thread_suffix_supported || static_cast<ProcessGDBRemote *>(process_sp.get())->GetGDBRemote().SetCurrentThread(m_thread.GetProtocolID()))
-        {
-            // The data_sp contains the entire G response packet including the
-            // G, and if the thread suffix is supported, it has the thread suffix
-            // as well.
-            const char *G_packet = (const char *)data_sp->GetBytes();
-            size_t G_packet_len = data_sp->GetByteSize();
-            if (use_g_packet
-                && gdb_comm.SendPacketAndWaitForResponse (G_packet,
-                                                          G_packet_len,
-                                                          response,
-                                                          false) == GDBRemoteCommunication::PacketResult::Success)
-            {
-                // The data_sp contains the entire G response packet including the
-                // G, and if the thread suffix is supported, it has the thread suffix
-                // as well.
-                const char *G_packet = (const char *)data_sp->GetBytes();
-                size_t G_packet_len = data_sp->GetByteSize();
-                if (gdb_comm.SendPacketAndWaitForResponse (G_packet,
-                                                           G_packet_len,
-                                                           response,
-                                                           false) == GDBRemoteCommunication::PacketResult::Success)
-                {
-                    if (response.IsOKResponse())
-                        return true;
-                    else if (response.IsErrorResponse())
-                    {
-                        uint32_t num_restored = 0;
-                        // We need to manually go through all of the registers and
-                        // restore them manually
-    
-                        response.GetStringRef().assign (G_packet, G_packet_len);
-                        response.SetFilePos(1); // Skip the leading 'G'
+        // The data_sp contains the G response packet.
+        llvm::StringRef data(reinterpret_cast<const char *>(data_sp->GetBytes()), data_sp->GetByteSize());
+        if (use_g_packet)
+            if (gdb_comm.WriteAllRegisters(m_thread.GetProtocolID(), data))
+
+            response.GetStringRef() = data;
+            DataBufferHeap buffer(data.size() / 2, 0);
+
+            const uint32_t bytes_extracted = response.GetHexBytes(buffer.GetBytes(), buffer.GetByteSize(), '\xcc');
 
-                        // G_packet_len is hex-ascii characters plus prefix 'G' plus suffix thread specifier.
-                        // This means buffer will be a little more than 2x larger than necessary but we resize
-                        // it down once we've extracted all hex ascii chars from the packet.
-                        DataBufferHeap buffer (G_packet_len, 0);
-
-                        const uint32_t bytes_extracted = response.GetHexBytes (buffer.GetBytes(),
-                                                                               buffer.GetByteSize(),
-                                                                               '\xcc');
-
-                        DataExtractor restore_data (buffer.GetBytes(),
-                                                    buffer.GetByteSize(),
-                                                    m_reg_data.GetByteOrder(),
-                                                    m_reg_data.GetAddressByteSize());
+            DataExtractor restore_data(buffer.GetBytes(), buffer.GetByteSize(), m_reg_data.GetByteOrder(),
 
-                        if (bytes_extracted < restore_data.GetByteSize())
-                            restore_data.SetData(restore_data.GetDataStart(), bytes_extracted, m_reg_data.GetByteOrder());
-    
-                        const RegisterInfo *reg_info;
 
-                        // The g packet contents may either include the slice registers (registers defined in
-                        // terms of other registers, e.g. eax is a subset of rax) or not.  The slice registers 
-                        // should NOT be in the g packet, but some implementations may incorrectly include them.
-                        // 
-                        // If the slice registers are included in the packet, we must step over the slice registers 
-                        // when parsing the packet -- relying on the RegisterInfo byte_offset field would be incorrect.
-                        // If the slice registers are not included, then using the byte_offset values into the
-                        // data buffer is the best way to find individual register values.
+            // terms of other registers, e.g. eax is a subset of rax) or not.  The slice registers
+            //
+            // If the slice registers are included in the packet, we must step over the slice registers
 
-                        uint64_t size_including_slice_registers = 0;
-                        uint64_t size_not_including_slice_registers = 0;
-                        uint64_t size_by_highest_offset = 0;
 
-                        for (uint32_t reg_idx=0; (reg_info = GetRegisterInfoAtIndex (reg_idx)) != NULL; ++reg_idx)
-                        {
-                            size_including_slice_registers += reg_info->byte_size;
-                            if (reg_info->value_regs == NULL)
-                                size_not_including_slice_registers += reg_info->byte_size;
-                            if (reg_info->byte_offset >= size_by_highest_offset)
-                                size_by_highest_offset = reg_info->byte_offset + reg_info->byte_size;
-                        }
+            for (uint32_t reg_idx = 0; (reg_info = GetRegisterInfoAtIndex(reg_idx)) != NULL; ++reg_idx)
 
-                        bool use_byte_offset_into_buffer;
-                        if (size_by_highest_offset == restore_data.GetByteSize())
-                        {
-                            // The size of the packet agrees with the highest offset: + size in the register file
-                            use_byte_offset_into_buffer = true;
-                        }
-                        else if (size_not_including_slice_registers == restore_data.GetByteSize())
-                        {
-                            // The size of the packet is the same as concatenating all of the registers sequentially,
-                            // skipping the slice registers
-                            use_byte_offset_into_buffer = true;
-                        }
-                        else if (size_including_slice_registers == restore_data.GetByteSize())
-                        {
-                            // The slice registers are present in the packet (when they shouldn't be).
-                            // Don't try to use the RegisterInfo byte_offset into the restore_data, it will
-                            // point to the wrong place.
-                            use_byte_offset_into_buffer = false;
-                        }
-                        else {
-                            // None of our expected sizes match the actual g packet data we're looking at.
-                            // The most conservative approach here is to use the running total byte offset.
-                            use_byte_offset_into_buffer = false;
-                        }
+            else
+            {
 
-                        // In case our register definitions don't include the correct offsets,
-                        // keep track of the size of each reg & compute offset based on that.
-                        uint32_t running_byte_offset = 0;
-                        for (uint32_t reg_idx=0; (reg_info = GetRegisterInfoAtIndex (reg_idx)) != NULL; ++reg_idx, running_byte_offset += reg_info->byte_size)
-                        {
-                            // Skip composite aka slice registers (e.g. eax is a slice of rax).
-                            if (reg_info->value_regs)
-                                continue;
+            for (uint32_t reg_idx = 0; (reg_info = GetRegisterInfoAtIndex(reg_idx)) != NULL;
+                 ++reg_idx, running_byte_offset += reg_info->byte_size)
 
-                            const uint32_t reg = reg_info->kinds[eRegisterKindLLDB];
 
-                            uint32_t register_offset;
-                            if (use_byte_offset_into_buffer)
-                            {
-                                register_offset = reg_info->byte_offset;
-                            }
-                            else
-                            {
-                                register_offset = running_byte_offset;
-                            }
 
-                            // Only write down the registers that need to be written
-                            // if we are going to be doing registers individually.
-                            bool write_reg = true;
-                            const uint32_t reg_byte_size = reg_info->byte_size;
-    
-                            const char *restore_src = (const char *)restore_data.PeekData(register_offset, reg_byte_size);
-                            if (restore_src)
-                            {
-                                StreamString packet;
-                                packet.Printf ("P%x=", reg_info->kinds[eRegisterKindProcessPlugin]);
-                                packet.PutBytesAsRawHex8 (restore_src,
-                                                          reg_byte_size,
-                                                          endian::InlHostByteOrder(),
-                                                          endian::InlHostByteOrder());
-
-                                if (thread_suffix_supported)
-                                    packet.Printf (";thread:%4.4" PRIx64 ";", m_thread.GetProtocolID());
-
-                                SetRegisterIsValid(reg, false);
-                                if (gdb_comm.SendPacketAndWaitForResponse(packet.GetString().c_str(),
-                                                                          packet.GetString().size(),
-                                                                          response,
-                                                                          false) == GDBRemoteCommunication::PacketResult::Success)
-                                {
-                                    const char *current_src = (const char *)m_reg_data.PeekData(register_offset, reg_byte_size);
-                                    if (current_src)
-                                        write_reg = memcmp (current_src, restore_src, reg_byte_size) != 0;
-                                }
-    
-                                if (write_reg)
-                                {
-                                    StreamString packet;
-                                    packet.Printf ("P%x=", reg_info->kinds[eRegisterKindProcessPlugin]);
-                                    packet.PutBytesAsRawHex8 (restore_src,
-                                                              reg_byte_size,
-                                                              endian::InlHostByteOrder(),
-                                                              endian::InlHostByteOrder());
-    
-                                    if (thread_suffix_supported)
-                                        packet.Printf (";thread:%4.4" PRIx64 ";", m_thread.GetProtocolID());
-    
-                                    SetRegisterIsValid(reg, false);
-                                    if (gdb_comm.SendPacketAndWaitForResponse(packet.GetString().c_str(),
-                                                                              packet.GetString().size(),
-                                                                              response,
-                                                                              false) == GDBRemoteCommunication::PacketResult::Success)
-                                    {
-                                        if (response.IsOKResponse())
-                                            ++num_restored;
-                                    }
-                                }
-                            }
-                        }
-                        return num_restored > 0;
-                    }
-                }
-            }
-            else
-            {
-                // For the use_g_packet == false case, we're going to write each register 
-                // individually.  The data buffer is binary data in this case, instead of 
-                // ascii characters.
+                    if (gdb_comm.WriteRegister(m_thread.GetProtocolID(), reg_info->kinds[eRegisterKindProcessPlugin],
+                                               llvm::StringRef(restore_src, reg_byte_size)))
+            // For the use_g_packet == false case, we're going to write each register
+            // individually.  The data buffer is binary data in this case, instead of
 
-                bool arm64_debugserver = false;
-                if (m_thread.GetProcess().get())
-                {
-                    const ArchSpec &arch = m_thread.GetProcess()->GetTarget().GetArchitecture();
-                    if (arch.IsValid()
-                        && arch.GetMachine() == llvm::Triple::aarch64
-                        && arch.GetTriple().getVendor() == llvm::Triple::Apple
-                        && arch.GetTriple().getOS() == llvm::Triple::IOS)
-                    {
-                        arm64_debugserver = true;
-                    }
-                }
-                uint32_t num_restored = 0;
-                const RegisterInfo *reg_info;
-                for (uint32_t i = 0; (reg_info = GetRegisterInfoAtIndex (i)) != NULL; i++)
-                {
-                    if (reg_info->value_regs) // skip registers that are slices of real registers
-                        continue;
-                    // Skip the fpsr and fpcr floating point status/control register writing to
-                    // work around a bug in an older version of debugserver that would lead to
-                    // register context corruption when writing fpsr/fpcr.
-                    if (arm64_debugserver &&
-                        (strcmp (reg_info->name, "fpsr") == 0 || strcmp (reg_info->name, "fpcr") == 0))
-                    {
-                        continue;
-                    }
-                    StreamString packet;
-                    packet.Printf ("P%x=", reg_info->kinds[eRegisterKindProcessPlugin]);
-                    packet.PutBytesAsRawHex8 (data_sp->GetBytes() + reg_info->byte_offset, reg_info->byte_size, endian::InlHostByteOrder(), endian::InlHostByteOrder());
-                    if (thread_suffix_supported)
-                        packet.Printf (";thread:%4.4" PRIx64 ";", m_thread.GetProtocolID());
+                if (arch.IsValid() && arch.GetMachine() == llvm::Triple::aarch64 &&
+                    arch.GetTriple().getVendor() == llvm::Triple::Apple &&
+                    arch.GetTriple().getOS() == llvm::Triple::IOS)
+            for (uint32_t i = 0; (reg_info = GetRegisterInfoAtIndex(i)) != NULL; i++)
+                if (arm64_debugserver && (strcmp(reg_info->name, "fpsr") == 0 || strcmp(reg_info->name, "fpcr") == 0))
 
-                    SetRegisterIsValid(reg_info, false);
-                    if (gdb_comm.SendPacketAndWaitForResponse(packet.GetString().c_str(),
-                                                              packet.GetString().size(),
-                                                              response,
-                                                              false) == GDBRemoteCommunication::PacketResult::Success)
-                    {
-                        if (response.IsOKResponse())
-                            ++num_restored;
-                    }
-                }
-                return num_restored > 0;
-            }
-        }
-    }
+                if (gdb_comm.WriteRegister(
+                        m_thread.GetProtocolID(), reg_info->kinds[eRegisterKindProcessPlugin],
+                        llvm::StringRef(reinterpret_cast<const char *>(data_sp->GetBytes() + reg_info->byte_offset),
+                                        reg_info->byte_size)))
     else
     {
         Log *log (ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet (GDBR_LOG_THREAD | GDBR_LOG_PACKETS));
         if (log)
         {
             if (log->GetVerbose())
             {
                 StreamString strm;

unittests/Process/gdb-remote/CMakeLists.txt

 add_lldb_unittest(ProcessGdbRemoteTests
   GDBRemoteClientBaseTest.cpp
+  GDBRemoteCommunicationClientTest.cpp
+  GDBRemoteTestUtils.cpp
   )

unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp

 //===-- GDBRemoteClientBaseTest.cpp -----------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #if defined(_MSC_VER) && (_HAS_EXCEPTIONS == 0)
 // Workaround for MSVC standard library bug, which fails to include <thread> when
 // exceptions are disabled.
 #include <eh.h>
 #endif
 #include <future>
 
+#include "GDBRemoteTestUtils.h"
 #include "gtest/gtest.h"
 
 #include "Plugins/Process/Utility/LinuxSignals.h"
 #include "Plugins/Process/gdb-remote/GDBRemoteClientBase.h"
 #include "Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h"
 
-#include "lldb/Host/common/TCPSocket.h"
-#include "lldb/Host/posix/ConnectionFileDescriptorPosix.h"
-
 #include "llvm/ADT/STLExtras.h"
 
 using namespace lldb_private::process_gdb_remote;
 using namespace lldb_private;
 using namespace lldb;
 typedef GDBRemoteCommunication::PacketResult PacketResult;
 
 namespace
struct MockDelegate : public GDBRemoteClientBase::ContinueDelegate
     }
     void
     HandleStopReply()
     {
         ++stop_reply_called;
     }
 };
 
-struct MockServer : public GDBRemoteCommunicationServer
-{
-    MockServer() : GDBRemoteCommunicationServer("mock-server", "mock-server.listener") { m_send_acks = false; }
-
-    PacketResult
-    SendPacket(llvm::StringRef payload)
-    {
-        return GDBRemoteCommunicationServer::SendPacketNoLock(payload.data(), payload.size());
-    }
-
-    PacketResult
-    GetPacket(StringExtractorGDBRemote &response)
-    {
-        const unsigned timeout_usec = 1000000; // 1s
-        const bool sync_on_timeout = false;
-        return WaitForPacketWithTimeoutMicroSecondsNoLock(response, timeout_usec, sync_on_timeout);
-    }
-};
-
 struct TestClient : public GDBRemoteClientBase
 {
     TestClient() : GDBRemoteClientBase("test.client", "test.client.listener") { m_send_acks = false; }
 };
 
 struct ContinueFixture
 {
     MockDelegate delegate;
WaitForRunEvent()
         EventSP event_sp;
         listener_sp->WaitForEventForBroadcasterWithType(std::chrono::microseconds(0), &client,
                                                         TestClient::eBroadcastBitRunPacketSent, event_sp);
     }
 };
 
 ContinueFixture::ContinueFixture() : listener_sp(Listener::MakeListener("listener"))
 {
-    bool child_processes_inherit = false;
-    Error error;
-    TCPSocket listen_socket(child_processes_inherit, error);
-    EXPECT_FALSE(error.Fail());
-    error = listen_socket.Listen("127.0.0.1:0", 5);
-    EXPECT_FALSE(error.Fail());
-
-    Socket *accept_socket;
-    std::future<Error> accept_error = std::async(std::launch::async, [&] {
-        return listen_socket.Accept("127.0.0.1:0", child_processes_inherit, accept_socket);
-    });
-
-    char connect_remote_address[64];
-    snprintf(connect_remote_address, sizeof(connect_remote_address), "connect://localhost:%u",
-             listen_socket.GetLocalPortNumber());
-
-    std::unique_ptr<ConnectionFileDescriptor> conn_ap(new ConnectionFileDescriptor());
-    EXPECT_EQ(conn_ap->Connect(connect_remote_address, nullptr), lldb::eConnectionStatusSuccess);
-
-    client.SetConnection(conn_ap.release());
-    EXPECT_TRUE(accept_error.get().Success());
-    server.SetConnection(new ConnectionFileDescriptor(accept_socket));
-
+    Connect(client, server);
     listener_sp->StartListeningForEvents(&client, TestClient::eBroadcastBitRunPacketSent);
 }
 
 } // end anonymous namespace
 
-class GDBRemoteClientBaseTest : public testing::Test
+class GDBRemoteClientBaseTest : public GDBRemoteTest
 {
-public:
-    static void
-    SetUpTestCase()
-    {
-#if defined(_MSC_VER)
-        WSADATA data;
-        ::WSAStartup(MAKEWORD(2, 2), &data);
-#endif
-    }
-
-    static void
-    TearDownTestCase()
-    {
-#if defined(_MSC_VER)
-        ::WSACleanup();
-#endif
-    }
 };
 
 TEST_F(GDBRemoteClientBaseTest, SendContinueAndWait)
 {
     StringExtractorGDBRemote response;
     ContinueFixture fix;
     if (HasFailure())
         return;

unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp

+//===-- GDBRemoteCommunicationClientTest.cpp --------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#if defined(_MSC_VER) && (_HAS_EXCEPTIONS == 0)
+// Workaround for MSVC standard library bug, which fails to include <thread> when
+// exceptions are disabled.
+#include <eh.h>
+#endif
+#include <future>
+
+#include "GDBRemoteTestUtils.h"
+#include "gtest/gtest.h"
+
+#include "Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h"
+
+using namespace lldb_private::process_gdb_remote;
+using namespace lldb_private;
+using namespace lldb;
+
+namespace
+{
+
+typedef GDBRemoteCommunication::PacketResult PacketResult;
+
+struct TestClient : public GDBRemoteCommunicationClient
+{
+    TestClient() { m_send_acks = false; }
+};
+
+void
+Handle_QThreadSuffixSupported(MockServer &server, bool supported)
+{
+    StringExtractorGDBRemote request;
+    ASSERT_EQ(PacketResult::Success, server.GetPacket(request));
+    ASSERT_EQ("QThreadSuffixSupported", request.GetStringRef());
+    if (supported)
+        ASSERT_EQ(PacketResult::Success, server.SendOKResponse());
+    else
+        ASSERT_EQ(PacketResult::Success, server.SendUnimplementedResponse(nullptr));
+}
+
+void
+HandlePacket(MockServer &server, llvm::StringRef expected, llvm::StringRef response)
+{
+    StringExtractorGDBRemote request;
+    ASSERT_EQ(PacketResult::Success, server.GetPacket(request));
+    ASSERT_EQ(expected, request.GetStringRef());
+    ASSERT_EQ(PacketResult::Success, server.SendPacket(response));
+}
+
+} // end anonymous namespace
+
+class GDBRemoteCommunicationClientTest : public GDBRemoteTest
+{
+};
+
+TEST_F(GDBRemoteCommunicationClientTest, WriteRegister)
+{
+    TestClient client;
+    MockServer server;
+    Connect(client, server);
+    if (HasFailure())
+        return;
+
+    const lldb::tid_t tid = 0x47;
+    const uint32_t reg_num = 4;
+    std::future<bool> write_result =
+        std::async(std::launch::async, [&client] { return client.WriteRegister(tid, reg_num, "ABCD"); });
+
+    Handle_QThreadSuffixSupported(server, true);
+
+    HandlePacket(server, "P4=41424344;thread:0047;", "OK");
+    ASSERT_TRUE(write_result.get());
+
+    write_result = std::async(std::launch::async,
+                              [&client] { return client.WriteAllRegisters(tid, "404142434445464748494a4b4c4d4e4f"); });
+
+    HandlePacket(server, "G404142434445464748494a4b4c4d4e4f;thread:0047;", "OK");
+    ASSERT_TRUE(write_result.get());
+}
+
+TEST_F(GDBRemoteCommunicationClientTest, WriteRegisterNoSuffix)
+{
+    TestClient client;
+    MockServer server;
+    Connect(client, server);
+    if (HasFailure())
+        return;
+
+    const lldb::tid_t tid = 0x47;
+    const uint32_t reg_num = 4;
+    std::future<bool> write_result =
+        std::async(std::launch::async, [&client] { return client.WriteRegister(tid, reg_num, "ABCD"); });
+
+    Handle_QThreadSuffixSupported(server, false);
+    HandlePacket(server, "Hg47", "OK");
+    HandlePacket(server, "P4=41424344", "OK");
+    ASSERT_TRUE(write_result.get());
+
+    write_result = std::async(std::launch::async,
+                              [&client] { return client.WriteAllRegisters(tid, "404142434445464748494a4b4c4d4e4f"); });
+
+    HandlePacket(server, "G404142434445464748494a4b4c4d4e4f", "OK");
+    ASSERT_TRUE(write_result.get());
+}

unittests/Process/gdb-remote/GDBRemoteTestUtils.h

+//===-- GDBRemoteTestUtils.h ------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+#ifndef lldb_unittests_Process_gdb_remote_GDBRemoteTestUtils_h
+#define lldb_unittests_Process_gdb_remote_GDBRemoteTestUtils_h
+
+#include "Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h"
+
+namespace lldb_private
+{
+namespace process_gdb_remote
+{
+
+class GDBRemoteTest : public testing::Test
+{
+public:
+    static void
+    SetUpTestCase();
+
+    static void
+    TearDownTestCase();
+};
+
+void
+Connect(GDBRemoteCommunication &client, GDBRemoteCommunication &server);
+
+struct MockServer : public GDBRemoteCommunicationServer
+{
+    MockServer() : GDBRemoteCommunicationServer("mock-server", "mock-server.listener") { m_send_acks = false; }
+
+    PacketResult
+    SendPacket(llvm::StringRef payload)
+    {
+        return GDBRemoteCommunicationServer::SendPacketNoLock(payload.data(), payload.size());
+    }
+
+    PacketResult
+    GetPacket(StringExtractorGDBRemote &response)
+    {
+        const unsigned timeout_usec = 1000000; // 1s
+        const bool sync_on_timeout = false;
+        return WaitForPacketWithTimeoutMicroSecondsNoLock(response, timeout_usec, sync_on_timeout);
+    }
+
+    using GDBRemoteCommunicationServer::SendOKResponse;
+    using GDBRemoteCommunicationServer::SendUnimplementedResponse;
+};
+
+} // namespace process_gdb_remote
+} // namespace lldb_private
+
+#endif // lldb_unittests_Process_gdb_remote_GDBRemoteTestUtils_h

unittests/Process/gdb-remote/GDBRemoteTestUtils.cpp

+//===-- GDBRemoteTestUtils.cpp ----------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#if defined(_MSC_VER) && (_HAS_EXCEPTIONS == 0)
+// Workaround for MSVC standard library bug, which fails to include <thread> when
+// exceptions are disabled.
+#include <eh.h>
+#endif
+
+#include "GDBRemoteTestUtils.h"
+#include "gtest/gtest.h"
+
+#include "lldb/Host/common/TCPSocket.h"
+#include "lldb/Host/posix/ConnectionFileDescriptorPosix.h"
+
+#include <future>
+
+namespace lldb_private
+{
+namespace process_gdb_remote
+{
+
+void
+GDBRemoteTest::SetUpTestCase()
+{
+#if defined(_MSC_VER)
+    WSADATA data;
+    ::WSAStartup(MAKEWORD(2, 2), &data);
+#endif
+}
+
+void
+GDBRemoteTest::TearDownTestCase()
+{
+#if defined(_MSC_VER)
+    ::WSACleanup();
+#endif
+}
+
+void
+Connect(GDBRemoteCommunication &client, GDBRemoteCommunication &server)
+{
+    bool child_processes_inherit = false;
+    Error error;
+    TCPSocket listen_socket(child_processes_inherit, error);
+    ASSERT_FALSE(error.Fail());
+    error = listen_socket.Listen("127.0.0.1:0", 5);
+    ASSERT_FALSE(error.Fail());
+
+    Socket *accept_socket;
+    std::future<Error> accept_error = std::async(std::launch::async, [&] {
+        return listen_socket.Accept("127.0.0.1:0", child_processes_inherit, accept_socket);
+    });
+
+    char connect_remote_address[64];
+    snprintf(connect_remote_address, sizeof(connect_remote_address), "connect://localhost:%u",
+             listen_socket.GetLocalPortNumber());
+
+    std::unique_ptr<ConnectionFileDescriptor> conn_ap(new ConnectionFileDescriptor());
+    ASSERT_EQ(conn_ap->Connect(connect_remote_address, nullptr), lldb::eConnectionStatusSuccess);
+
+    client.SetConnection(conn_ap.release());
+    ASSERT_TRUE(accept_error.get().Success());
+    server.SetConnection(new ConnectionFileDescriptor(accept_socket));
+}
+
+} // namespace process_gdb_remote
+} // namespace lldb_private