[lldb] [gdb-remote] Include PID in vCont packets if multiprocess

[mgorny]

Summary

Try to always send vCont packets and include the PID in them if running
multiprocess. This is necessary to ensure that with the upcoming full
multiprocess support always resumes the correct process without having
to resort to the legacy Hc packets.

Sponsored by: The FreeBSD Foundation

[labath]

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

1189–1192

If we're going to have these in more places, we should add a helper function to format a pid.tid pair.

1190

Is this really necessary. If I recall correctly, we go through a lot of trouble to come up with some pid value (resort to hardcoding pid to 1 if everything fails)..

[mgorny]

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

1189–1192

I'll bear that in mind.

1190

Probably not. I've guessed maybe it could happen somewhat early in the process but I couldn't really reproduce it ever.

[Michael137]

This seems to cause all API tests to time out.

See LLDB Incremental buildbot: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/46215/execution/node/70/log/

Can we revert this until we know what the root cause is?

[aprantl]

In D131758#3735621, @Michael137 wrote:

This seems to cause all API tests to time out.

See LLDB Incremental buildbot: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/46215/execution/node/70/log/

Can we revert this until we know what the root cause is?

I reverted this and the follow-up commit that disables a test on Windows for now. Please let us know if you need any help in diagnosing the problem!

[mgorny]

In D131758#3735628, @aprantl wrote:

In D131758#3735621, @Michael137 wrote:

This seems to cause all API tests to time out.

See LLDB Incremental buildbot: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/46215/execution/node/70/log/

Can we revert this until we know what the root cause is?

I reverted this and the follow-up commit that disables a test on Windows for now. Please let us know if you need any help in diagnosing the problem!

Help would be most welcome. Unless my grep skills are failing me, the log doesn't contain anything but timeouts. I'd use at least some gdb-remote log. My only guess right now would be that debugserver doesn't support vCont;c:-1.

Alternatively, could you try reapplying the original commit but changing:

continue_packet.Format("vCont;c:{0}-1", pid_prefix);

back to:

continue_packet.PutCString("c");

[Michael137]

In D131758#3735670, @mgorny wrote:

In D131758#3735628, @aprantl wrote:

In D131758#3735621, @Michael137 wrote:

This seems to cause all API tests to time out.

See LLDB Incremental buildbot: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/46215/execution/node/70/log/

Can we revert this until we know what the root cause is?

I reverted this and the follow-up commit that disables a test on Windows for now. Please let us know if you need any help in diagnosing the problem!

Help would be most welcome. Unless my grep skills are failing me, the log doesn't contain anything but timeouts. I'd use at least some gdb-remote log. My only guess right now would be that debugserver doesn't support vCont;c:-1.

Alternatively, could you try reapplying the original commit but changing:

continue_packet.Format("vCont;c:{0}-1", pid_prefix);

back to:

continue_packet.PutCString("c");

Can confirm this fixes the timeouts.
I'll revert for now and let you push the fix.

[Michael137]

In D131758#3735670, @mgorny wrote:

In D131758#3735628, @aprantl wrote:

In D131758#3735621, @Michael137 wrote:

This seems to cause all API tests to time out.

See LLDB Incremental buildbot: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/46215/execution/node/70/log/

Can we revert this until we know what the root cause is?

I reverted this and the follow-up commit that disables a test on Windows for now. Please let us know if you need any help in diagnosing the problem!

Help would be most welcome. Unless my grep skills are failing me, the log doesn't contain anything but timeouts. I'd use at least some gdb-remote log. My only guess right now would be that debugserver doesn't support vCont;c:-1.

Alternatively, could you try reapplying the original commit but changing:

continue_packet.Format("vCont;c:{0}-1", pid_prefix);

back to:

continue_packet.PutCString("c");

@mgorny

Snippet of test log with timeout:

********************
UNSUPPORTED: lldb-api :: commands/expression/call-restarts/TestCallThatRestarts.py (64 of 1755)
TIMEOUT: lldb-api :: sample_test/TestSampleInlineTest.py (65 of 1755)
******************** TEST 'lldb-api :: sample_test/TestSampleInlineTest.py' FAILED ********************
Script:
--
/Applications/Xcode.app/Contents/Developer/Library/Frameworks/Python3.framework/Versions/3.8/bin/python3.8 /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/test/API/dotest.py -u CXXFLAGS -u CFLAGS --codesign-identity lldb_codesign --arch x86_64 --build-dir /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex -t --env TERM=vt100 --env LLVM_LIBS_DIR=/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/./lib --env LLVM_INCLUDE_DIR=/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/include --env LLVM_TOOLS_DIR=/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/./bin --hermetic-libcxx --arch x86_64 --build-dir /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex --lldb-module-cache-dir /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex/module-cache-lldb/lldb-api --clang-module-cache-dir /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/lldb-test-build.noindex/module-cache-clang/lldb-api --executable /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/./bin/lldb --compiler /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/./bin/clang --dsymutil /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/./bin/dsymutil --llvm-tools-dir /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/./bin --lldb-libs-dir /Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/./lib /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/test/API/sample_test -p TestSampleInlineTest.py
--
Exit Code: -9
Timeout: Reached timeout of 600 seconds

[mgorny]

@aprantl, @Michael137, could you confirm that this version works correctly?

[Michael137]

In D131758#3735763, @mgorny wrote:

@aprantl, @Michael137, could you confirm that this version works correctly?

ninja check-lldb-api runs fine with your patch

[mgorny]

@aprantl, why did you revert d38985a36be8b0165787f8893b3d2b0f831d1e83? This change had nothing to do with this patch, and you broke Windows buildbot again. If you really can't wait a few hours for me to able to show up, I'd really appreciate if you took responsibility for the things you've broken.

[Michael137]

In D131758#3736793, @mgorny wrote:

@aprantl, why did you revert d38985a36be8b0165787f8893b3d2b0f831d1e83? This change had nothing to do with this patch, and you broke Windows buildbot again. If you really can't wait a few hours for me to able to show up, I'd really appreciate if you took responsibility for the things you've broken.

Apologies, there was some miscommunication around which commit to revert. I should’ve checked the buildbot again

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

public:
   bool GetAugmentedLibrariesSVR4ReadSupported();
 
   bool GetQXferFeaturesReadSupported();
 
   bool GetQXferMemoryMapReadSupported();
 
   bool GetQXferSigInfoReadSupported();
 
+  bool GetMultiprocessSupported();
+
   LazyBool SupportsAllocDeallocMemory() // const
   {
     // Uncomment this to have lldb pretend the debug server doesn't respond to
     // alloc/dealloc memory packets.
     // m_supports_alloc_dealloc_memory = lldb_private::eLazyBoolNo;
     return m_supports_alloc_dealloc_memory;
   }
 

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

 
 bool GDBRemoteCommunicationClient::GetQXferSigInfoReadSupported() {
   if (m_supports_qXfer_siginfo_read == eLazyBoolCalculate) {
     GetRemoteQSupported();
   }
   return m_supports_qXfer_siginfo_read == eLazyBoolYes;
 }
 
+bool GDBRemoteCommunicationClient::GetMultiprocessSupported() {
+  if (m_supports_memory_tagging == eLazyBoolCalculate)
+    GetRemoteQSupported();
+  return m_supports_multiprocess == eLazyBoolYes;
+}
+
 uint64_t GDBRemoteCommunicationClient::GetRemoteMaxPacketSize() {
   if (m_max_packet_size == 0) {
     GetRemoteQSupported();
   }
   return m_max_packet_size;
 }
 
 bool GDBRemoteCommunicationClient::QueryNoAckModeSupported() {
if (keep_stopped) {
     if (m_supports_detach_stay_stopped == eLazyBoolNo) {
       error.SetErrorString("Stays stopped not supported by this target.");
       return error;
     } else {
       packet.PutChar('1');
     }
   }
 
-  if (m_supports_multiprocess) {
+  if (GetMultiprocessSupported()) {
     // Some servers (e.g. qemu) require specifying the PID even if only a single
     // process is running.
     if (pid == LLDB_INVALID_PROCESS_ID)
       pid = GetCurrentProcessID();
     packet.PutChar(';');
     packet.PutHex64(pid);
   } else if (pid != LLDB_INVALID_PROCESS_ID) {
     error.SetErrorString("Multiprocess extension not supported by the server.");

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

listener_sp->StartListeningForEvents(
         &m_async_broadcaster,
         ProcessGDBRemote::eBroadcastBitAsyncThreadDidExit);
 
     const size_t num_threads = GetThreadList().GetSize();
 
     StreamString continue_packet;
     bool continue_packet_error = false;
     if (m_gdb_comm.HasAnyVContSupport()) {
+      std::string pid_prefix;
+      if (m_gdb_comm.GetMultiprocessSupported())
+        pid_prefix = llvm::formatv("p{0:x-}.", GetID());
+
       if (m_continue_c_tids.size() == num_threads ||
           (m_continue_c_tids.empty() && m_continue_C_tids.empty() &&
            m_continue_s_tids.empty() && m_continue_S_tids.empty())) {
-        // All threads are continuing, just send a "c" packet
+        // All threads are continuing
+        if (m_gdb_comm.GetMultiprocessSupported())
+          continue_packet.Format("vCont;c:{0}-1", pid_prefix);
+        else
           continue_packet.PutCString("c");
       } else {
         continue_packet.PutCString("vCont");
 
         if (!m_continue_c_tids.empty()) {
           if (m_gdb_comm.GetVContSupported('c')) {
             for (tid_collection::const_iterator
                      t_pos = m_continue_c_tids.begin(),
                      t_end = m_continue_c_tids.end();
                  t_pos != t_end; ++t_pos)
-              continue_packet.Printf(";c:%4.4" PRIx64, *t_pos);
+              continue_packet.Format(";c:{0}{1:x-}", pid_prefix, *t_pos);
           } else
             continue_packet_error = true;
         }
 
         if (!continue_packet_error && !m_continue_C_tids.empty()) {
           if (m_gdb_comm.GetVContSupported('C')) {
             for (tid_sig_collection::const_iterator
                      s_pos = m_continue_C_tids.begin(),
                      s_end = m_continue_C_tids.end();
                  s_pos != s_end; ++s_pos)
-              continue_packet.Printf(";C%2.2x:%4.4" PRIx64, s_pos->second,
-                                     s_pos->first);
+              continue_packet.Format(";C{0:x-2}:{1}{2:x-}", s_pos->second,
+                                     pid_prefix, s_pos->first);
           } else
             continue_packet_error = true;
         }
 
         if (!continue_packet_error && !m_continue_s_tids.empty()) {
           if (m_gdb_comm.GetVContSupported('s')) {
             for (tid_collection::const_iterator
                      t_pos = m_continue_s_tids.begin(),
                      t_end = m_continue_s_tids.end();
                  t_pos != t_end; ++t_pos)
-              continue_packet.Printf(";s:%4.4" PRIx64, *t_pos);
+              continue_packet.Format(";s:{0}{1:x-}", pid_prefix, *t_pos);
           } else
             continue_packet_error = true;
         }
 
         if (!continue_packet_error && !m_continue_S_tids.empty()) {
           if (m_gdb_comm.GetVContSupported('S')) {
             for (tid_sig_collection::const_iterator
                      s_pos = m_continue_S_tids.begin(),
                      s_end = m_continue_S_tids.end();
                  s_pos != s_end; ++s_pos)
-              continue_packet.Printf(";S%2.2x:%4.4" PRIx64, s_pos->second,
-                                     s_pos->first);
+              continue_packet.Format(";S{0:x-2}:{1}{2:x-}", s_pos->second,
+                                     pid_prefix, s_pos->first);
           } else
             continue_packet_error = true;
         }
 
         if (continue_packet_error)
           continue_packet.Clear();
       }
     } else

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

+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from lldbsuite.test.gdbclientutils import *
+from lldbsuite.test.lldbgdbclient import GDBRemoteTestBase
+
+
+class TestContinue(GDBRemoteTestBase):
+    class BaseResponder(MockGDBServerResponder):
+        def qSupported(self, client_supported):
+            return "PacketSize=3fff;QStartNoAckMode+;multiprocess+"
+
+        def qfThreadInfo(self):
+            return "mp400.401"
+
+        def haltReason(self):
+            return "S13"
+
+        def cont(self):
+            return "W01"
+
+        def other(self, packet):
+            if packet == "vCont?":
+                return "vCont;c;C;s;S"
+            if packet.startswith("vCont;"):
+                return "W00"
+            return ""
+
+    def test_continue_no_multiprocess(self):
+        class MyResponder(self.BaseResponder):
+            def qSupported(self, client_supported):
+                return "PacketSize=3fff;QStartNoAckMode+"
+
+            def qfThreadInfo(self):
+                return "m401"
+
+        self.server.responder = MyResponder()
+        self.runCmd("platform select remote-linux")
+        target = self.createTarget("a.yaml")
+        process = self.connect(target)
+        lldbutil.expect_state_changes(self, self.dbg.GetListener(), process,
+                                      [lldb.eStateExited])
+        self.assertPacketLogContains(["vCont;C13:401"])
+
+    def test_continue_no_vCont(self):
+        class MyResponder(self.BaseResponder):
+            def qSupported(self, client_supported):
+                return "PacketSize=3fff;QStartNoAckMode+"
+
+            def qfThreadInfo(self):
+                return "m401"
+
+            def other(self, packet):
+                return ""
+
+        self.server.responder = MyResponder()
+        self.runCmd("platform select remote-linux")
+        target = self.createTarget("a.yaml")
+        process = self.connect(target)
+        lldbutil.expect_state_changes(self, self.dbg.GetListener(), process,
+                                      [lldb.eStateExited])
+        self.assertPacketLogContains(["Hc401", "C13"])
+
+    def test_continue_multiprocess(self):
+        class MyResponder(self.BaseResponder):
+            pass
+
+        self.server.responder = MyResponder()
+        self.runCmd("platform select remote-linux")
+        target = self.createTarget("a.yaml")
+        process = self.connect(target)
+        lldbutil.expect_state_changes(self, self.dbg.GetListener(), process,
+                                      [lldb.eStateExited])
+        self.assertPacketLogContains(["vCont;C13:p400.401"])