Update the thread list before setting stop reasons with an OS plugin

[jingham]

Summary

When talking to a gdb-remote server that didn't support the newer thread info packets, we noticed that lldb would hit a breakpoint in target, but then continue on without stopping. This was because the only indication of why you stopped in this scenario was the initial "T05thread:01;" packet. Unfortunately, lldb read the stop reasons from the stop packet and created the new thread & set its stop reason BEFORE calling UpdateThreadListIfNeeded. So when you finally got around to calling UpdateThreadListIfNeeded, we would notice that the ThreadList StopID was out of date, wipe the stop reasons, and then update. Normally some other thread info packet would tell us the stop reason again, and we we'd get back on track. But if they weren't supported, we'd be left thinking that the process had stopped for no reason, and auto-continue it.

The main part of the patch just adjusts the call to UpdateThreadListIfNeeded so it happens before we try to set StopInfo's.

I also added a test that captures this failure.

[clayborg]

source/Target/Process.cpp

3037–3041 ↗

(On Diff #203053)

Should this be in the above if statement?

if (!m_os_up) {
    LoadOperatingSystemPlugin(false);
    // Somebody might have gotten threads before now, but we need to force the
    // update after we've loaded the OperatingSystem plugin or it won't get a
    // chance to process the threads.
    m_thread_list.Clear();
    UpdateThreadListIfNeeded();
}

And if we do this in the if statement, do we need to clear the m_thread_list?

[jingham]

Addresses Greg's question about what happens when we load a new OS plugin. Indeed we should limit the work we do only to the case where we didn't have an OS plugin, then tried to load one and succeeded. Only then do we need to clear and update the thread list.

[jingham]

source/Target/Process.cpp

3037–3041 ↗

(On Diff #203053)

You're right. Actually, we should try to get the OS plugin and only if we succeed do we need to do any work. I changed the code to do that.

lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestRecognizeBreakpoint.py

+from __future__ import print_function
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from gdbclientutils import *
+
+
+class TestRecognizeBreakpoint(GDBRemoteTestBase):
+    """ This tests the case where the gdb-remote server doesn't support any
+        of the thread-info packets, and just tells which thread got the stop
+        signal with:
+              T05thread:01;
+        There was a bug in lldb that we would set the stop reason from this 
+        packet too early - before we had updated the thread list.  So when we
+        later updated the thread list, we would throw away this info.  Normally
+        we would be able to reconstruct it from the thread info, but not if the
+        stub doesn't support it """
+             
+    def test(self):
+        class MyResponder(MockGDBServerResponder):
+            def __init__(self):
+                MockGDBServerResponder.__init__(self)
+                self.thread_info_count = 0
+                self.after_cont = False
+                self.current_thread = 0
+                
+            def cont(self):
+                # Simulate process stopping due to a breakpoint:
+                self.after_cont = True
+                return "T05thread:01;"
+
+            def vCont(self, packet):
+                self.after_cont = True
+                return "T05thread:01;"
+            
+            def haltReason(self):
+                return "T02thread:01;"
+
+            def threadStopInfo(self, num):
+                return ""
+
+            def QThreadSuffixSupported(self):
+                return ""
+
+            def QListThreadsInStopReply(self):
+                return ""
+
+            def setBreakpoint(self, packet):
+                return "OK"
+            
+            def qfThreadInfo(self):
+                return "m1"
+
+            def qsThreadInfo(self):
+                if (self.thread_info_count % 2) == 0:
+                    str = "m2"
+                else:
+                    str = "l"
+                self.thread_info_count += 1
+                return str
+
+            def readRegisters(self):
+                if self.after_cont and self.current_thread == 1:
+                    return "c01e990080ffffff"
+                else:
+                    return "badcfe10325476980"
+            
+            def readRegister(self, regno):
+                return ""
+            
+            def qXferRead(self, obj, annex, offset, length):
+                if annex == "target.xml":
+                    return """<?xml version="1.0"?>
+      <target version="1.0">
+                          <architecture>i386:x86-64</architecture>
+                          <feature name="org.gnu.gdb.i386.core">
+                            <reg name="rip" bitsize="64" regnum="0" type="code_ptr" group="general"/>
+                          </feature>
+                        </target>""", False
+    else:
+                    return None, False
+
+            def selectThread(self, op, thread):
+                if op != 'g':
+                    return ''
+                
+                self.current_thread = thread
+                return "OK"
+            
+            def other (self, packet):
+                if packet == "vCont?":
+                    return "vCont;c;C;s;S"
+                return ''
+                
+        python_os_plugin_path = os.path.join(self.getSourceDir(),
+                                             'operating_system_2.py')
+        command ="settings set target.process.python-os-plugin-path '{}'".format(
+            python_os_plugin_path)
+        self.runCmd(command)
+
+        self.server.responder = MyResponder()
+        target = self.dbg.CreateTarget("")
+        process = self.connect(target)
+
+        bkpt = target.BreakpointCreateByAddress(0xffffff8000991ec0)
+        self.assertEqual(bkpt.GetNumLocations(), 1, "Fake breakpoint was resolved.")
+
+        # Get the initial stop, and we should have two threads.
+        num_threads = len(process.threads)
+        self.assertEqual(num_threads, 2, "Got two threads")
+
+        thread_0 = process.threads[0]
+        self.assertEqual(thread_0.GetStopReason(), 1, "Thread_0 stopped for no reason")
+        self.assertEqual(thread_0.GetName(), "one", "Thread_0 is called one")
+        
+        thread_1 = process.threads[1]
+        self.assertEqual(thread_1.GetStopReason(), 5, "Thread_0 stopped for SIGSTOP")
+        self.assertEqual(thread_1.GetName(), "two", "Thread_0 is called two")
+        
+        # Now continue and we will fake hitting a breakpoint.
+        process.Continue()
+
+        self.assertEqual(process.GetState(),lldb.eStateStopped, "Process is stopped")
+        num_threads = len(process.threads)
+
+        num_threads = len(process.threads)
+        self.assertEqual(num_threads, 2, "Got two threads")
+
+        thread_0 = process.threads[0]
+        self.assertEqual(thread_0.GetStopReason(), 1, "Thread_0 stopped for no reason")
+        self.assertEqual(thread_0.GetName(), "one", "Thread_0 is called one")
+        
+        thread_1 = process.threads[1]
+        self.assertEqual(thread_1.GetStopReason(), 3, "Thread_0 stopped for SIGTRAP")
+        self.assertEqual(thread_1.GetName(), "three", "Thread_0 is called three")
+
+        self.assertTrue(thread_1.IsValid(), "Thread_1 is valid")
+        self.assertEqual(thread_1.GetStopReason(), lldb.eStopReasonBreakpoint, "Stopped at breakpoint")
+        

lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py

def respond(self, packet):
         Return the unframed packet data that the server should issue in response
         to the given packet received from the client.
         """
         self.packetLog.append(packet)
         if packet is MockGDBServer.PACKET_INTERRUPT:
             return self.interrupt()
         if packet == "c":
             return self.cont()
+        if packet.startswith("vCont;c"):
+            return self.vCont(packet)
         if packet[0] == "g":
             return self.readRegisters()
         if packet[0] == "G":
             return self.writeRegisters(packet[1:])
         if packet[0] == "p":
             regnum = packet[1:].split(';')[0]
             return self.readRegister(int(regnum, 16))
         if packet[0] == "P":
def respond(self, packet):
         return self.other(packet)
 
     def interrupt(self):
         raise self.UnexpectedPacketException()
 
     def cont(self):
         raise self.UnexpectedPacketException()
 
+    def vCont(self, packet):
+        raise self.UnexpectedPacketException()
+    
     def readRegisters(self):
         return "00000000" * self.registerCount
 
     def readRegister(self, register):
         return "00000000"
 
     def writeRegisters(self, registers_hex):
         return "OK"

lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/operating_system_2.py

+import lldb
+import struct
+
+
+class OperatingSystemPlugIn(object):
+    """Class that provides data for an instance of a LLDB 'OperatingSystemPython' plug-in class
+       This version stops once with threads 0x111 and 0x222, then stops a second time with threads
+       0x111 and 0x333."""
+
+    def __init__(self, process):
+        '''Initialization needs a valid.SBProcess object.
+
+        This plug-in will get created after a live process is valid and has stopped for the first time.
+        '''
+        self.process = None
+        self.registers = None
+        self.threads = None
+        self.times_called = 0
+        if isinstance(process, lldb.SBProcess) and process.IsValid():
+            self.process = process
+            self.threads = None  # Will be an dictionary containing info for each thread
+
+    def get_target(self):
+        return self.process.target
+
+    def get_thread_info(self):
+        self.times_called += 1
+
+        if self.times_called == 1:
+            self.threads = [{
+                'tid': 0x111,
+                'name': 'one',
+                'queue': 'queue1',
+                'state': 'stopped',
+                'stop_reason': 'none',
+                'core': 1
+            }, {
+                'tid': 0x222,
+                'name': 'two',
+                'queue': 'queue2',
+                'state': 'stopped',
+                'stop_reason': 'none',
+                'core': 0
+            }]
+        else:
+            self.threads = [{
+                'tid': 0x111,
+                'name': 'one',
+                'queue': 'queue1',
+                'state': 'stopped',
+                'stop_reason': 'none',
+                'core': 1
+            }, {
+                'tid': 0x333,
+                'name': 'three',
+                'queue': 'queue3',
+                'state': 'stopped',
+                'stop_reason': 'none',
+                'core': 0
+            }]
+        return self.threads
+

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

void ProcessGDBRemote::RefreshStateAfterStop() {
   m_thread_ids.clear();
   m_thread_pcs.clear();
   // Set the thread stop info. It might have a "threads" key whose value is a
   // list of all thread IDs in the current process, so m_thread_ids might get
   // set.
 
   // Scope for the lock
   {
+    // Check to see if SetThreadStopInfo() filled in m_thread_ids?
+    if (m_thread_ids.empty()) {
+        // No, we need to fetch the thread list manually
+        UpdateThreadIDList();
+    }
+    // We might set some stop info's so make sure the thread list is up to
+    // date before we do that or we might overwrite what was computed here.
+    UpdateThreadListIfNeeded();
+
     // Lock the thread stack while we access it
     std::lock_guard<std::recursive_mutex> guard(m_last_stop_packet_mutex);
     // Get the number of stop packets on the stack
     int nItems = m_stop_packet_stack.size();
     // Iterate over them
     for (int i = 0; i < nItems; i++) {
       // Get the thread stop info
       StringExtractorGDBRemote stop_info = m_stop_packet_stack[i];
       // Process thread stop info
       SetThreadStopInfo(stop_info);
     }
     // Clear the thread stop stack
     m_stop_packet_stack.clear();
   }
   
-  // Check to see if SetThreadStopInfo() filled in m_thread_ids?
-  if (m_thread_ids.empty()) {
-    // No, we need to fetch the thread list manually
-    UpdateThreadIDList();
-  }
-
   // If we have queried for a default thread id
   if (m_initial_tid != LLDB_INVALID_THREAD_ID) {
     m_thread_list.SetSelectedThreadByID(m_initial_tid);
     m_initial_tid = LLDB_INVALID_THREAD_ID;
   }
 
   // Let all threads recover from stopping and do any clean up based on the
   // previous thread state (if any).

lldb/trunk/source/Target/Process.cpp

if (log) {
                   "executable is %s (using %s plugin)",
                   __FUNCTION__,
                   exe_module_sp ? exe_module_sp->GetFileSpec().GetPath().c_str()
                                 : "<none>",
                   system_runtime->GetPluginName().AsCString("<unnamed>"));
     }
   }
 
-  if (!m_os_up)
+  if (!m_os_up) {
     LoadOperatingSystemPlugin(false);
+    if (m_os_up) {
+      // Somebody might have gotten threads before now, but we need to force the
+      // update after we've loaded the OperatingSystem plugin or it won't get a
+      // chance to process the threads.
+      m_thread_list.Clear();
+      UpdateThreadListIfNeeded();
+    }
+  }
   // Figure out which one is the executable, and set that in our target:
   const ModuleList &target_modules = GetTarget().GetImages();
   std::lock_guard<std::recursive_mutex> guard(target_modules.GetMutex());
   size_t num_modules = target_modules.GetSize();
   ModuleSP new_executable_module_sp;
 
   for (size_t i = 0; i < num_modules; i++) {
     ModuleSP module_sp(target_modules.GetModuleAtIndexUnlocked(i));