[lldb-server] Verify 'g' is supported before relying on them

[guiandrade]

Summary

This ensures that 'g' packets are only used if the config
plugin.process.gdb-remote.use-g-packet-for-reading = true and
we get a normal packet in response to a 'g' packet.

[jasonmolenda]

Hi, sorry I haven't had time to get back to this one. I wanted to look into the m_avoid_g_packets ivar in GDBRemoteCommunicationClient and GDBRemoteCommunicationClient::AvoidGPackets. It looks like it was written to specifically avoid the g packet with arm64 devices, and I think it's trying to avoid the g packet for older debugservers.

The last time we enabled this, the macos bots immediately started getting failures -- I suspect there is a bug with debugserver's g packet right now, which we hadn't noticed because we don't use the g packet on this platform (we pre-seed all of the GPR values so it's not a perf problem). I need to spend a little time looking into what's going on there --- but we'll probably need to make sure GDBRemoteCommunicationClient::AvoidGPackets is called / used, and that it is updated to avoid some more debugservers. :)

lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteClient.py

def test_read_registers_using_g_packets(self):
         self.dbg.HandleCommand(
                 "settings set plugin.process.gdb-remote.use-g-packet-for-reading true")
         self.addTearDownHook(lambda: 
                 self.runCmd("settings set plugin.process.gdb-remote.use-g-packet-for-reading false"))
         self.server.responder = self.gPacketResponder()
         target = self.createTarget("a.yaml")
         process = self.connect(target)
 
-        self.assertEquals(1, self.server.responder.packetLog.count("g"))
+        # 1 packet used to verify that 'g' is supported + 1 to fetch the registers.
+        self.assertEquals(2, self.server.responder.packetLog.count("g"))
         self.server.responder.packetLog = []
         self.read_registers(process)
         # Reading registers should not cause any 'p' packets to be exchanged.
         self.assertEquals(
                 0, len([p for p in self.server.responder.packetLog if p.startswith("p")]))
 
     def test_read_registers_using_p_packets(self):
         """Test reading registers using 'p' packets"""

lldb/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestNoGPacketSupported.py

def test(self):
                             <reg name="rbx" bitsize="64" regnum="1" type="code_ptr" group="general"/>
                             <reg name="rsi" bitsize="64" regnum="2" type="code_ptr" group="general"/>
                             <reg name="rip" bitsize="64" regnum="3" type="code_ptr" group="general" altname="pc" generic="pc"/>
                           </feature>
                         </target>""", False
                 else:
                     return None, False
 
+        self.dbg.HandleCommand(
+                "settings set plugin.process.gdb-remote.use-g-packet-for-reading true")
+        self.addTearDownHook(lambda:
+                self.runCmd("settings set plugin.process.gdb-remote.use-g-packet-for-reading false"))
         self.server.responder = MyResponder()
         target = self.dbg.CreateTarget('')
-        self.runCmd("log enable gdb-remote packets")
         if self.TraceOn():
           self.runCmd("log enable gdb-remote packets")
           self.addTearDownHook(
                 lambda: self.runCmd("log disable gdb-remote packets"))
         process = self.connect(target)
 
         thread = process.GetThreadAtIndex(0)
         frame = thread.GetFrameAtIndex(0)

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

public:
   const ArchSpec &GetProcessArchitecture();
 
   void GetRemoteQSupported();
 
   bool GetVContSupported(char flavor);
 
   bool GetpPacketSupported(lldb::tid_t tid);
 
+  bool GetgPacketSupported(lldb::tid_t tid);
+
   bool GetxPacketSupported();
 
   bool GetVAttachOrWaitSupported();
 
   bool GetSyncThreadStateSupported();
 
   void ResetDiscoverableSettings(bool did_exec);
 
protected:
   LazyBool m_supports_alloc_dealloc_memory;
   LazyBool m_supports_memory_region_info;
   LazyBool m_supports_watchpoint_support_info;
   LazyBool m_supports_detach_stay_stopped;
   LazyBool m_watchpoints_trigger_after_instruction;
   LazyBool m_attach_or_wait_reply;
   LazyBool m_prepare_for_reg_writing_reply;
   LazyBool m_supports_p;
+  LazyBool m_supports_g;
   LazyBool m_supports_x;
   LazyBool m_avoid_g_packets;
   LazyBool m_supports_QSaveRegisterState;
   LazyBool m_supports_qXfer_auxv_read;
   LazyBool m_supports_qXfer_libraries_read;
   LazyBool m_supports_qXfer_libraries_svr4_read;
   LazyBool m_supports_qXfer_features_read;
   LazyBool m_supports_qXfer_memory_map_read;

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

: GDBRemoteClientBase("gdb-remote.client", "gdb-remote.client.rx_packet"),
       m_qGDBServerVersion_is_valid(eLazyBoolCalculate),
       m_supports_alloc_dealloc_memory(eLazyBoolCalculate),
       m_supports_memory_region_info(eLazyBoolCalculate),
       m_supports_watchpoint_support_info(eLazyBoolCalculate),
       m_supports_detach_stay_stopped(eLazyBoolCalculate),
       m_watchpoints_trigger_after_instruction(eLazyBoolCalculate),
       m_attach_or_wait_reply(eLazyBoolCalculate),
       m_prepare_for_reg_writing_reply(eLazyBoolCalculate),
-      m_supports_p(eLazyBoolCalculate), m_supports_x(eLazyBoolCalculate),
-      m_avoid_g_packets(eLazyBoolCalculate),
+      m_supports_p(eLazyBoolCalculate), m_supports_g(eLazyBoolCalculate),
+      m_supports_x(eLazyBoolCalculate), m_avoid_g_packets(eLazyBoolCalculate),
       m_supports_QSaveRegisterState(eLazyBoolCalculate),
       m_supports_qXfer_auxv_read(eLazyBoolCalculate),
       m_supports_qXfer_libraries_read(eLazyBoolCalculate),
       m_supports_qXfer_libraries_svr4_read(eLazyBoolCalculate),
       m_supports_qXfer_features_read(eLazyBoolCalculate),
       m_supports_qXfer_memory_map_read(eLazyBoolCalculate),
       m_supports_augmented_libraries_svr4_read(eLazyBoolCalculate),
       m_supports_jThreadExtendedInfo(eLazyBoolCalculate),

 //
 // Takes a valid thread ID because p needs to apply to a thread.
 bool GDBRemoteCommunicationClient::GetpPacketSupported(lldb::tid_t tid) {
   if (m_supports_p == eLazyBoolCalculate)
     m_supports_p = GetThreadPacketSupported(tid, "p0");
   return m_supports_p;
 }
 
+bool GDBRemoteCommunicationClient::GetgPacketSupported(lldb::tid_t tid) {
+  if (m_supports_g == eLazyBoolCalculate)
+    m_supports_g = GetThreadPacketSupported(tid, "g");
+  return m_supports_g;
+}
+
 LazyBool GDBRemoteCommunicationClient::GetThreadPacketSupported(
     lldb::tid_t tid, llvm::StringRef packetStr) {
   StreamString payload;
   payload.PutCString(packetStr);
   StringExtractorGDBRemote response;
   if (SendThreadSpecificPacketAndWaitForResponse(
           tid, std::move(payload), response, false) == PacketResult::Success &&
       response.IsNormalResponse()) {

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

ThreadGDBRemote::CreateRegisterContextForFrame(StackFrame *frame) {
   if (concrete_frame_idx == 0) {
     ProcessSP process_sp(GetProcess());
     if (process_sp) {
       ProcessGDBRemote *gdb_process =
           static_cast<ProcessGDBRemote *>(process_sp.get());
       bool pSupported =
           gdb_process->GetGDBRemote().GetpPacketSupported(GetID());
       bool read_all_registers_at_once =
-          !pSupported || gdb_process->m_use_g_packet_for_reading;
+          !pSupported ||
+          (gdb_process->m_use_g_packet_for_reading &&
+           gdb_process->GetGDBRemote().GetgPacketSupported(GetID()));
       bool write_all_registers_at_once = !pSupported;
       reg_ctx_sp = std::make_shared<GDBRemoteRegisterContext>(
           *this, concrete_frame_idx, gdb_process->m_register_info,
           read_all_registers_at_once, write_all_registers_at_once);
     }
   } else {
     Unwind *unwinder = GetUnwinder();
     if (unwinder != nullptr)