diff --git a/lldb/include/lldb/Utility/StringExtractorGDBRemote.h b/lldb/include/lldb/Utility/StringExtractorGDBRemote.h --- a/lldb/include/lldb/Utility/StringExtractorGDBRemote.h +++ b/lldb/include/lldb/Utility/StringExtractorGDBRemote.h @@ -193,6 +193,15 @@ size_t GetEscapedBinaryData(std::string &str); + static constexpr lldb::pid_t AllProcesses = UINT64_MAX; + static constexpr lldb::tid_t AllThreads = UINT64_MAX; + + // Read thread-id from the packet. If the packet is valid, returns + // the pair (PID, TID), otherwise returns llvm::None. If the packet + // does not list a PID, default_pid is used. + llvm::Optional> + GetPidTid(lldb::pid_t default_pid); + protected: ResponseValidatorCallback m_validator; void *m_validator_baton; diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py --- a/lldb/packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py +++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py @@ -902,7 +902,8 @@ "qXfer:libraries-svr4:read", "qXfer:features:read", "qEcho", - "QPassSignals" + "QPassSignals", + "multiprocess", ] def parse_qSupported_response(self, context): diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h @@ -558,6 +558,7 @@ 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, diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp @@ -848,6 +848,7 @@ response.PutCString(";qXfer:auxv:read+"); response.PutCString(";qXfer:libraries-svr4:read+"); #endif + response.PutCString(";multiprocess+"); return SendPacketNoLock(response.GetString()); } diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h @@ -244,6 +244,15 @@ void StopSTDIOForwarding(); + // Read thread-id from packet. If the thread-id is correct, returns it. + // Otherwise, returns the error. + // + // If allow_all is true, then the pid/tid value of -1 ('all') will be allowed. + // In any case, the function assumes that exactly one inferior is being + // debugged and rejects pid values that do no match that inferior. + llvm::Expected ReadTid(StringExtractorGDBRemote &packet, + bool allow_all = false); + // For GDBRemoteCommunicationServerLLGS only GDBRemoteCommunicationServerLLGS(const GDBRemoteCommunicationServerLLGS &) = delete; diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp @@ -1732,10 +1732,13 @@ // Consume the separator. packet.GetChar(); - thread_action.tid = packet.GetHexMaxU64(false, LLDB_INVALID_THREAD_ID); - if (thread_action.tid == LLDB_INVALID_THREAD_ID) - return SendIllFormedResponse( - packet, "Could not parse thread number in vCont packet"); + llvm::Expected tid_ret = ReadTid(packet, /*allow_all=*/true); + if (!tid_ret) + return SendErrorResponse(tid_ret.takeError()); + + thread_action.tid = tid_ret.get(); + if (thread_action.tid == StringExtractorGDBRemote::AllThreads) + thread_action.tid = LLDB_INVALID_THREAD_ID; } thread_actions.Append(thread_action); @@ -2220,10 +2223,11 @@ } // 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::max()); + llvm::Expected tid_ret = ReadTid(packet, /*allow_all=*/true); + if (!tid_ret) + return SendErrorResponse(tid_ret.takeError()); + lldb::tid_t tid = tid_ret.get(); // Ensure we have the given thread when not specifying -1 (all threads) or 0 // (any thread). if (tid != LLDB_INVALID_THREAD_ID && tid != 0) { @@ -3653,3 +3657,36 @@ } return result; } + +llvm::Expected +GDBRemoteCommunicationServerLLGS::ReadTid(StringExtractorGDBRemote &packet, + bool allow_all) { + assert(m_debugged_process_up); + assert(m_debugged_process_up->GetID() != LLDB_INVALID_PROCESS_ID); + + auto pid_tid = packet.GetPidTid(m_debugged_process_up->GetID()); + if (!pid_tid) + return llvm::make_error(inconvertibleErrorCode(), + "Malformed thread-id"); + + lldb::pid_t pid = pid_tid->first; + lldb::tid_t tid = pid_tid->second; + + if (!allow_all && pid == StringExtractorGDBRemote::AllProcesses) + return llvm::make_error( + inconvertibleErrorCode(), + llvm::formatv("PID value {0} not allowed", pid == 0 ? 0 : -1)); + + if (!allow_all && tid == StringExtractorGDBRemote::AllThreads) + return llvm::make_error( + inconvertibleErrorCode(), + llvm::formatv("TID value {0} not allowed", tid == 0 ? 0 : -1)); + + if (pid != StringExtractorGDBRemote::AllProcesses) { + if (pid != m_debugged_process_up->GetID()) + return llvm::make_error( + inconvertibleErrorCode(), llvm::formatv("PID {0} not debugged", pid)); + } + + return tid; +} diff --git a/lldb/source/Utility/StringExtractorGDBRemote.cpp b/lldb/source/Utility/StringExtractorGDBRemote.cpp --- a/lldb/source/Utility/StringExtractorGDBRemote.cpp +++ b/lldb/source/Utility/StringExtractorGDBRemote.cpp @@ -11,6 +11,9 @@ #include #include +constexpr lldb::pid_t StringExtractorGDBRemote::AllProcesses; +constexpr lldb::tid_t StringExtractorGDBRemote::AllThreads; + StringExtractorGDBRemote::ResponseType StringExtractorGDBRemote::GetResponseType() const { if (m_packet.empty()) @@ -606,3 +609,46 @@ else return true; // No validator, so response is valid } + +llvm::Optional> +StringExtractorGDBRemote::GetPidTid(lldb::pid_t default_pid) { + llvm::StringRef view = llvm::StringRef(m_packet).substr(m_index); + size_t initial_length = view.size(); + lldb::pid_t pid = default_pid; + lldb::tid_t tid; + + if (view.consume_front("p")) { + // process identifier + if (view.consume_front("-1")) { + // -1 is a special case + pid = AllProcesses; + } else if (view.consumeInteger(16, pid) || pid == 0) { + // not a valid hex integer OR unsupported pid 0 + m_index = UINT64_MAX; + return llvm::None; + } + + // "." must follow if we expect TID too; otherwise, we assume -1 + if (!view.consume_front(".")) { + // update m_index + m_index += initial_length - view.size(); + + return {{pid, AllThreads}}; + } + } + + // thread identifier + if (view.consume_front("-1")) { + // -1 is a special case + tid = AllThreads; + } else if (view.consumeInteger(16, tid) || tid == 0 || pid == AllProcesses) { + // not a valid hex integer OR tid 0 OR pid -1 + a specific tid + m_index = UINT64_MAX; + return llvm::None; + } + + // update m_index + m_index += initial_length - view.size(); + + return {{pid, tid}}; +} diff --git a/lldb/test/API/tools/lldb-server/TestGdbRemote_vContThreads.py b/lldb/test/API/tools/lldb-server/TestGdbRemote_vContThreads.py --- a/lldb/test/API/tools/lldb-server/TestGdbRemote_vContThreads.py +++ b/lldb/test/API/tools/lldb-server/TestGdbRemote_vContThreads.py @@ -49,6 +49,13 @@ for x in range(1, len(threads)+1)) self.assertEqual(tids, sorted(threads)) + def get_pid(self): + self.add_process_info_collection_packets() + context = self.expect_gdbremote_sequence() + self.assertIsNotNone(context) + procinfo = self.parse_process_info_response(context) + return int(procinfo['pid'], 16) + @skipIfWindows @expectedFailureNetBSD @expectedFailureDarwin # No signals delivered @@ -88,6 +95,82 @@ *threads), threads) + @skipIfWindows + @expectedFailureNetBSD + def test_signal_process_by_pid(self): + self.build() + self.set_inferior_startup_launch() + + threads = self.start_threads(1) + self.send_and_check_signal( + "C{0:x}:p{1:x}".format( + lldbutil.get_signal_number('SIGUSR1'), + self.get_pid()), + threads) + + @skipIfWindows + @expectedFailureNetBSD + def test_signal_process_minus_one(self): + self.build() + self.set_inferior_startup_launch() + + threads = self.start_threads(1) + self.send_and_check_signal( + "C{0:x}:p-1".format( + lldbutil.get_signal_number('SIGUSR1')), + threads) + + @skipIfWindows + @expectedFailureNetBSD + def test_signal_minus_one(self): + self.build() + self.set_inferior_startup_launch() + + threads = self.start_threads(1) + self.send_and_check_signal( + "C{0:x}:-1".format(lldbutil.get_signal_number('SIGUSR1')), + threads) + + @skipIfWindows + @expectedFailureNetBSD + def test_signal_all_threads_by_pid(self): + self.build() + self.set_inferior_startup_launch() + + threads = self.start_threads(1) + # try sending a signal to two threads (= the process) + self.send_and_check_signal( + "C{0:x}:p{1:x}.{2:x};C{0:x}:p{1:x}.{3:x}".format( + lldbutil.get_signal_number('SIGUSR1'), + self.get_pid(), + *threads), + threads) + + @skipIfWindows + @expectedFailureNetBSD + def test_signal_minus_one_by_pid(self): + self.build() + self.set_inferior_startup_launch() + + threads = self.start_threads(1) + self.send_and_check_signal( + "C{0:x}:p{1:x}.-1".format( + lldbutil.get_signal_number('SIGUSR1'), + self.get_pid()), + threads) + + @skipIfWindows + @expectedFailureNetBSD + def test_signal_minus_one_by_minus_one(self): + self.build() + self.set_inferior_startup_launch() + + threads = self.start_threads(1) + self.send_and_check_signal( + "C{0:x}:p-1.-1".format( + lldbutil.get_signal_number('SIGUSR1')), + threads) + @skipUnlessPlatform(["netbsd"]) def test_signal_two_of_three_threads(self): self.build() diff --git a/lldb/test/API/tools/lldb-server/TestLldbGdbServer.py b/lldb/test/API/tools/lldb-server/TestLldbGdbServer.py --- a/lldb/test/API/tools/lldb-server/TestLldbGdbServer.py +++ b/lldb/test/API/tools/lldb-server/TestLldbGdbServer.py @@ -349,7 +349,7 @@ # 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"]) @@ -363,12 +363,16 @@ 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"}}], @@ -393,6 +397,50 @@ 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: $Eff#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. diff --git a/lldb/unittests/Utility/CMakeLists.txt b/lldb/unittests/Utility/CMakeLists.txt --- a/lldb/unittests/Utility/CMakeLists.txt +++ b/lldb/unittests/Utility/CMakeLists.txt @@ -29,6 +29,7 @@ StatusTest.cpp StreamTeeTest.cpp StreamTest.cpp + StringExtractorGDBRemoteTest.cpp StringExtractorTest.cpp StringLexerTest.cpp StringListTest.cpp diff --git a/lldb/unittests/Utility/StringExtractorGDBRemoteTest.cpp b/lldb/unittests/Utility/StringExtractorGDBRemoteTest.cpp new file mode 100644 --- /dev/null +++ b/lldb/unittests/Utility/StringExtractorGDBRemoteTest.cpp @@ -0,0 +1,185 @@ +#include "gmock/gmock.h" +#include "gtest/gtest.h" +#include + +#include "lldb/Utility/StringExtractorGDBRemote.h" +#include "lldb/lldb-defines.h" + +TEST(StringExtractorGDBRemoteTest, GetPidTid) { + StringExtractorGDBRemote ex(""); + EXPECT_EQ(ex.GetPidTid(0), llvm::None); + + // invalid/short values + + ex.Reset("narf"); + EXPECT_EQ(ex.GetPidTid(0), llvm::None); + + ex.Reset(";1234"); + EXPECT_EQ(ex.GetPidTid(0), llvm::None); + + ex.Reset(".1234"); + EXPECT_EQ(ex.GetPidTid(0), llvm::None); + + ex.Reset("p"); + EXPECT_EQ(ex.GetPidTid(0), llvm::None); + + ex.Reset("pnarf"); + EXPECT_EQ(ex.GetPidTid(0), llvm::None); + + ex.Reset("p;1234"); + EXPECT_EQ(ex.GetPidTid(0), llvm::None); + + ex.Reset("p.1234"); + EXPECT_EQ(ex.GetPidTid(0), llvm::None); + + ex.Reset("p1234."); + EXPECT_EQ(ex.GetPidTid(0), llvm::None); + + EXPECT_EQ(ex.GetPidTid(0), llvm::None); + + ex.Reset("p1234.;1234"); + EXPECT_EQ(ex.GetPidTid(0), llvm::None); + + ex.Reset("-2"); + EXPECT_EQ(ex.GetPidTid(0), llvm::None); + + ex.Reset("p1234.-2"); + EXPECT_EQ(ex.GetPidTid(0), llvm::None); + + ex.Reset("p-2"); + EXPECT_EQ(ex.GetPidTid(0), llvm::None); + + ex.Reset("p-2.1234"); + EXPECT_EQ(ex.GetPidTid(0), llvm::None); + + // overflow + + ex.Reset("p10000000000000000"); + EXPECT_EQ(ex.GetPidTid(0), llvm::None); + + ex.Reset("p10000000000000000.0"); + EXPECT_EQ(ex.GetPidTid(0), llvm::None); + + ex.Reset("10000000000000000"); + EXPECT_EQ(ex.GetPidTid(0), llvm::None); + + ex.Reset("p0.10000000000000000"); + EXPECT_EQ(ex.GetPidTid(0), llvm::None); + + ex.Reset("p10000000000000000.10000000000000000"); + EXPECT_EQ(ex.GetPidTid(0), llvm::None); + + // invalid: all processes but specific thread + + ex.Reset("p-1.0"); + EXPECT_EQ(ex.GetPidTid(100), llvm::None); + + ex.Reset("p-1.1234"); + EXPECT_EQ(ex.GetPidTid(100), llvm::None); + + ex.Reset("p-1.123456789ABCDEF0"); + EXPECT_EQ(ex.GetPidTid(100), llvm::None); + + // unsupported: pid/tid 0 + + ex.Reset("0"); + EXPECT_EQ(ex.GetPidTid(100), llvm::None); + + ex.Reset("p0"); + EXPECT_EQ(ex.GetPidTid(100), llvm::None); + + ex.Reset("p0.0"); + EXPECT_EQ(ex.GetPidTid(100), llvm::None); + + ex.Reset("p0.-1"); + EXPECT_EQ(ex.GetPidTid(100), llvm::None); + + ex.Reset("p0.1234"); + EXPECT_EQ(ex.GetPidTid(100), llvm::None); + + ex.Reset("p0.123456789ABCDEF0"); + EXPECT_EQ(ex.GetPidTid(100), llvm::None); + + ex.Reset("p1234.0"); + EXPECT_EQ(ex.GetPidTid(100), llvm::None); + + ex.Reset("p123456789ABCDEF0.0"); + EXPECT_EQ(ex.GetPidTid(100), llvm::None); + + // pure thread id + + ex.Reset("-1"); + EXPECT_THAT(ex.GetPidTid(100).getValue(), + ::testing::Pair(100, StringExtractorGDBRemote::AllThreads)); + + ex.Reset("1234"); + EXPECT_THAT(ex.GetPidTid(100).getValue(), ::testing::Pair(100, 0x1234ULL)); + + ex.Reset("123456789ABCDEF0"); + EXPECT_THAT(ex.GetPidTid(100).getValue(), + ::testing::Pair(100, 0x123456789ABCDEF0ULL)); + + // pure process id + + ex.Reset("p-1"); + EXPECT_THAT(ex.GetPidTid(100).getValue(), + ::testing::Pair(StringExtractorGDBRemote::AllProcesses, + StringExtractorGDBRemote::AllThreads)); + + ex.Reset("p1234"); + EXPECT_THAT(ex.GetPidTid(100).getValue(), + ::testing::Pair(0x1234ULL, StringExtractorGDBRemote::AllThreads)); + + ex.Reset("p123456789ABCDEF0"); + EXPECT_THAT(ex.GetPidTid(100).getValue(), + ::testing::Pair(0x123456789ABCDEF0ULL, + StringExtractorGDBRemote::AllThreads)); + + ex.Reset("pFFFFFFFFFFFFFFFF"); + EXPECT_THAT(ex.GetPidTid(100).getValue(), + ::testing::Pair(StringExtractorGDBRemote::AllProcesses, + StringExtractorGDBRemote::AllThreads)); + + // combined thread id + process id + + ex.Reset("p-1.-1"); + EXPECT_THAT(ex.GetPidTid(100).getValue(), + ::testing::Pair(StringExtractorGDBRemote::AllProcesses, + StringExtractorGDBRemote::AllThreads)); + + ex.Reset("p1234.-1"); + EXPECT_THAT(ex.GetPidTid(100).getValue(), + ::testing::Pair(0x1234ULL, StringExtractorGDBRemote::AllThreads)); + + ex.Reset("p1234.123456789ABCDEF0"); + EXPECT_THAT(ex.GetPidTid(100).getValue(), + ::testing::Pair(0x1234ULL, 0x123456789ABCDEF0ULL)); + + ex.Reset("p123456789ABCDEF0.-1"); + EXPECT_THAT(ex.GetPidTid(100).getValue(), + ::testing::Pair(0x123456789ABCDEF0ULL, + StringExtractorGDBRemote::AllThreads)); + + ex.Reset("p123456789ABCDEF0.1234"); + EXPECT_THAT(ex.GetPidTid(100).getValue(), + ::testing::Pair(0x123456789ABCDEF0ULL, 0x1234ULL)); + + ex.Reset("p123456789ABCDEF0.123456789ABCDEF0"); + EXPECT_THAT(ex.GetPidTid(100).getValue(), + ::testing::Pair(0x123456789ABCDEF0ULL, 0x123456789ABCDEF0ULL)); + + ex.Reset("p123456789ABCDEF0.123456789ABCDEF0"); + EXPECT_THAT(ex.GetPidTid(100).getValue(), + ::testing::Pair(0x123456789ABCDEF0ULL, 0x123456789ABCDEF0ULL)); +} + +TEST(StringExtractorGDBRemoteTest, GetPidTidMultipleValues) { + StringExtractorGDBRemote ex("1234;p12;p1234.-1"); + ASSERT_THAT(ex.GetPidTid(100).getValue(), ::testing::Pair(100, 0x1234ULL)); + ASSERT_EQ(ex.GetChar(), ';'); + ASSERT_THAT(ex.GetPidTid(100).getValue(), + ::testing::Pair(0x12ULL, StringExtractorGDBRemote::AllThreads)); + ASSERT_EQ(ex.GetChar(), ';'); + ASSERT_THAT(ex.GetPidTid(100).getValue(), + ::testing::Pair(0x1234ULL, StringExtractorGDBRemote::AllThreads)); +}