[lldb] [server] Support for multiprocess extension

[mgorny]

Summary

Add a minimal support for the multiprocess extension in lldb-server.
The server indicates support for it via qSupported, and accepts
thread-ids containing a PID. However, the server still supports
debugging only one inferior, so any other PID value results in an error.

[mgorny]

Updated to include multiprocess in qSupported, and proposed how to handle it server-side on the example of H packet. Basically, accept the correct PID of current process, reject anything else (including 0 and -1 that do not make sense for H).

[labath]

Some initial remarks:

• the parsing function is clearly gdb-related, so it should go into StringExtractorGDBRemote.h, and not its base class
• the universalness of the function makes its interface pretty complicated, and bloats the call sites. Its probably good for the lowest layer, but maybe we could make things a lot cleaner if we created some more specialized functions on top of that. For example, most of the code (packets) statically knows whether it can accept -1 or 0 for process or thread ids, so we would benefit from those checks being done in a central place. in particular, until we really start supporting multiple processes, all of the packets will be doing something like if (pid != m_process->pid()) return error(), so we could have a version that accepts a value that *must* be present as the pid field. That would also allow us to mostly abstract away the protocol differences since the caller could just deal with the tid, knowing that the pid (if present) has already been checked.
• I wouldn't add multiprocess to the qSupported packets just yet. Since this is accepting the extended syntax regardless of whether the other side claimed to support it (there's no hard in doing that, right?), we could flip the switch after all packets have been ported.

lldb/source/Utility/StringExtractor.cpp

375 ↗

(On Diff #330225)

I think this would benefit from using more StringRefs (this class is ancient, so it reads very C like), similar to the GetNameColonValue function above. Maybe something like:

llvm::StringRef view = StringRef(m_packet).substr(m_index);
view = view.ltrim();
if (view.consume_front("p")) {
  view = view.ltrim();
  if (view.consume_front("-1") ...
    
}

[mgorny]

Updated utility function to use StringRef and reformatted. Further changes incoming.

[mgorny]

In D98482#2623005, @labath wrote:

• I wouldn't add multiprocess to the qSupported packets just yet. Since this is accepting the extended syntax regardless of whether the other side claimed to support it (there's no hard in doing that, right?), we could flip the switch after all packets have been ported.

Sure, I have this split into more commits locally, I'll keep `qSupported' last.

[mgorny]

Add a GDBRemoteCommunicationServerLLGS::ReadTid() helper that does appropriate PID verification and constructs error packets.

[mgorny]

Added support for 0 and -1 values in ReadTid() helper, controlled via allow_any and allow_all parameters.

[labath]

I'm still bugged by the GetPidTid interface -- the pair of optionals is very complicated. What if we had this function take a default_pid argument (which would be equal to the currently selected process), and it returned that as the process id, in case the packet does not contain one? Then the interface could be something like Optional<pair<pid_t, tid_t>> GetPidTid(pid_t default_pid) (because tid automatically defaults to -1), which I think is more intuitive. In case we really need to differentiate between a pid not being present and an implicitly chosen pid, then the interface could be like Optional<pair<Optional<pid_t>, tid_t>> GetPidTid(), which still seems /slightly/ better as its easier to recognise invalid input. WDYT?

Regarding the ReadTid function, I think that sending the (error) response packet from within that is a bit too much. I think it would be better if that returned an Error/Expected and left the sending to the caller.

[mgorny]

In D98482#2624973, @labath wrote:

I'm still bugged by the GetPidTid interface -- the pair of optionals is very complicated. What if we had this function take a default_pid argument (which would be equal to the currently selected process), and it returned that as the process id, in case the packet does not contain one? Then the interface could be something like Optional<pair<pid_t, tid_t>> GetPidTid(pid_t default_pid) (because tid automatically defaults to -1), which I think is more intuitive. In case we really need to differentiate between a pid not being present and an implicitly chosen pid, then the interface could be like Optional<pair<Optional<pid_t>, tid_t>> GetPidTid(), which still seems /slightly/ better as its easier to recognise invalid input. WDYT?

I've originally wanted to precisely convey whatever was in the packet since it was a generic class, and I actually thought nested Optionals are going to be worse. But thinking about it, you're probably right — the spec explicitly says to assume tid=-1 when only pid is provided, so I guess we can use a single Optional and just assume a default pid. However, I think I'm going to go a step further and eliminate pair as well, in favor of llvm::Optional<lldb::tid_t> GetPidTid(lldb:pid_t& pid), with pid being modified if specified or otherwise left in its current (i.e. default) value.

[mgorny]

Or scratch that. What you propose is cleaner and requires less changes to tests ;-).

[mgorny]

Updated GetPidTid() to use llvm::Optional<std::pair<lldb::pid_t, lldb::tid_t>> return type with a default PID approach as suggested by @labath.

Updated ReadTid() to use llvm::Expected<lldb::tid_t>. Create a new GDBRemoteError class to pass gdb-remote $E codes through cleanly.

[mgorny]

clang-format.

[mgorny]

I've just discovered that LLDB_INVALID_*_ID isn't -1/UINT_MAX as I thought, so I've had to introduce additional constants. That said, I'm not convinced about the current 0/-1 API. Maybe it'd make sense to handle them directly in ReadTid() and return some (current?) thread ID for 0, and LLDB_INVALID_*_ID for -1?

Added vCont support along with tests.

[labath]

In D98482#2626237, @mgorny wrote:

Create a new GDBRemoteError class to pass gdb-remote $E codes through cleanly.

The error codes we use right now are completely meaningless, so don't bother preserving them. I don't think we should be introducing a separate error class on their account, and I'm particularly unhappy about how this class has insinuated itself into the Status object.

In D98482#2631420, @mgorny wrote:

I've just discovered that LLDB_INVALID_*_ID isn't -1/UINT_MAX as I thought, so I've had to introduce additional constants. That said, I'm not convinced about the current 0/-1 API. Maybe it'd make sense to handle them directly in ReadTid() and return some (current?) thread ID for 0, and LLDB_INVALID_*_ID for -1?

I think that decision which thread to use for the "any" thread should not be handled at such a low level, as it can have real impact on how the inferior behaves. I think using constants for this is fine. Although I applaud the avoidance of macros, I think the constants should also use non-macro nomenclature. Maybe just put the names inside the StringExtractorGdbRemote class, and call them AnyThread (etc.) ? Also, I think that these days we can just use constexpr variables instead of the enum hack.

lldb/source/Utility/StringExtractorGDBRemote.cpp

625

This should be something like view.consumeInteger(16, pid). At that point you can stop keeping m_index in sync, and just update it at the very end (maybe even via llvm::scope_exit).

lldb/unittests/Utility/StringExtractorGDBRemoteTest.cpp

9–11

For empty fixtures, you can just drop this, along with the _F in the TEST_F macro.

[mgorny]

In D98482#2631817, @labath wrote:

In D98482#2626237, @mgorny wrote:

Create a new GDBRemoteError class to pass gdb-remote $E codes through cleanly.

The error codes we use right now are completely meaningless, so don't bother preserving them. I don't think we should be introducing a separate error class on their account, and I'm particularly unhappy about how this class has insinuated itself into the Status object.

Well, I found the ability to use different codes useful for debugging tests but I guess it doesn't matter much. So a plain StringError then?

lldb/source/Utility/StringExtractorGDBRemote.cpp

625

Do you have some specific method of updating it mind? Comparing size before and after?

[mgorny]

• switched thread-id parsing to use view.consumeInteger() and replaced m_index manipulations with a single update at the very end
• added additional test for parsing multiple thread-ids in a single packet (to verify that m_index is correctly updated)
• moved thread-id constants to static constexpr inside the class
• replaced GDBError with plain StringErrors

[labath]

This should be fine, assuming the following statement is true: "all thread id's that we're passing from server to client are in the form of some lldb-specific extension to the gdb-remote protocol". If that is not the case, then we should also update the client to work with the new format.

In D98482#2631986, @mgorny wrote:

In D98482#2631817, @labath wrote:

In D98482#2626237, @mgorny wrote:

Create a new GDBRemoteError class to pass gdb-remote $E codes through cleanly.

The error codes we use right now are completely meaningless, so don't bother preserving them. I don't think we should be introducing a separate error class on their account, and I'm particularly unhappy about how this class has insinuated itself into the Status object.

Well, I found the ability to use different codes useful for debugging tests but I guess it doesn't matter much. So a plain StringError then?

Yeah. Note we also have the ability to send the error messages through the protocol nowadays. I'm not sure if the lldb-server test suite enables them, but we could certainly change that.

lldb/source/Utility/StringExtractorGDBRemote.cpp

625

StringExtractor::GetNameColonValue does it by subtracting the .data() pointers, but this could be even better...

lldb/unittests/Utility/StringExtractorGDBRemoteTest.cpp

143–146

IIRC gdb docs say this is invalid, so I'd reject it immediately at this level.

[mgorny]

In D98482#2640981, @labath wrote:

This should be fine, assuming the following statement is true: "all thread id's that we're passing from server to client are in the form of some lldb-specific extension to the gdb-remote protocol". If that is not the case, then we should also update the client to work with the new format.

That statement is somewhat confusing. Do I understand correctly that I should update the client to handle process IDs in all the places where GDB protocol permits server to send PIDs? I suppose that makes sense.

lldb/unittests/Utility/StringExtractorGDBRemoteTest.cpp

143–146

Will do.

[mgorny]

Move 0/-1 value checking down to StringExtractorGDBRemote. Reject 0 unconditionally, since we do not use it.

TODO: client support

[labath]

In D98482#2641041, @mgorny wrote:

In D98482#2640981, @labath wrote:

This should be fine, assuming the following statement is true: "all thread id's that we're passing from server to client are in the form of some lldb-specific extension to the gdb-remote protocol". If that is not the case, then we should also update the client to work with the new format.

That statement is somewhat confusing. Do I understand correctly that I should update the client to handle process IDs in all the places where GDB protocol permits server to send PIDs? I suppose that makes sense.

Yeah, that's what I meant. The having "multiprocess" in qSupported means that we support the relevant syntax, and I don't want the client to lie about that. If it turns out to be particularly problematic, we can discuss that. Possibly by including the extension only in the server response?

[mgorny]

Added client support for PIDs.

[mgorny]

try uploading again

[mgorny]

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

741

I've noticed a few places where we take U32 instead of U64, so fixed that as well.

2780

@labath, any clue if we could make this work some other way? Calling GetCurrentProcessID() causes a crash for some tests here:

Fatal Python error: Segmentation fault

Thread 0x00007f918d203640 (most recent call first):
  File "/home/mgorny/git/llvm-project/lldb/test/API/functionalities/gdb_remote_client/gdbclientutils.py", line 470 in _handlePacket
  File "/home/mgorny/git/llvm-project/lldb/test/API/functionalities/gdb_remote_client/gdbclientutils.py", line 380 in _receive
  File "/home/mgorny/git/llvm-project/lldb/test/API/functionalities/gdb_remote_client/gdbclientutils.py", line 364 in _run
  File "/usr/lib/python3.10/threading.py", line 910 in run
  File "/usr/lib/python3.10/threading.py", line 972 in _bootstrap_inner
  File "/usr/lib/python3.10/threading.py", line 930 in _bootstrap

Current thread 0x00007f9196327740 (most recent call first):
  File "/home/mgorny/git/llvm-project/build.gentoo/lib/python3.10/site-packages/lldb/__init__.py", line 10250 in ConnectRemote
  File "/home/mgorny/git/llvm-project/lldb/test/API/functionalities/gdb_remote_client/gdbclientutils.py", line 526 in connect
  File "/home/mgorny/git/llvm-project/lldb/test/API/functionalities/gdb_remote_client/TestRecognizeBreakpoint.py", line 103 in test
  File "/home/mgorny/git/llvm-project/lldb/third_party/Python/module/unittest2/unittest2/case.py", line 413 in runMethod
  File "/home/mgorny/git/llvm-project/lldb/third_party/Python/module/unittest2/unittest2/case.py", line 383 in run
  File "/home/mgorny/git/llvm-project/lldb/third_party/Python/module/unittest2/unittest2/case.py", line 458 in __call__
  File "/home/mgorny/git/llvm-project/lldb/third_party/Python/module/unittest2/unittest2/suite.py", line 117 in _wrapped_run
  File "/home/mgorny/git/llvm-project/lldb/third_party/Python/module/unittest2/unittest2/suite.py", line 115 in _wrapped_run
  File "/home/mgorny/git/llvm-project/lldb/third_party/Python/module/unittest2/unittest2/suite.py", line 85 in run
  File "/home/mgorny/git/llvm-project/lldb/third_party/Python/module/unittest2/unittest2/suite.py", line 66 in __call__
  File "/home/mgorny/git/llvm-project/lldb/third_party/Python/module/unittest2/unittest2/runner.py", line 165 in run
  File "/home/mgorny/git/llvm-project/lldb/packages/Python/lldbsuite/test/dotest.py", line 997 in run_suite
  File "/home/mgorny/git/llvm-project/lldb/test/API/dotest.py", line 7 in <module>

Extension modules: lldb._lldb (total: 1)

[labath]

I think it'd be better to commit this in two patches after all. The first one could include the server bits (and the multiprocess+ server thingy) -- I believe this is good to go already. The second patch/review could deal with the thread/process id business on the client (and finally enable multiprocess+ client-side).

lldb/include/lldb/Utility/StringExtractorGDBRemote.h

197

These also need a definition in the .cpp file (debug builds fail without that).

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

741

Maybe just commit that separately (no review needed).

2780

This backtrace is useless, as it only shows python threads (and the crash happens in a native one), but if you run the test in a debugger, the problem becomes obvious -- mutual recursion between GetCurrentThreadIDs and GetCurrentProcessID. To fix that, we need to break it.

I'd probably to in by creating a new function, which implements the core of q[fs]ThreadInfo functionality (for instance, it could return something like (vector<pair<pid, tid>> with INVALID_PID for unknown), and then call it from both of these functions to do their thing.

[mgorny]

Ok, I'm going to use this review to push the server part, then commit the uint64 client change separately, then create a new diff for client.

If you find a few more minutes, the server-side fork/vfork patch is ready for review (and independent of this one).

[mgorny]

Limited to server changes. Added definitions for constexpr vars.

[stella.stamenova]

Looks like this broke the Windows lldb buildbot:

https://lab.llvm.org/buildbot/#/builders/83/builds/5173

[mgorny]

In D98482#2658991, @stella.stamenova wrote:

Looks like this broke the Windows lldb buildbot:

https://lab.llvm.org/buildbot/#/builders/83/builds/5173

Thanks for pinging me. It seems that all Hg tests fail on Windows, so just added xfail to these 3 as well.

lldb/include/lldb/Utility/StringExtractorGDBRemote.h

197

I've reproduced that and fixed it.

lldb/include/lldb/Utility/StringExtractorGDBRemote.h

 //===-- StringExtractorGDBRemote.h ------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_UTILITY_STRINGEXTRACTORGDBREMOTE_H
 #define LLDB_UTILITY_STRINGEXTRACTORGDBREMOTE_H
 
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/StringExtractor.h"
 #include "llvm/ADT/StringRef.h"
 
 #include <string>
 
 #include <stddef.h>
 #include <stdint.h>
 
public:
   // Returns zero if the packet isn't a EXX packet where XX are two hex digits.
   // Otherwise the error encoded in XX is returned.
   uint8_t GetError();
 
   lldb_private::Status GetStatus();
 
   size_t GetEscapedBinaryData(std::string &str);
 
+  // Read thread-id in the <tid> | "p" <pid> ["." <tid>] format, where <pid>
+  // and <tid> can be either "-1", "0" or a hex-encoded number.  Note that
+  // the function does not check for incorrect values such as -1.<tid != -1>.
+  // A pair of (llvm::None, llvm::None) is returned for malformed input.
+  std::pair<llvm::Optional<lldb::pid_t>, llvm::Optional<lldb::tid_t>>
+  GetPidTid();
+
 protected:
   ResponseValidatorCallback m_validator;
   void *m_validator_baton;
 };
 
 #endif // LLDB_UTILITY_STRINGEXTRACTORGDBREMOTE_H

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

_KNOWN_QSUPPORTED_STUB_FEATURES = [
         "QStartNoAckMode",
         "QThreadSuffixSupported",
         "QListThreadsInStopReply",
         "qXfer:auxv:read",
         "qXfer:libraries:read",
         "qXfer:libraries-svr4:read",
         "qXfer:features:read",
         "qEcho",
-        "QPassSignals"
+        "QPassSignals",
+        "multiprocess",
     ]
 
     def parse_qSupported_response(self, context):
         self.assertIsNotNone(context)
 
         raw_response = context.get("qSupported_response")
         self.assertIsNotNone(raw_response)
 

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

protected:
   LazyBool m_supports_qXfer_features_read;
   LazyBool m_supports_qXfer_memory_map_read;
   LazyBool m_supports_augmented_libraries_svr4_read;
   LazyBool m_supports_jThreadExtendedInfo;
   LazyBool m_supports_jLoadedDynamicLibrariesInfos;
   LazyBool m_supports_jGetSharedCacheInfo;
   LazyBool m_supports_QPassSignals;
   LazyBool m_supports_error_string_reply;
+  LazyBool m_supports_multiprocess;
 
   bool m_supports_qProcessInfoPID : 1, m_supports_qfProcessInfo : 1,
       m_supports_qUserName : 1, m_supports_qGroupName : 1,
       m_supports_qThreadStopInfo : 1, m_supports_z0 : 1, m_supports_z1 : 1,
       m_supports_z2 : 1, m_supports_z3 : 1, m_supports_z4 : 1,
       m_supports_QEnvironment : 1, m_supports_QEnvironmentHexEncoded : 1,
       m_supports_qSymbol : 1, m_qSymbol_requests_done : 1,
       m_supports_qModuleInfo : 1, m_supports_jThreadsInfo : 1,

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

: GDBRemoteClientBase("gdb-remote.client", "gdb-remote.client.rx_packet"),
       m_supports_qXfer_features_read(eLazyBoolCalculate),
       m_supports_qXfer_memory_map_read(eLazyBoolCalculate),
       m_supports_augmented_libraries_svr4_read(eLazyBoolCalculate),
       m_supports_jThreadExtendedInfo(eLazyBoolCalculate),
       m_supports_jLoadedDynamicLibrariesInfos(eLazyBoolCalculate),
       m_supports_jGetSharedCacheInfo(eLazyBoolCalculate),
       m_supports_QPassSignals(eLazyBoolCalculate),
       m_supports_error_string_reply(eLazyBoolCalculate),
+      m_supports_multiprocess(eLazyBoolCalculate),
       m_supports_qProcessInfoPID(true), m_supports_qfProcessInfo(true),
       m_supports_qUserName(true), m_supports_qGroupName(true),
       m_supports_qThreadStopInfo(true), m_supports_z0(true),
       m_supports_z1(true), m_supports_z2(true), m_supports_z3(true),
       m_supports_z4(true), m_supports_QEnvironment(true),
       m_supports_QEnvironmentHexEncoded(true), m_supports_qSymbol(true),
       m_qSymbol_requests_done(false), m_supports_qModuleInfo(true),
       m_supports_jThreadsInfo(true), m_supports_jModulesInfo(true),
if (!did_exec) {
     m_qHostInfo_is_valid = eLazyBoolCalculate;
     m_curr_pid_is_valid = eLazyBoolCalculate;
     m_qGDBServerVersion_is_valid = eLazyBoolCalculate;
     m_supports_alloc_dealloc_memory = eLazyBoolCalculate;
     m_supports_memory_region_info = eLazyBoolCalculate;
     m_prepare_for_reg_writing_reply = eLazyBoolCalculate;
     m_attach_or_wait_reply = eLazyBoolCalculate;
     m_avoid_g_packets = eLazyBoolCalculate;
+    m_supports_multiprocess = 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_qProcessInfoPID = true;
     m_supports_qfProcessInfo = true;

 void GDBRemoteCommunicationClient::GetRemoteQSupported() {
   // Clear out any capabilities we expect to see in the qSupported response
   m_supports_qXfer_auxv_read = eLazyBoolNo;
   m_supports_qXfer_libraries_read = eLazyBoolNo;
   m_supports_qXfer_libraries_svr4_read = eLazyBoolNo;
   m_supports_augmented_libraries_svr4_read = eLazyBoolNo;
   m_supports_qXfer_features_read = eLazyBoolNo;
   m_supports_qXfer_memory_map_read = eLazyBoolNo;
+  m_supports_multiprocess = eLazyBoolNo;
   m_max_packet_size = UINT64_MAX; // It's supposed to always be there, but if
                                   // not, we assume no limit
 
   // build the qSupported packet
-  std::vector<std::string> features = {"xmlRegisters=i386,arm,mips,arc"};
+  std::vector<std::string> features = {"xmlRegisters=i386,arm,mips,arc",
+                                       "multiprocess+"};
   StreamString packet;
   packet.PutCString("qSupported");
   for (uint32_t i = 0; i < features.size(); ++i) {
     packet.PutCString(i == 0 ? ":" : ";");
     packet.PutCString(features[i]);
   }
 
   StringExtractorGDBRemote response;
if (SendPacketAndWaitForResponse(packet.GetString(), response,
     else
       m_supports_qEcho = eLazyBoolNo;
 
     if (::strstr(response_cstr, "QPassSignals+"))
       m_supports_QPassSignals = eLazyBoolYes;
     else
       m_supports_QPassSignals = eLazyBoolNo;
 
+    if (::strstr(response_cstr, "multiprocess+"))
+      m_supports_multiprocess = eLazyBoolYes;
+    else
+      m_supports_multiprocess = eLazyBoolNo;
+
     const char *packet_size_str = ::strstr(response_cstr, "PacketSize=");
     if (packet_size_str) {
       StringExtractorGDBRemote packet_response(packet_size_str +
                                                strlen("PacketSize="));
       m_max_packet_size =
           packet_response.GetHexMaxU64(/*little_endian=*/false, UINT64_MAX);
       if (m_max_packet_size == 0) {
         m_max_packet_size = UINT64_MAX; // Must have been a garbled response
if (m_curr_pid_is_valid == eLazyBoolYes) {
     // lldb-platform stubs. The gdb remote protocol documents $qC as returning
     // the thread id, which newer debugserver and lldb-gdbserver stubs return
     // correctly.
     StringExtractorGDBRemote response;
     if (SendPacketAndWaitForResponse("qC", response, false) ==
         PacketResult::Success) {
       if (response.GetChar() == 'Q') {
         if (response.GetChar() == 'C') {
           m_curr_pid = response.GetHexMaxU32(false, LLDB_INVALID_PROCESS_ID);
           if (m_curr_pid != LLDB_INVALID_PROCESS_ID) {
             m_curr_pid_is_valid = eLazyBoolYes;
             return m_curr_pid;
           }
         }
       }
     }
 
if (SendPacketAndWaitForResponse(packet, response, true) ==
     }
   }
   // Signal generic failure
   return UINT8_MAX;
 }
 
 size_t GDBRemoteCommunicationClient::GetCurrentThreadIDs(
     std::vector<lldb::tid_t> &thread_ids, bool &sequence_mutex_unavailable) {
   thread_ids.clear();
 
   Lock lock(*this, false);
   if (lock) {
     sequence_mutex_unavailable = false;
     StringExtractorGDBRemote response;
 
     PacketResult packet_result;
     for (packet_result =

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

GDBRemoteCommunicationServerCommon::Handle_qSupported(
   response.PutCString(";QListThreadsInStopReply+");
   response.PutCString(";qEcho+");
   response.PutCString(";qXfer:features:read+");
 #if defined(__linux__) || defined(__NetBSD__) || defined(__FreeBSD__)
   response.PutCString(";QPassSignals+");
   response.PutCString(";qXfer:auxv:read+");
   response.PutCString(";qXfer:libraries-svr4:read+");
 #endif
+  response.PutCString(";multiprocess+");
 
   return SendPacketNoLock(response.GetString());
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerCommon::Handle_QThreadSuffixSupported(
     StringExtractorGDBRemote &packet) {
   m_thread_suffix_supported = true;

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

 //===-- GDBRemoteCommunicationServerLLGS.h ----------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_SOURCE_PLUGINS_PROCESS_GDB_REMOTE_GDBREMOTECOMMUNICATIONSERVERLLGS_H
 #define LLDB_SOURCE_PLUGINS_PROCESS_GDB_REMOTE_GDBREMOTECOMMUNICATIONSERVERLLGS_H
 
 #include <mutex>
 #include <unordered_map>
 
 #include "lldb/Core/Communication.h"
 #include "lldb/Host/MainLoop.h"
 #include "lldb/Host/common/NativeProcessProtocol.h"
 #include "lldb/lldb-private-forward.h"
 
 #include "GDBRemoteCommunicationServerCommon.h"
 
 class StringExtractorGDBRemote;
 
private:
   void DataAvailableCallback();
 
   void SendProcessOutput();
 
   void StartSTDIOForwarding();
 
   void StopSTDIOForwarding();
 
+  // Read thread-id from packet.  If the thread-id is correct (and PID matches
+  // the current program), sets tid to it and returns llvm::None.  Otherwise,
+  // returns the error response.
+  llvm::Optional<PacketResult>
+  ReadTid(StringExtractorGDBRemote &packet, lldb::tid_t &tid);
+
   // For GDBRemoteCommunicationServerLLGS only
   GDBRemoteCommunicationServerLLGS(const GDBRemoteCommunicationServerLLGS &) =
       delete;
   const GDBRemoteCommunicationServerLLGS &
   operator=(const GDBRemoteCommunicationServerLLGS &) = delete;
 };
 
 } // namespace process_gdb_remote
 } // namespace lldb_private
 
 #endif // LLDB_SOURCE_PLUGINS_PROCESS_GDB_REMOTE_GDBREMOTECOMMUNICATIONSERVERLLGS_H

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

 //===-- GDBRemoteCommunicationServerLLGS.cpp ------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include <errno.h>
 
 #include "lldb/Host/Config.h"
 
 
 #include <chrono>
 #include <cstring>
 #include <thread>
 
 #include "GDBRemoteCommunicationServerLLGS.h"
 #include "lldb/Host/ConnectionFileDescriptor.h"
LLDB_LOGF(
         log,
         "GDBRemoteCommunicationServerLLGS::%s failed, invalid $H variant %c",
         __FUNCTION__, h_variant);
     return SendIllFormedResponse(packet,
                                  "H variant unsupported, should be c or g");
   }
 
   // Parse out the thread number.
-  // FIXME return a parse success/fail value.  All values are valid here.
-  const lldb::tid_t tid =
-      packet.GetHexMaxU64(false, std::numeric_limits<lldb::tid_t>::max());
+  lldb::tid_t tid;
+  auto tid_ret = ReadTid(packet, tid);
+  if (tid_ret)
+    return tid_ret.getValue();
 
   // Ensure we have the given thread when not specifying -1 (all threads) or 0
   // (any thread).
   if (tid != LLDB_INVALID_THREAD_ID && tid != 0) {
     NativeThreadProtocol *thread = m_debugged_process_up->GetThreadByID(tid);
     if (!thread) {
       LLDB_LOGF(log,
                 "GDBRemoteCommunicationServerLLGS::%s failed, tid %" PRIu64
case '>':
       break;
     default:
       result += c;
       break;
     }
   }
   return result;
 }
+
+llvm::Optional<GDBRemoteCommunication::PacketResult>
+GDBRemoteCommunicationServerLLGS::ReadTid(StringExtractorGDBRemote &packet,
+                                          lldb::tid_t &tid) {
+  Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_THREAD));
+
+  auto pid_tid = packet.GetPidTid();
+  if (pid_tid.first) {
+    const lldb::pid_t pid = pid_tid.first.getValue();
+    // Since we require a specific thread number, the PID must not be -1/0.
+    if (pid != m_debugged_process_up->GetID()) {
+      LLDB_LOGF(log,
+                "GDBRemoteCommunicationServerLLGS::%s failed, pid %" PRIu64
+                " not found",
+                __FUNCTION__, pid);
+      return SendErrorResponse(0x15);
+    }
+  } else if (!pid_tid.second) {
+    LLDB_LOGF(
+        log,
+        "GDBRemoteCommunicationServerLLGS::%s failed, malformed thread-id",
+        __FUNCTION__);
+    return SendIllFormedResponse(packet, "Malformed thread-id");
+  }
+
+  tid = pid_tid.second.getValue();
+  return llvm::None;
+}

lldb/source/Utility/StringExtractorGDBRemote.cpp

 //===-- StringExtractorGDBRemote.cpp --------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Utility/StringExtractorGDBRemote.h"
 
 #include <ctype.h>
 #include <string.h>
 
 StringExtractorGDBRemote::ResponseType
 StringExtractorGDBRemote::GetResponseType() const {
   if (m_packet.empty())
     return eUnsupported;

 
 bool StringExtractorGDBRemote::ValidateResponse() const {
   // If we have a validator callback, try to validate the callback
   if (m_validator)
     return m_validator(m_validator_baton, *this);
   else
     return true; // No validator, so response is valid
 }
+
+std::pair<llvm::Optional<lldb::pid_t>, llvm::Optional<lldb::tid_t>>
+StringExtractorGDBRemote::GetPidTid() {
+  llvm::Optional<lldb::pid_t> pid = llvm::None;
+  llvm::Optional<lldb::tid_t> tid = llvm::None;
+  llvm::StringRef view = llvm::StringRef(m_packet).substr(m_index);
+
+  if (view.consume_front("p")) {
+    // process identifier
+    ++m_index;
+    if (view.consume_front("-1")) {
+      // -1 is a special case
+      m_index += 2;
+      pid = LLDB_INVALID_PROCESS_ID;
+    } else {
+      uint64_t prev_index = m_index;
+      pid = GetHexMaxU64(false, 0);
+      if (m_index == prev_index || m_index == UINT64_MAX) {
+        // if index was not incremented or we overflowed, it failed
+        m_index = UINT64_MAX;
+        return {llvm::None, llvm::None};
+      }
+      view = view.substr(m_index - prev_index);
+    }
+
+    // "." must follow if we expect TID too
+    if (!view.consume_front("."))
+      return {pid, tid};
+    ++m_index;
+  }
+
+  // thread identifier
+  if (view.consume_front("-1")) {
+    // -1 is a special case
+    m_index += 2;
+    tid = LLDB_INVALID_PROCESS_ID;
+  } else {
+    uint64_t prev_index = m_index;
+    tid = GetHexMaxU64(false, 0);
+    if (m_index == prev_index || m_index == UINT64_MAX) {
+      // if index was not incremented or we overflowed, it failed
+      // (also reset pid since the value is incorrect)
+      m_index = UINT64_MAX;
+      return {llvm::None, llvm::None};
+    }
+  }
+
+  return {pid, tid};
+}

lldb/test/API/tools/lldb-server/TestLldbGdbServer.py

def test_p_returns_correct_data_size_for_each_qRegisterInfo_launch(self):
             if "dynamic_size_dwarf_expr_bytes" in reg_info:
                 self.updateRegInfoBitsize(reg_info, byte_order)
             self.assertEqual(len(p_response), 2 * int(reg_info["bitsize"]) / 8,
                              reg_info)
 
             # Increment loop
             reg_index += 1
 
-    def Hg_switches_to_3_threads(self):
+    def Hg_switches_to_3_threads(self, pass_pid=False):
         # Startup the inferior with three threads (main + 2 new ones).
         procs = self.prep_debug_monitor_and_inferior(
             inferior_args=["thread:new", "thread:new"])
 
         # Let the inferior process have a few moments to start up the thread
         # when launched.  (The launch scenario has no time to run, so threads
         # won't be there yet.)
         self.run_process_then_stop(run_seconds=1)
 
         # Wait at most x seconds for 3 threads to be present.
         threads = self.wait_for_thread_count(3)
         self.assertEqual(len(threads), 3)
 
+        pid_str = ""
+        if pass_pid:
+            pid_str = "p{0:x}.".format(procs["inferior"].pid)
+
         # verify we can $H to each thead, and $qC matches the thread we set.
         for thread in threads:
             # Change to each thread, verify current thread id.
             self.reset_test_sequence()
             self.test_sequence.add_log_lines(
-                ["read packet: $Hg{0:x}#00".format(thread),  # Set current thread.
+                ["read packet: $Hg{0}{1:x}#00".format(pid_str, thread),  # Set current thread.
                  "send packet: $OK#00",
                  "read packet: $qC#00",
                  {"direction": "send", "regex": r"^\$QC([0-9a-fA-F]+)#", "capture": {1: "thread_id"}}],
                 True)
 
             context = self.expect_gdbremote_sequence()
             self.assertIsNotNone(context)
 
             # Verify the thread id.
             self.assertIsNotNone(context.get("thread_id"))
             self.assertEqual(int(context.get("thread_id"), 16), thread)
 
     @expectedFailureAll(oslist=["windows"]) # expect 4 threads
     def test_Hg_switches_to_3_threads_launch(self):
         self.build()
         self.set_inferior_startup_launch()
         self.Hg_switches_to_3_threads()
 
     @expectedFailureAll(oslist=["windows"]) # expecting one more thread
     def test_Hg_switches_to_3_threads_attach(self):
         self.build()
         self.set_inferior_startup_attach()
         self.Hg_switches_to_3_threads()
 
+    @expectedFailureAll(oslist=["windows"]) # expect 4 threads
+    def test_Hg_switches_to_3_threads_attach_pass_correct_pid(self):
+        self.build()
+        self.set_inferior_startup_attach()
+        self.Hg_switches_to_3_threads(pass_pid=True)
+
+    def Hg_fails_on_pid(self, pass_pid):
+        # Start the inferior.
+        procs = self.prep_debug_monitor_and_inferior(
+            inferior_args=["thread:new"])
+
+        self.run_process_then_stop(run_seconds=1)
+
+        threads = self.wait_for_thread_count(2)
+        self.assertEqual(len(threads), 2)
+
+        if pass_pid == -1:
+            pid_str = "p-1."
+        else:
+            pid_str = "p{0:x}.".format(pass_pid)
+        thread = threads[1]
+
+        self.test_sequence.add_log_lines(
+            ["read packet: $Hg{0}{1:x}#00".format(pid_str, thread),  # Set current thread.
+             "send packet: $E15#00"],
+            True)
+
+        self.expect_gdbremote_sequence()
+
+    def test_Hg_fails_on_another_pid(self):
+        self.build()
+        self.set_inferior_startup_launch()
+        self.Hg_fails_on_pid(1)
+
+    def test_Hg_fails_on_zero_pid(self):
+        self.build()
+        self.set_inferior_startup_launch()
+        self.Hg_fails_on_pid(0)
+
+    def test_Hg_fails_on_minus_one_pid(self):
+        self.build()
+        self.set_inferior_startup_launch()
+        self.Hg_fails_on_pid(-1)
+
     def Hc_then_Csignal_signals_correct_thread(self, segfault_signo):
         # NOTE only run this one in inferior-launched mode: we can't grab inferior stdout when running attached,
         # and the test requires getting stdout from the exe.
 
         NUM_THREADS = 3
 
         # Startup the inferior with three threads (main + NUM_THREADS-1 worker threads).
         # inferior_args=["thread:print-ids"]

lldb/unittests/Utility/CMakeLists.txt

add_lldb_unittest(UtilityTests
   ReproducerInstrumentationTest.cpp
   ReproducerTest.cpp
   ScalarTest.cpp
   SharedClusterTest.cpp
   StateTest.cpp
   StatusTest.cpp
   StreamTeeTest.cpp
   StreamTest.cpp
+  StringExtractorGDBRemoteTest.cpp
   StringExtractorTest.cpp
   StringLexerTest.cpp
   StringListTest.cpp
   StructuredDataTest.cpp
   SubsystemRAIITest.cpp
   TildeExpressionResolverTest.cpp
   TimeoutTest.cpp
   TimerTest.cpp

lldb/unittests/Utility/StringExtractorGDBRemoteTest.cpp

+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include <limits.h>
+
+#include "lldb/Utility/StringExtractorGDBRemote.h"
+#include "lldb/lldb-defines.h"
+
+namespace {
+class StringExtractorGDBRemoteTest : public ::testing::Test {};
+} // namespace
+
+TEST_F(StringExtractorGDBRemoteTest, GetPidTid) {
+  StringExtractorGDBRemote ex("");
+  EXPECT_THAT(ex.GetPidTid(), ::testing::Pair(llvm::None, llvm::None));
+
+  // invalid/short values
+
+  ex.Reset("narf");
+  EXPECT_THAT(ex.GetPidTid(), ::testing::Pair(llvm::None, llvm::None));
+
+  ex.Reset(";1234");
+  EXPECT_THAT(ex.GetPidTid(), ::testing::Pair(llvm::None, llvm::None));
+
+  ex.Reset(".1234");
+  EXPECT_THAT(ex.GetPidTid(), ::testing::Pair(llvm::None, llvm::None));
+
+  ex.Reset("p");
+  EXPECT_THAT(ex.GetPidTid(), ::testing::Pair(llvm::None, llvm::None));
+
+  ex.Reset("pnarf");
+  EXPECT_THAT(ex.GetPidTid(), ::testing::Pair(llvm::None, llvm::None));
+
+  ex.Reset("p;1234");
+  EXPECT_THAT(ex.GetPidTid(), ::testing::Pair(llvm::None, llvm::None));
+
+  ex.Reset("p.1234");
+  EXPECT_THAT(ex.GetPidTid(), ::testing::Pair(llvm::None, llvm::None));
+
+  ex.Reset("p1234.");
+  EXPECT_THAT(ex.GetPidTid(), ::testing::Pair(llvm::None, llvm::None));
+
+  EXPECT_THAT(ex.GetPidTid(), ::testing::Pair(llvm::None, llvm::None));
+
+  ex.Reset("p1234.;1234");
+  EXPECT_THAT(ex.GetPidTid(), ::testing::Pair(llvm::None, llvm::None));
+
+  ex.Reset("-2");
+  EXPECT_THAT(ex.GetPidTid(), ::testing::Pair(llvm::None, llvm::None));
+
+  ex.Reset("p1234.-2");
+  EXPECT_THAT(ex.GetPidTid(), ::testing::Pair(llvm::None, llvm::None));
+
+  ex.Reset("p-2");
+  EXPECT_THAT(ex.GetPidTid(), ::testing::Pair(llvm::None, llvm::None));
+
+  ex.Reset("p-2.1234");
+  EXPECT_THAT(ex.GetPidTid(), ::testing::Pair(llvm::None, llvm::None));
+
+  // overflow
+
+  ex.Reset("p10000000000000000");
+  EXPECT_THAT(ex.GetPidTid(), ::testing::Pair(llvm::None, llvm::None));
+
+  ex.Reset("p10000000000000000.0");
+  EXPECT_THAT(ex.GetPidTid(), ::testing::Pair(llvm::None, llvm::None));
+
+  ex.Reset("10000000000000000");
+  EXPECT_THAT(ex.GetPidTid(), ::testing::Pair(llvm::None, llvm::None));
+
+  ex.Reset("p0.10000000000000000");
+  EXPECT_THAT(ex.GetPidTid(), ::testing::Pair(llvm::None, llvm::None));
+
+  ex.Reset("p10000000000000000.10000000000000000");
+  EXPECT_THAT(ex.GetPidTid(), ::testing::Pair(llvm::None, llvm::None));
+
+  // pure thread id
+
+  ex.Reset("0");
+  EXPECT_THAT(ex.GetPidTid(), ::testing::Pair(llvm::None, 0));
+
+  ex.Reset("-1");
+  EXPECT_THAT(ex.GetPidTid(),
+              ::testing::Pair(llvm::None, LLDB_INVALID_THREAD_ID));
+
+  ex.Reset("1234");
+  EXPECT_THAT(ex.GetPidTid(), ::testing::Pair(llvm::None, 0x1234ULL));
+
+  ex.Reset("123456789ABCDEF0");
+  EXPECT_THAT(ex.GetPidTid(),
+              ::testing::Pair(llvm::None, 0x123456789ABCDEF0ULL));
+
+  // pure process id
+
+  ex.Reset("p0");
+  EXPECT_THAT(ex.GetPidTid(), ::testing::Pair(0, llvm::None));
+
+  ex.Reset("p-1");
+  EXPECT_THAT(ex.GetPidTid(),
+              ::testing::Pair(LLDB_INVALID_PROCESS_ID, llvm::None));
+
+  ex.Reset("p1234");
+  EXPECT_THAT(ex.GetPidTid(), ::testing::Pair(0x1234ULL, llvm::None));
+
+  ex.Reset("p123456789ABCDEF0");
+  EXPECT_THAT(ex.GetPidTid(),
+              ::testing::Pair(0x123456789ABCDEF0ULL, llvm::None));
+
+  // combined thread id + process id
+
+  ex.Reset("p0.0");
+  EXPECT_THAT(ex.GetPidTid(), ::testing::Pair(0, 0));
+
+  ex.Reset("p0.-1");
+  EXPECT_THAT(ex.GetPidTid(), ::testing::Pair(0, LLDB_INVALID_THREAD_ID));
+
+  // NB: technically, specific thread with unspecified process is invalid
+  // but we do not filter that in StringExtractor
+
+  ex.Reset("p0.1234");
+  EXPECT_THAT(ex.GetPidTid(), ::testing::Pair(0, 0x1234ULL));
+
+  ex.Reset("p0.123456789ABCDEF0");
+  EXPECT_THAT(ex.GetPidTid(), ::testing::Pair(0, 0x123456789ABCDEF0ULL));
+
+  ex.Reset("p-1.0");
+  EXPECT_THAT(ex.GetPidTid(), ::testing::Pair(LLDB_INVALID_PROCESS_ID, 0));
+
+  ex.Reset("p-1.-1");
+  EXPECT_THAT(ex.GetPidTid(),
+              ::testing::Pair(LLDB_INVALID_PROCESS_ID, LLDB_INVALID_THREAD_ID));
+
+  ex.Reset("p-1.1234");
+  EXPECT_THAT(ex.GetPidTid(),
+              ::testing::Pair(LLDB_INVALID_PROCESS_ID, 0x1234ULL));
+
+  ex.Reset("p-1.123456789ABCDEF0");
+  EXPECT_THAT(ex.GetPidTid(),
+              ::testing::Pair(LLDB_INVALID_PROCESS_ID, 0x123456789ABCDEF0ULL));
+
+  ex.Reset("p1234.0");
+  EXPECT_THAT(ex.GetPidTid(), ::testing::Pair(0x1234ULL, 0));
+
+  ex.Reset("p1234.-1");
+  EXPECT_THAT(ex.GetPidTid(),
+              ::testing::Pair(0x1234ULL, LLDB_INVALID_THREAD_ID));
+
+  ex.Reset("p1234.123456789ABCDEF0");
+  EXPECT_THAT(ex.GetPidTid(),
+              ::testing::Pair(0x1234ULL, 0x123456789ABCDEF0ULL));
+
+  ex.Reset("p123456789ABCDEF0.0");
+  EXPECT_THAT(ex.GetPidTid(), ::testing::Pair(0x123456789ABCDEF0ULL, 0));
+
+  ex.Reset("p123456789ABCDEF0.-1");
+  EXPECT_THAT(ex.GetPidTid(),
+              ::testing::Pair(0x123456789ABCDEF0ULL, LLDB_INVALID_THREAD_ID));
+
+  ex.Reset("p123456789ABCDEF0.1234");
+  EXPECT_THAT(ex.GetPidTid(),
+              ::testing::Pair(0x123456789ABCDEF0ULL, 0x1234ULL));
+
+  ex.Reset("p123456789ABCDEF0.123456789ABCDEF0");
+  EXPECT_THAT(ex.GetPidTid(),
+              ::testing::Pair(0x123456789ABCDEF0ULL, 0x123456789ABCDEF0ULL));
+
+  ex.Reset("p123456789ABCDEF0.123456789ABCDEF0");
+  EXPECT_THAT(ex.GetPidTid(),
+              ::testing::Pair(0x123456789ABCDEF0ULL, 0x123456789ABCDEF0ULL));
+}