Prevent client from querying each thread's PC at each stop.

[jmajors]

Summary

The server was no longer sending the thread PCs the way the client
expected them.
I changed the server to send them back as a threadstop info field,
similar to the Apple version of the server.
I also changed the client to look for them there, before querying the
server.
I added a test to ensure the server doesn't stop sending them.

[labath]

Thank you for writing the test. I have a couple of cleanup suggestions. Let me know what you think.

Also, for the future, please upload a new version of the patch under the same differential revision as the previous one (so one can compare the changes, keep all the subscribers, etc.). If you're submitting using arcanist, it should amend your git commit to include the "differential revision" blurb. Then subsequent "arc diff"s should update automatically.

packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteThreadsInStopReply.py

86 ↗

(On Diff #84912)

This makes the figuring out the return type of the function quite confusing. How about just keeping the "tuple" branch, adjusting callers (there's only 2 of them) to call the function with a tuple (possibly containing only one element)?
Maybe also rename the function to _field*s*, as it can return multiple items.

101 ↗

(On Diff #84912)

this would become:

stop_reply_threads_text = self.gather_stop_reply_fields( post_startup_log_lines, thread_count, ("threads"))["threads"]

119 ↗

(On Diff #84912)

Shouldn't we just assert here?

298 ↗

(On Diff #84912)

This should be @debugserver_test

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

1774 ↗

(On Diff #84912)

auto iter = std::find(m_thread_ids.begin(), m_thread_ids.end(), tid);
if (iter != m_thread_ids.end())
  SetThreadPc(thread_sp, iter - m_thread_ids.begin());

[jmajors]

Addressed feedback.

[labath]

Looks good. Thank you.

I am going to do the array conversion and commit this (if I am not mistaken, you don't have commit access).

packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteThreadsInStopReply.py

98 ↗

(On Diff #85075)

Ok, this should probably be an array instead of a tuple ["threads"]. Then we can avoid the funny-looking comma.

lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteThreadsInStopReply.py

 from __future__ import print_function
 
+import json
+import re
+
 import gdbremote_testcase
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
 
-
 class TestGdbRemoteThreadsInStopReply(
         gdbremote_testcase.GdbRemoteTestCaseBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
     ENABLE_THREADS_IN_STOP_REPLY_ENTRIES = [
         "read packet: $QListThreadsInStopReply#21",
         "send packet: $OK#00",
     ]
 
-    def gather_stop_reply_threads(self, post_startup_log_lines, thread_count):
+    def gather_stop_reply_fields(self, post_startup_log_lines, thread_count,
+            field_names):
         # Set up the inferior args.
         inferior_args = []
         for i in range(thread_count - 1):
             inferior_args.append("thread:new")
         inferior_args.append("sleep:10")
         procs = self.prep_debug_monitor_and_inferior(
             inferior_args=inferior_args)
 
+        self.add_register_info_collection_packets()
+        self.add_process_info_collection_packets()
+
         # Assumes test_sequence has anything added needed to setup the initial state.
         # (Like optionally enabling QThreadsInStopReply.)
         if post_startup_log_lines:
             self.test_sequence.add_log_lines(post_startup_log_lines, True)
         self.test_sequence.add_log_lines([
             "read packet: $c#63"
         ], True)
         context = self.expect_gdbremote_sequence()
         self.assertIsNotNone(context)
+        hw_info = self.parse_hw_info(context)
 
         # Give threads time to start up, then break.
         time.sleep(1)
         self.reset_test_sequence()
         self.test_sequence.add_log_lines(
             [
                 "read packet: {}".format(
                     chr(3)),
def gather_stop_reply_fields(self, post_startup_log_lines, thread_count,
         self.assertIsNotNone(context)
 
         # Parse the stop reply contents.
         key_vals_text = context.get("key_vals_text")
         self.assertIsNotNone(key_vals_text)
         kv_dict = self.parse_key_val_dict(key_vals_text)
         self.assertIsNotNone(kv_dict)
 
+        result = dict();
+        result["pc_register"] = hw_info["pc_register"]
+        result["little_endian"] = hw_info["little_endian"]
+        for key_field in field_names:
+            result[key_field] = kv_dict.get(key_field)
+
+        return result
+
+    def gather_stop_reply_threads(self, post_startup_log_lines, thread_count):
         # Pull out threads from stop response.
-        stop_reply_threads_text = kv_dict.get("threads")
+        stop_reply_threads_text = self.gather_stop_reply_fields(
+                post_startup_log_lines, thread_count, ["threads"])["threads"]
         if stop_reply_threads_text:
             return [int(thread_id, 16)
                     for thread_id in stop_reply_threads_text.split(",")]
         else:
             return []
 
+    def gather_stop_reply_pcs(self, post_startup_log_lines, thread_count):
+        results = self.gather_stop_reply_fields( post_startup_log_lines,
+                thread_count, ["threads", "thread-pcs"])
+        if not results:
+            return []
+
+        threads_text = results["threads"]
+        pcs_text = results["thread-pcs"]
+        thread_ids = threads_text.split(",")
+        pcs = pcs_text.split(",")
+        self.assertTrue(len(thread_ids) == len(pcs))
+
+        thread_pcs = dict()
+        for i in range(0, len(pcs)):
+            thread_pcs[int(thread_ids[i], 16)] = pcs[i]
+
+        result = dict()
+        result["thread_pcs"] = thread_pcs
+        result["pc_register"] = results["pc_register"]
+        result["little_endian"] = results["little_endian"]
+        return result
+
+    def switch_endian(self, egg):
+        return "".join(reversed(re.findall("..", egg)))
+
+    def parse_hw_info(self, context):
+        self.assertIsNotNone(context)
+        process_info = self.parse_process_info_response(context)
+        endian = process_info.get("endian")
+        reg_info = self.parse_register_info_packets(context)
+        (pc_lldb_reg_index, pc_reg_info) = self.find_pc_reg_info(reg_info)
+
+        hw_info = dict()
+        hw_info["pc_register"] = pc_lldb_reg_index
+        hw_info["little_endian"] = (endian == "little")
+        return hw_info
+
+    def gather_threads_info_pcs(self, pc_register, little_endian):
+        self.reset_test_sequence()
+        self.test_sequence.add_log_lines(
+                [
+                    "read packet: $jThreadsInfo#c1",
+                    {
+                        "direction": "send",
+                        "regex": r"^\$(.*)#[0-9a-fA-F]{2}$",
+                        "capture": {
+                            1: "threads_info"}},
+                ],
+                True)
+
+        context = self.expect_gdbremote_sequence()
+        self.assertIsNotNone(context)
+        threads_info = context.get("threads_info")
+        register = str(pc_register)
+        # The jThreadsInfo response is not valid JSON data, so we have to
+        # clean it up first.
+        jthreads_info = json.loads(re.sub(r"}]", "}", threads_info))
+        thread_pcs = dict()
+        for thread_info in jthreads_info:
+            tid = thread_info["tid"]
+            pc = thread_info["registers"][register]
+            thread_pcs[tid] = self.switch_endian(pc) if little_endian else pc
+
+        return thread_pcs
+
     def QListThreadsInStopReply_supported(self):
         procs = self.prep_debug_monitor_and_inferior()
         self.test_sequence.add_log_lines(
             self.ENABLE_THREADS_IN_STOP_REPLY_ENTRIES, True)
 
         context = self.expect_gdbremote_sequence()
         self.assertIsNotNone(context)
 
def test_stop_reply_reports_correct_threads_debugserver(self):
         self.stop_reply_reports_correct_threads(5)
 
     @llgs_test
     def test_stop_reply_reports_correct_threads_llgs(self):
         self.init_llgs_test()
         self.build()
         self.set_inferior_startup_launch()
         self.stop_reply_reports_correct_threads(5)
+
+    def stop_reply_contains_thread_pcs(self, thread_count):
+        results = self.gather_stop_reply_pcs(
+                self.ENABLE_THREADS_IN_STOP_REPLY_ENTRIES, thread_count)
+        stop_reply_pcs = results["thread_pcs"]
+        pc_register = results["pc_register"]
+        little_endian = results["little_endian"]
+        self.assertEqual(len(stop_reply_pcs), thread_count)
+
+        threads_info_pcs = self.gather_threads_info_pcs(pc_register,
+                little_endian)
+
+        self.assertEqual(len(threads_info_pcs), thread_count)
+        for thread_id in stop_reply_pcs:
+            self.assertTrue(thread_id in threads_info_pcs)
+            self.assertTrue(int(stop_reply_pcs[thread_id], 16)
+                    == int(threads_info_pcs[thread_id], 16))
+
+    @llgs_test
+    def test_stop_reply_contains_thread_pcs_llgs(self):
+        self.init_llgs_test()
+        self.build()
+        self.set_inferior_startup_launch()
+        self.stop_reply_contains_thread_pcs(5)
+
+    @debugserver_test
+    def test_stop_reply_contains_thread_pcs_debugserver(self):
+        self.init_debugserver_test()
+        self.build()
+        self.set_inferior_startup_launch()
+        self.stop_reply_contains_thread_pcs(5)

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

static void AppendHexValue(StreamString &response, const uint8_t *buf,
   } else {
     for (i = 0; i < buf_size; i++)
       response.PutHex8(buf[i]);
   }
 }
 
 static void WriteRegisterValueInHexFixedWidth(
     StreamString &response, NativeRegisterContextSP &reg_ctx_sp,
-    const RegisterInfo &reg_info, const RegisterValue *reg_value_p) {
+    const RegisterInfo &reg_info, const RegisterValue *reg_value_p,
+    lldb::ByteOrder byte_order) {
   RegisterValue reg_value;
   if (!reg_value_p) {
     Error error = reg_ctx_sp->ReadRegister(&reg_info, reg_value);
     if (error.Success())
       reg_value_p = &reg_value;
     // else log.
   }
 
   if (reg_value_p) {
     AppendHexValue(response, (const uint8_t *)reg_value_p->GetBytes(),
-                   reg_value_p->GetByteSize(), false);
+                   reg_value_p->GetByteSize(),
+                   byte_order == lldb::eByteOrderLittle);
   } else {
     // Zero-out any unreadable values.
     if (reg_info.byte_size > 0) {
       std::basic_string<uint8_t> zeros(reg_info.byte_size, '\0');
       AppendHexValue(response, zeros.data(), zeros.size(), false);
     }
   }
 }
 
-static JSONObject::SP GetRegistersAsJSON(NativeThreadProtocol &thread,
-                                         bool abridged) {
+static JSONObject::SP GetRegistersAsJSON(NativeThreadProtocol &thread) {
   Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_THREAD));
 
   NativeRegisterContextSP reg_ctx_sp = thread.GetRegisterContext();
   if (!reg_ctx_sp)
     return nullptr;
 
   JSONObject::SP register_object_sp = std::make_shared<JSONObject>();
 
 #ifdef LLDB_JTHREADSINFO_FULL_REGISTER_SET
   // Expedite all registers in the first register set (i.e. should be GPRs) that
   // are not contained in other registers.
   const RegisterSet *reg_set_p = reg_ctx_sp->GetRegisterSet(0);
   if (!reg_set_p)
     return nullptr;
   for (const uint32_t *reg_num_p = reg_set_p->registers;
        *reg_num_p != LLDB_INVALID_REGNUM; ++reg_num_p) {
     uint32_t reg_num = *reg_num_p;
 #else
   // Expedite only a couple of registers until we figure out why sending
   // registers is
   // expensive.
   static const uint32_t k_expedited_registers[] = {
       LLDB_REGNUM_GENERIC_PC, LLDB_REGNUM_GENERIC_SP, LLDB_REGNUM_GENERIC_FP,
       LLDB_REGNUM_GENERIC_RA, LLDB_INVALID_REGNUM};
-  static const uint32_t k_abridged_expedited_registers[] = {
-      LLDB_REGNUM_GENERIC_PC, LLDB_INVALID_REGNUM};
 
-  for (const uint32_t *generic_reg_p = abridged ? k_abridged_expedited_registers
-                                                : k_expedited_registers;
+  for (const uint32_t *generic_reg_p = k_expedited_registers;
        *generic_reg_p != LLDB_INVALID_REGNUM; ++generic_reg_p) {
     uint32_t reg_num = reg_ctx_sp->ConvertRegisterKindToRegisterNumber(
         eRegisterKindGeneric, *generic_reg_p);
     if (reg_num == LLDB_INVALID_REGNUM)
       continue; // Target does not support the given register.
 #endif
 
     const RegisterInfo *const reg_info_p =
if (error.Fail()) {
                     __FUNCTION__,
                     reg_info_p->name ? reg_info_p->name : "<unnamed-register>",
                     reg_num, error.AsCString());
       continue;
     }
 
     StreamString stream;
     WriteRegisterValueInHexFixedWidth(stream, reg_ctx_sp, *reg_info_p,
-                                      &reg_value);
+                                      &reg_value, lldb::eByteOrderBig);
 
     register_object_sp->SetObject(
         llvm::to_string(reg_num),
         std::make_shared<JSONString>(stream.GetString()));
   }
 
   return register_object_sp;
 }
if (log) {
                   " got signal signo = %d, reason = %d, exc_type = %" PRIu64,
                   __FUNCTION__, process.GetID(), tid, signum,
                   tid_stop_info.reason, tid_stop_info.details.exception.type);
     }
 
     JSONObject::SP thread_obj_sp = std::make_shared<JSONObject>();
     threads_array_sp->AppendObject(thread_obj_sp);
 
-    if (JSONObject::SP registers_sp = GetRegistersAsJSON(*thread_sp, abridged))
+    if (!abridged) {
+      if (JSONObject::SP registers_sp = GetRegistersAsJSON(*thread_sp))
         thread_obj_sp->SetObject("registers", registers_sp);
+    }
 
     thread_obj_sp->SetObject("tid", std::make_shared<JSONNumber>(tid));
     if (signum != 0)
       thread_obj_sp->SetObject("signal", std::make_shared<JSONNumber>(signum));
 
     const std::string thread_name = thread_sp->GetName();
     if (!thread_name.empty())
       thread_obj_sp->SetObject("name",
if (thread_index > 0) {
         threads_info_sp->Write(unescaped_response);
         response.PutCStringAsRawHex8(unescaped_response.GetData());
         response.PutChar(';');
       } else if (log)
         log->Printf("GDBRemoteCommunicationServerLLGS::%s failed to prepare a "
                     "jstopinfo field for pid %" PRIu64,
                     __FUNCTION__, m_debugged_process_sp->GetID());
     }
+
+    uint32_t i = 0;
+    response.PutCString("thread-pcs");
+    char delimiter = ':';
+    for (NativeThreadProtocolSP thread_sp;
+         (thread_sp = m_debugged_process_sp->GetThreadAtIndex(i)) != nullptr;
+         ++i) {
+      NativeRegisterContextSP reg_ctx_sp = thread_sp->GetRegisterContext();
+      if (!reg_ctx_sp)
+        continue;
+
+      uint32_t reg_to_read = reg_ctx_sp->ConvertRegisterKindToRegisterNumber(
+          eRegisterKindGeneric, LLDB_REGNUM_GENERIC_PC);
+      const RegisterInfo *const reg_info_p =
+          reg_ctx_sp->GetRegisterInfoAtIndex(reg_to_read);
+
+      RegisterValue reg_value;
+      Error error = reg_ctx_sp->ReadRegister(reg_info_p, reg_value);
+      if (error.Fail()) {
+        if (log)
+          log->Printf("%s failed to read register '%s' index %" PRIu32 ": %s",
+                      __FUNCTION__,
+                      reg_info_p->name ? reg_info_p->name
+                                       : "<unnamed-register>",
+                      reg_to_read, error.AsCString());
+        continue;
+      }
+
+      response.PutChar(delimiter);
+      delimiter = ',';
+      WriteRegisterValueInHexFixedWidth(response, reg_ctx_sp, *reg_info_p,
+                                        &reg_value, endian::InlHostByteOrder());
+    }
+
+    response.PutChar(';');
   }
 
   //
   // Expedite registers.
   //
 
   // Grab the register context.
   NativeRegisterContextSP reg_ctx_sp = thread_sp->GetRegisterContext();
if (reg_ctx_sp->GetRegisterSetCount() > 0 &&
                         *reg_num_p);
         } else if (reg_info_p->value_regs == nullptr) {
           // Only expediate registers that are not contained in other registers.
           RegisterValue reg_value;
           Error error = reg_ctx_sp->ReadRegister(reg_info_p, reg_value);
           if (error.Success()) {
             response.Printf("%.02x:", *reg_num_p);
             WriteRegisterValueInHexFixedWidth(response, reg_ctx_sp, *reg_info_p,
-                                              &reg_value);
+                                              &reg_value, lldb::eByteOrderBig);
             response.PutChar(';');
           } else {
             if (log)
               log->Printf("GDBRemoteCommunicationServerLLGS::%s failed to read "
                           "register '%s' index %" PRIu32 ": %s",
                           __FUNCTION__, reg_info_p->name ? reg_info_p->name
                                                          : "<unnamed-register>",
                           *reg_num_p, error.AsCString());

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

private:
   //------------------------------------------------------------------
   // ContinueDelegate interface
   //------------------------------------------------------------------
   void HandleAsyncStdout(llvm::StringRef out) override;
   void HandleAsyncMisc(llvm::StringRef data) override;
   void HandleStopReply() override;
   void HandleAsyncStructuredDataPacket(llvm::StringRef data) override;
 
+  void SetThreadPc(const lldb::ThreadSP &thread_sp, uint64_t index);
   using ModuleCacheKey = std::pair<std::string, std::string>;
   // KeyInfo for the cached module spec DenseMap.
   // The invariant is that all real keys will have the file and architecture
   // set.
   // The empty key has an empty file and an empty arch.
   // The tombstone key has an invalid arch and an empty file.
   // The comparison and hash functions take the file name and architecture
   // triple into account.

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

for (size_t i = 0; i < num_thread_ids; ++i) {
                       thread_sp->GetID());
       } else {
         if (log && log->GetMask().Test(GDBR_LOG_VERBOSE))
           log->Printf("ProcessGDBRemote::%s Found old thread: %p for thread "
                       "ID: 0x%" PRIx64 ".\n",
                       __FUNCTION__, static_cast<void *>(thread_sp.get()),
                       thread_sp->GetID());
       }
-      // The m_thread_pcs vector has pc values in big-endian order, not
-      // target-endian, unlike most
-      // of the register read/write packets in gdb-remote protocol.
-      // Early in the process startup, we may not yet have set the process
-      // ByteOrder so we ignore these;
-      // they are a performance improvement over fetching thread register values
-      // individually, the
-      // method we will fall back to if needed.
-      if (m_thread_ids.size() == m_thread_pcs.size() && thread_sp.get() &&
-          GetByteOrder() != eByteOrderInvalid) {
-        ThreadGDBRemote *gdb_thread =
-            static_cast<ThreadGDBRemote *>(thread_sp.get());
-        RegisterContextSP reg_ctx_sp(thread_sp->GetRegisterContext());
-        if (reg_ctx_sp) {
-          uint32_t pc_regnum = reg_ctx_sp->ConvertRegisterKindToRegisterNumber(
-              eRegisterKindGeneric, LLDB_REGNUM_GENERIC_PC);
-          if (pc_regnum != LLDB_INVALID_REGNUM) {
-            gdb_thread->PrivateSetRegisterValue(pc_regnum, m_thread_pcs[i]);
-          }
-        }
-      }
+
+      SetThreadPc(thread_sp, i);
       new_thread_list.AddThreadSortedByIndexID(thread_sp);
     }
   }
 
   // Whatever that is left in old_thread_list_copy are not
   // present in new_thread_list. Remove non-existent threads from internal id
   // table.
   size_t old_num_thread_ids = old_thread_list_copy.GetSize(false);
   for (size_t i = 0; i < old_num_thread_ids; i++) {
     ThreadSP old_thread_sp(old_thread_list_copy.GetThreadAtIndex(i, false));
     if (old_thread_sp) {
       lldb::tid_t old_thread_id = old_thread_sp->GetProtocolID();
       m_thread_id_to_index_id_map.erase(old_thread_id);
     }
   }
 
   return true;
 }
 
+void ProcessGDBRemote::SetThreadPc(const ThreadSP &thread_sp, uint64_t index) {
+  if (m_thread_ids.size() == m_thread_pcs.size() && thread_sp.get() &&
+      GetByteOrder() != eByteOrderInvalid) {
+    ThreadGDBRemote *gdb_thread =
+        static_cast<ThreadGDBRemote *>(thread_sp.get());
+    RegisterContextSP reg_ctx_sp(thread_sp->GetRegisterContext());
+    if (reg_ctx_sp) {
+      uint32_t pc_regnum = reg_ctx_sp->ConvertRegisterKindToRegisterNumber(
+          eRegisterKindGeneric, LLDB_REGNUM_GENERIC_PC);
+      if (pc_regnum != LLDB_INVALID_REGNUM) {
+        gdb_thread->PrivateSetRegisterValue(pc_regnum, m_thread_pcs[index]);
+      }
+    }
+  }
+}
+
 bool ProcessGDBRemote::GetThreadStopInfoFromJSON(
     ThreadGDBRemote *thread, const StructuredData::ObjectSP &thread_infos_sp) {
   // See if we got thread stop infos for all threads via the "jThreadsInfo"
   // packet
   if (thread_infos_sp) {
     StructuredData::Array *thread_infos = thread_infos_sp->GetAsArray();
     if (thread_infos) {
       lldb::tid_t tid;
// Scope for "locker" below
       }
     }
 
     if (thread_sp) {
       ThreadGDBRemote *gdb_thread =
           static_cast<ThreadGDBRemote *>(thread_sp.get());
       gdb_thread->GetRegisterContext()->InvalidateIfNeeded(true);
 
+      auto iter = std::find(m_thread_ids.begin(), m_thread_ids.end(), tid);
+      if (iter != m_thread_ids.end()) {
+        SetThreadPc(thread_sp, iter - m_thread_ids.begin());
+      }
+
       for (const auto &pair : expedited_register_map) {
         StringExtractor reg_value_extractor;
         reg_value_extractor.GetStringRef() = pair.second;
         DataBufferSP buffer_sp(new DataBufferHeap(
             reg_value_extractor.GetStringRef().size() / 2, 0));
         reg_value_extractor.GetHexBytes(buffer_sp->GetData(), '\xcc');
         gdb_thread->PrivateSetRegisterValue(pair.first, buffer_sp->GetData());
       }