[LLDB] Support GDB remote g packet partial read

[omjavaid]

Summary

GDB remote protocol does not specify length of g packet for register read. It depends on remote to include all or exclude certain registers from g packet. In case a register or set of registers is not included as part of g packet then we should fall back to p packet for reading all registers excluded from g packet by remote. This patch adds support for above feature and adds a test-case for the same.

[labath]

Seems reasonable. I think the names like m_read_all_at_once and ReadAllRegisters will now be somewhat confusing. Maybe it would be worth (in a separate patch) renaming them to something which does not include "all" in the name.
Maybe we could use m_use_g_packet_for_reading/writing instead of m_read/write_all_at_once. I'm not sure what would be a good name for ReadAllRegisters... Gdb docs describe the g packet as "Read general registers" so maybe just ReadGeneralRegisters with some comment that this may in fact include all of them?

lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.h

 //===-- GDBRemoteRegisterContext.h ------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_SOURCE_PLUGINS_PROCESS_GDB_REMOTE_GDBREMOTEREGISTERCONTEXT_H
 #define LLDB_SOURCE_PLUGINS_PROCESS_GDB_REMOTE_GDBREMOTEREGISTERCONTEXT_H
 
 #include <vector>
 
 #include "Plugins/Process/Utility/DynamicRegisterInfo.h"
 #include "lldb/Target/RegisterContext.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/DataExtractor.h"
 #include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-private.h"
 
 #include "GDBRemoteCommunicationClient.h"
 
if (reg < m_reg_valid.size())
       m_reg_valid[reg] = valid;
   }
 
   GDBRemoteDynamicRegisterInfoSP m_reg_info_sp;
   std::vector<bool> m_reg_valid;
   DataExtractor m_reg_data;
   bool m_read_all_at_once;
   bool m_write_all_at_once;
+  bool m_gpacket_cached;
 
 private:
   // Helper function for ReadRegisterBytes().
   bool GetPrimordialRegister(const RegisterInfo *reg_info,
                              GDBRemoteCommunicationClient &gdb_comm);
   // Helper function for WriteRegisterBytes().
   bool SetPrimordialRegister(const RegisterInfo *reg_info,
                              GDBRemoteCommunicationClient &gdb_comm);

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

 // GDBRemoteRegisterContext constructor
 GDBRemoteRegisterContext::GDBRemoteRegisterContext(
     ThreadGDBRemote &thread, uint32_t concrete_frame_idx,
     GDBRemoteDynamicRegisterInfoSP reg_info_sp, bool read_all_at_once,
     bool write_all_at_once)
     : RegisterContext(thread, concrete_frame_idx),
       m_reg_info_sp(std::move(reg_info_sp)), m_reg_valid(), m_reg_data(),
       m_read_all_at_once(read_all_at_once),
-      m_write_all_at_once(write_all_at_once) {
+      m_write_all_at_once(write_all_at_once), m_gpacket_cached(false) {
   // Resize our vector of bools to contain one bool for every register. We will
   // use these boolean values to know when a register value is valid in
   // m_reg_data.
   m_reg_valid.resize(m_reg_info_sp->GetNumRegisters());
 
   // Make a heap based buffer that is big enough to store all registers
   DataBufferSP reg_data_sp(
       new DataBufferHeap(m_reg_info_sp->GetRegisterDataByteSize(), 0));
   m_reg_data.SetData(reg_data_sp);
   m_reg_data.SetByteOrder(thread.GetProcess()->GetByteOrder());
 }
 
 // Destructor
 GDBRemoteRegisterContext::~GDBRemoteRegisterContext() {}
 
 void GDBRemoteRegisterContext::InvalidateAllRegisters() {
   SetAllRegisterValid(false);
 }
 
 void GDBRemoteRegisterContext::SetAllRegisterValid(bool b) {
+  m_gpacket_cached = b;
   std::vector<bool>::iterator pos, end = m_reg_valid.end();
   for (pos = m_reg_valid.begin(); pos != end; ++pos)
     *pos = b;
 }
 
 size_t GDBRemoteRegisterContext::GetRegisterCount() {
   return m_reg_info_sp->GetNumRegisters();
 }
bool GDBRemoteRegisterContext::ReadRegisterBytes(const RegisterInfo *reg_info,
   GDBRemoteCommunicationClient &gdb_comm(
       ((ProcessGDBRemote *)process)->GetGDBRemote());
 
   InvalidateIfNeeded(false);
 
   const uint32_t reg = reg_info->kinds[eRegisterKindLLDB];
 
   if (!GetRegisterIsValid(reg)) {
-    if (m_read_all_at_once) {
+    if (m_read_all_at_once && !m_gpacket_cached) {
       if (DataBufferSP buffer_sp =
               gdb_comm.ReadAllRegisters(m_thread.GetProtocolID())) {
         memcpy(const_cast<uint8_t *>(m_reg_data.GetDataStart()),
                buffer_sp->GetBytes(),
                std::min(buffer_sp->GetByteSize(), m_reg_data.GetByteSize()));
         if (buffer_sp->GetByteSize() >= m_reg_data.GetByteSize()) {
           SetAllRegisterValid(true);
           return true;
         } else if (buffer_sp->GetByteSize() > 0) {
           const int regcount = m_reg_info_sp->GetNumRegisters();
           for (int i = 0; i < regcount; i++) {
             struct RegisterInfo *reginfo =
                 m_reg_info_sp->GetRegisterInfoAtIndex(i);
             if (reginfo->byte_offset + reginfo->byte_size <=
                 buffer_sp->GetByteSize()) {
               m_reg_valid[i] = true;
             } else {
               m_reg_valid[i] = false;
             }
           }
+
+          m_gpacket_cached = true;
+          if (GetRegisterIsValid(reg))
             return true;
         } else {
           Log *log(ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet(GDBR_LOG_THREAD |
                                                                 GDBR_LOG_PACKETS));
           LLDB_LOGF(
               log,
               "error: GDBRemoteRegisterContext::ReadRegisterBytes tried "
               "to read the "
               "entire register context at once, expected at least %" PRId64
               " bytes "
               "but only got %" PRId64 " bytes.",
               m_reg_data.GetByteSize(), buffer_sp->GetByteSize());
+          return false;
         }
       }
-      return false;
     }
     if (reg_info->value_regs) {
       // Process this composite register request by delegating to the
       // constituent primordial registers.
 
       // Index of the primordial register.
       bool success = true;
       for (uint32_t idx = 0; success; ++idx) {

lldb/test/API/functionalities/gdb_remote_client/TestPartialGPacket.py

+from __future__ import print_function
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from gdbclientutils import *
+
+
+class TestPartialGPacket(GDBRemoteTestBase):
+
+    @skipIfXmlSupportMissing
+    @skipIfRemote
+    def test(self):
+        """
+        Test GDB remote fallback to 'p' packet when 'g' packet does not include all registers.
+        """
+        class MyResponder(MockGDBServerResponder):
+
+            def qXferRead(self, obj, annex, offset, length):
+                if annex == "target.xml":
+                    return """<?xml version="1.0"?>
+                        <!DOCTYPE feature SYSTEM "gdb-target.dtd">
+                        <target>
+                        <architecture>arm</architecture>
+                        <feature name="org.gnu.gdb.arm.m-profile">
+                        <reg name="r0" bitsize="32" type="uint32" group="general"/>
+                        <reg name="r1" bitsize="32" type="uint32" group="general"/>
+                        <reg name="r2" bitsize="32" type="uint32" group="general"/>
+                        <reg name="r3" bitsize="32" type="uint32" group="general"/>
+                        <reg name="r4" bitsize="32" type="uint32" group="general"/>
+                        <reg name="r5" bitsize="32" type="uint32" group="general"/>
+                        <reg name="r6" bitsize="32" type="uint32" group="general"/>
+                        <reg name="r7" bitsize="32" type="uint32" group="general"/>
+                        <reg name="r8" bitsize="32" type="uint32" group="general"/>
+                        <reg name="r9" bitsize="32" type="uint32" group="general"/>
+                        <reg name="r10" bitsize="32" type="uint32" group="general"/>
+                        <reg name="r11" bitsize="32" type="uint32" group="general"/>
+                        <reg name="r12" bitsize="32" type="uint32" group="general"/>
+                        <reg name="sp" bitsize="32" type="data_ptr" group="general"/>
+                        <reg name="lr" bitsize="32" type="uint32" group="general"/>
+                        <reg name="pc" bitsize="32" type="code_ptr" group="general"/>
+                        <reg name="xpsr" bitsize="32" regnum="25" type="uint32" group="general"/>
+                        <reg name="MSP" bitsize="32" regnum="26" type="uint32" group="general"/>
+                        <reg name="PSP" bitsize="32" regnum="27" type="uint32" group="general"/>
+                        <reg name="PRIMASK" bitsize="32" regnum="28" type="uint32" group="general"/>
+                        <reg name="BASEPRI" bitsize="32" regnum="29" type="uint32" group="general"/>
+                        <reg name="FAULTMASK" bitsize="32" regnum="30" type="uint32" group="general"/>
+                        <reg name="CONTROL" bitsize="32" regnum="31" type="uint32" group="general"/>
+                        </feature>
+                        </target>""", False
+                else:
+                    return None, False
+
+            def readRegister(self, regnum):
+                if regnum == 31:
+                    return "cdcc8c3f00000000"
+                return "E01"
+
+            def readRegisters(self):
+                return "20000000f8360020001000002fcb0008f8360020a0360020200c0020000000000000000000000000000000000000000000000000b87f0120b7d100082ed2000800000001"
+
+            def haltReason(self):
+                return "S05"
+
+            def qfThreadInfo(self):
+                return "mdead"
+
+            def qC(self):
+                return ""
+
+            def qSupported(self, client_supported):
+                return "PacketSize=4000;qXfer:memory-map:read-;QStartNoAckMode+;qXfer:threads:read+;hwbreak+;qXfer:features:read+"
+
+            def QThreadSuffixSupported(self):
+                return "OK"
+
+            def QListThreadsInStopReply(self):
+                return "OK"
+
+        self.server.responder = MyResponder()
+        if self.TraceOn():
+            self.runCmd("log enable gdb-remote packets")
+            self.addTearDownHook(
+                lambda: self.runCmd("log disable gdb-remote packets"))
+
+        self.dbg.SetDefaultArchitecture("armv7em")
+        target = self.dbg.CreateTargetWithFileAndArch(None, None)
+
+        process = self.connect(target)
+
+        if self.TraceOn():
+            interp = self.dbg.GetCommandInterpreter()
+            result = lldb.SBCommandReturnObject()
+            interp.HandleCommand("target list", result)
+            print(result.GetOutput())
+
+        r0_valobj = process.GetThreadAtIndex(
+            0).GetFrameAtIndex(0).FindRegister("r0")
+        self.assertEqual(r0_valobj.GetValueAsUnsigned(), 0x20)
+
+        pc_valobj = process.GetThreadAtIndex(
+            0).GetFrameAtIndex(0).FindRegister("pc")
+        self.assertEqual(pc_valobj.GetValueAsUnsigned(), 0x0800d22e)
+
+        pc_valobj = process.GetThreadAtIndex(
+            0).GetFrameAtIndex(0).FindRegister("CONTROL")
+        self.assertEqual(pc_valobj.GetValueAsUnsigned(), 0x3f8ccccd)