Prevent client from querying each thread's PC at each stop.
authorPavel Labath <labath@google.com>
Fri, 20 Jan 2017 14:17:16 +0000 (14:17 +0000)
committerPavel Labath <labath@google.com>
Fri, 20 Jan 2017 14:17:16 +0000 (14:17 +0000)
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.

Reviewed By: labath

Differential Revision: https://reviews.llvm.org/D28880
Author: Jason Majors

llvm-svn: 292611

lldb/packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteThreadsInStopReply.py
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h

index 57d4d5a..b361b9e 100644 (file)
@@ -1,11 +1,13 @@
 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):
 
@@ -16,7 +18,8 @@ class TestGdbRemoteThreadsInStopReply(
         "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):
@@ -25,6 +28,9 @@ class TestGdbRemoteThreadsInStopReply(
         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:
@@ -34,6 +40,7 @@ class TestGdbRemoteThreadsInStopReply(
         ], 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)
@@ -77,14 +84,89 @@ class TestGdbRemoteThreadsInStopReply(
         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(
@@ -183,3 +265,34 @@ class TestGdbRemoteThreadsInStopReply(
         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)
index bf72673..27dcd88 100644 (file)
@@ -415,7 +415,8 @@ static void AppendHexValue(StreamString &response, const uint8_t *buf,
 
 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);
@@ -426,7 +427,8 @@ static void WriteRegisterValueInHexFixedWidth(
 
   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) {
@@ -436,8 +438,7 @@ static void WriteRegisterValueInHexFixedWidth(
   }
 }
 
-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();
@@ -462,11 +463,8 @@ static JSONObject::SP GetRegistersAsJSON(NativeThreadProtocol &thread,
   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);
@@ -501,7 +499,7 @@ static JSONObject::SP GetRegistersAsJSON(NativeThreadProtocol &thread,
 
     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),
@@ -567,8 +565,10 @@ static JSONArray::SP GetJSONThreadsInfo(NativeProcessProtocol &process,
     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))
-      thread_obj_sp->SetObject("registers", registers_sp);
+    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)
@@ -721,6 +721,41 @@ GDBRemoteCommunicationServerLLGS::SendStopReplyPacketForThread(
                     "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(';');
   }
 
   //
@@ -761,7 +796,7 @@ GDBRemoteCommunicationServerLLGS::SendStopReplyPacketForThread(
           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)
index 992a719..397c57f 100644 (file)
@@ -1649,27 +1649,8 @@ bool ProcessGDBRemote::UpdateThreadList(ThreadList &old_thread_list,
                       __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);
     }
   }
@@ -1689,6 +1670,22 @@ bool ProcessGDBRemote::UpdateThreadList(ThreadList &old_thread_list,
   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"
@@ -1774,6 +1771,11 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo(
           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;
index 6423abc..1ecd582 100644 (file)
@@ -417,6 +417,7 @@ private:
   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