Author: Michał Górny Date: 2022-07-15T13:43:34+02:00 New Revision: 7a2b09b4798dbd5ec69e934b595eab5afddf33c5
URL: https://github.com/llvm/llvm-project/commit/7a2b09b4798dbd5ec69e934b595eab5afddf33c5 DIFF: https://github.com/llvm/llvm-project/commit/7a2b09b4798dbd5ec69e934b595eab5afddf33c5.diff LOG: Revert "[lldb] [llgs] Fix multi-resume bugs with nonstop mode" This reverts commit f8605da8758fbae16410e4ed5493a39429fd73ec. This is causing buildbot failures and now I see that I have not updated the tests to use "stop" instead of "trap". Added: Modified: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h lldb/test/API/tools/lldb-server/TestNonStop.py lldb/test/API/tools/lldb-server/vCont-threads/TestPartialResume.py Removed: ################################################################################ diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp index 8edb49f06a334..a2ea53b0e8e19 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp @@ -1512,30 +1512,6 @@ GDBRemoteCommunicationServerLLGS::Handle_QListThreadsInStopReply( return SendOKResponse(); } -GDBRemoteCommunication::PacketResult -GDBRemoteCommunicationServerLLGS::ResumeProcess( - NativeProcessProtocol &process, const ResumeActionList &actions) { - Log *log = GetLog(LLDBLog::Process | LLDBLog::Thread); - - // In non-stop protocol mode, the process could be running already. - // We do not support resuming threads independently, so just error out. - if (!process.CanResume()) { - LLDB_LOG(log, "process {0} cannot be resumed (state={1})", process.GetID(), - process.GetState()); - return SendErrorResponse(0x37); - } - - Status error = process.Resume(actions); - if (error.Fail()) { - LLDB_LOG(log, "process {0} failed to resume: {1}", process.GetID(), error); - return SendErrorResponse(GDBRemoteServerError::eErrorResume); - } - - LLDB_LOG(log, "process {0} resumed", process.GetID()); - - return PacketResult::Success; -} - GDBRemoteCommunication::PacketResult GDBRemoteCommunicationServerLLGS::Handle_C(StringExtractorGDBRemote &packet) { Log *log = GetLog(LLDBLog::Process | LLDBLog::Thread); @@ -1571,14 +1547,6 @@ GDBRemoteCommunicationServerLLGS::Handle_C(StringExtractorGDBRemote &packet) { packet, "unexpected content after $C{signal-number}"); } - // In non-stop protocol mode, the process could be running already. - // We do not support resuming threads independently, so just error out. - if (!m_continue_process->CanResume()) { - LLDB_LOG(log, "process cannot be resumed (state={0})", - m_continue_process->GetState()); - return SendErrorResponse(0x37); - } - ResumeActionList resume_actions(StateType::eStateRunning, LLDB_INVALID_SIGNAL_NUMBER); Status error; @@ -1612,11 +1580,14 @@ GDBRemoteCommunicationServerLLGS::Handle_C(StringExtractorGDBRemote &packet) { } } - // NB: this checks CanResume() twice but using a single code path for - // resuming still seems worth it. - PacketResult resume_res = ResumeProcess(*m_continue_process, resume_actions); - if (resume_res != PacketResult::Success) - return resume_res; + // Resume the threads. + error = m_continue_process->Resume(resume_actions); + if (error.Fail()) { + LLDB_LOG(log, "failed to resume threads for process {0}: {1}", + m_continue_process->GetID(), error); + + return SendErrorResponse(0x38); + } // Don't send an "OK" packet, except in non-stop mode; // otherwise, the response is the stopped/exited message. @@ -1651,9 +1622,14 @@ GDBRemoteCommunicationServerLLGS::Handle_c(StringExtractorGDBRemote &packet) { ResumeActionList actions(StateType::eStateRunning, LLDB_INVALID_SIGNAL_NUMBER); - PacketResult resume_res = ResumeProcess(*m_continue_process, actions); - if (resume_res != PacketResult::Success) - return resume_res; + Status error = m_continue_process->Resume(actions); + if (error.Fail()) { + LLDB_LOG(log, "c failed for process {0}: {1}", m_continue_process->GetID(), + error); + return SendErrorResponse(GDBRemoteServerError::eErrorResume); + } + + LLDB_LOG(log, "continued process {0}", m_continue_process->GetID()); return SendContinueSuccessResponse(); } @@ -1667,18 +1643,6 @@ GDBRemoteCommunicationServerLLGS::Handle_vCont_actions( return SendPacketNoLock(response.GetString()); } -static bool ResumeActionListStopsAllThreads(ResumeActionList &actions) { - // We're doing a stop-all if and only if our only action is a "t" for all - // threads. - if (const ResumeAction *default_action = - actions.GetActionForThread(LLDB_INVALID_THREAD_ID, false)) { - if (default_action->state == eStateSuspended && actions.GetSize() == 1) - return true; - } - - return false; -} - GDBRemoteCommunication::PacketResult GDBRemoteCommunicationServerLLGS::Handle_vCont( StringExtractorGDBRemote &packet) { @@ -1700,6 +1664,9 @@ GDBRemoteCommunicationServerLLGS::Handle_vCont( // Move past the ';', then do a simple 's'. packet.SetFilePos(packet.GetFilePos() + 1); return Handle_s(packet); + } else if (m_non_stop && ::strcmp(packet.Peek(), ";t") == 0) { + // TODO: add full support for "t" action + return SendOKResponse(); } std::unordered_map<lldb::pid_t, ResumeActionList> thread_actions; @@ -1766,12 +1733,6 @@ GDBRemoteCommunicationServerLLGS::Handle_vCont( tid = pid_tid->second; } - if (thread_action.state == eStateSuspended && - tid != StringExtractorGDBRemote::AllThreads) { - return SendIllFormedResponse( - packet, "'t' action not supported for individual threads"); - } - if (pid == StringExtractorGDBRemote::AllProcesses) { if (m_debugged_processes.size() > 1) return SendIllFormedResponse( @@ -1804,43 +1765,13 @@ GDBRemoteCommunicationServerLLGS::Handle_vCont( return SendErrorResponse(GDBRemoteServerError::eErrorResume); } - // There are four possible scenarios here. These are: - // 1. vCont on a stopped process that resumes at least one thread. - // In this case, we call Resume(). - // 2. vCont on a stopped process that leaves all threads suspended. - // A no-op. - // 3. vCont on a running process that requests suspending all - // running threads. In this case, we call Interrupt(). - // 4. vCont on a running process that requests suspending a subset - // of running threads or resuming a subset of suspended threads. - // Since we do not support full nonstop mode, this is unsupported - // and we return an error. - - assert(process_it->second.process_up); - if (ResumeActionListStopsAllThreads(x.second)) { - if (process_it->second.process_up->IsRunning()) { - assert(m_non_stop); - - Status error = process_it->second.process_up->Interrupt(); - if (error.Fail()) { - LLDB_LOG(log, "vCont failed to halt process {0}: {1}", x.first, - error); - return SendErrorResponse(GDBRemoteServerError::eErrorResume); - } - - LLDB_LOG(log, "halted process {0}", x.first); - - // hack to avoid enabling stdio forwarding after stop - // TODO: remove this when we improve stdio forwarding for nonstop - assert(thread_actions.size() == 1); - return SendOKResponse(); - } - } else { - PacketResult resume_res = - ResumeProcess(*process_it->second.process_up, x.second); - if (resume_res != PacketResult::Success) - return resume_res; + Status error = process_it->second.process_up->Resume(x.second); + if (error.Fail()) { + LLDB_LOG(log, "vCont failed for process {0}: {1}", x.first, error); + return SendErrorResponse(GDBRemoteServerError::eErrorResume); } + + LLDB_LOG(log, "continued process {0}", x.first); } return SendContinueSuccessResponse(); @@ -3009,10 +2940,15 @@ GDBRemoteCommunicationServerLLGS::Handle_s(StringExtractorGDBRemote &packet) { // All other threads stop while we're single stepping a thread. actions.SetDefaultThreadActionIfNeeded(eStateStopped, 0); - - PacketResult resume_res = ResumeProcess(*m_continue_process, actions); - if (resume_res != PacketResult::Success) - return resume_res; + Status error = m_continue_process->Resume(actions); + if (error.Fail()) { + LLDB_LOGF(log, + "GDBRemoteCommunicationServerLLGS::%s pid %" PRIu64 + " tid %" PRIu64 " Resume() failed with error: %s", + __FUNCTION__, m_continue_process->GetID(), tid, + error.AsCString()); + return SendErrorResponse(0x49); + } // No response here, unless in non-stop mode. // Otherwise, the stop or exit will come from the resulting action. diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h index 47e8a783603de..ebd656687da93 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h @@ -155,9 +155,6 @@ class GDBRemoteCommunicationServerLLGS PacketResult Handle_QListThreadsInStopReply(StringExtractorGDBRemote &packet); - PacketResult ResumeProcess(NativeProcessProtocol &process, - const ResumeActionList &actions); - PacketResult Handle_C(StringExtractorGDBRemote &packet); PacketResult Handle_c(StringExtractorGDBRemote &packet); diff --git a/lldb/test/API/tools/lldb-server/TestNonStop.py b/lldb/test/API/tools/lldb-server/TestNonStop.py index d544cdd45c48e..83772ada78e30 100644 --- a/lldb/test/API/tools/lldb-server/TestNonStop.py +++ b/lldb/test/API/tools/lldb-server/TestNonStop.py @@ -168,137 +168,3 @@ def test_exit_query(self): "send packet: $OK#00", ], True) self.expect_gdbremote_sequence() - - def multiple_resume_test(self, second_command): - self.build() - self.set_inferior_startup_launch() - procs = self.prep_debug_monitor_and_inferior( - inferior_args=["sleep:15"]) - self.test_sequence.add_log_lines( - ["read packet: $QNonStop:1#00", - "send packet: $OK#00", - "read packet: $c#63", - "send packet: $OK#00", - "read packet: ${}#00".format(second_command), - "send packet: $E37#00", - ], True) - self.expect_gdbremote_sequence() - - @add_test_categories(["llgs"]) - def test_multiple_C(self): - self.multiple_resume_test("C05") - - @add_test_categories(["llgs"]) - def test_multiple_c(self): - self.multiple_resume_test("c") - - @add_test_categories(["llgs"]) - def test_multiple_s(self): - self.multiple_resume_test("s") - - @expectedFailureAll(archs=["arm"]) # TODO - @expectedFailureAll(archs=["aarch64"], - bugnumber="https://github.com/llvm/llvm-project/issues/56268";) - @add_test_categories(["llgs"]) - def test_multiple_vCont(self): - self.build() - self.set_inferior_startup_launch() - procs = self.prep_debug_monitor_and_inferior( - inferior_args=["thread:new", "trap", "sleep:15"]) - self.test_sequence.add_log_lines( - ["read packet: $QNonStop:1#00", - "send packet: $OK#00", - "read packet: $c#63", - "send packet: $OK#00", - {"direction": "send", - "regex": r"^%Stop:T[0-9a-fA-F]{2}thread:([0-9a-fA-F]+);", - "capture": {1: "tid1"}, - }, - "read packet: $vStopped#63", - {"direction": "send", - "regex": r"^[$]T[0-9a-fA-F]{2}thread:([0-9a-fA-F]+);", - "capture": {1: "tid2"}, - }, - "read packet: $vStopped#63", - "send packet: $OK#00", - ], True) - ret = self.expect_gdbremote_sequence() - - self.reset_test_sequence() - self.test_sequence.add_log_lines( - ["read packet: $vCont;c:{}#00".format(ret["tid1"]), - "send packet: $OK#00", - "read packet: $vCont;c:{}#00".format(ret["tid2"]), - "send packet: $E37#00", - ], True) - self.expect_gdbremote_sequence() - - @add_test_categories(["llgs"]) - def test_vCont_then_stop(self): - self.build() - self.set_inferior_startup_launch() - procs = self.prep_debug_monitor_and_inferior( - inferior_args=["sleep:15"]) - self.test_sequence.add_log_lines( - ["read packet: $QNonStop:1#00", - "send packet: $OK#00", - "read packet: $c#63", - "send packet: $OK#00", - "read packet: $vCont;t#00", - "send packet: $OK#00", - ], True) - self.expect_gdbremote_sequence() - - def vCont_then_partial_stop_test(self, run_both): - self.build() - self.set_inferior_startup_launch() - procs = self.prep_debug_monitor_and_inferior( - inferior_args=["thread:new", "trap", "sleep:15"]) - self.test_sequence.add_log_lines( - ["read packet: $QNonStop:1#00", - "send packet: $OK#00", - "read packet: $c#63", - "send packet: $OK#00", - {"direction": "send", - "regex": r"^%Stop:T[0-9a-fA-F]{2}thread:([0-9a-fA-F]+);", - "capture": {1: "tid1"}, - }, - "read packet: $vStopped#63", - {"direction": "send", - "regex": r"^[$]T[0-9a-fA-F]{2}thread:([0-9a-fA-F]+);", - "capture": {1: "tid2"}, - }, - "read packet: $vStopped#63", - "send packet: $OK#00", - ], True) - ret = self.expect_gdbremote_sequence() - - self.reset_test_sequence() - if run_both: - self.test_sequence.add_log_lines( - ["read packet: $vCont;c#00", - ], True) - else: - self.test_sequence.add_log_lines( - ["read packet: $vCont;c:{}#00".format(ret["tid1"]), - ], True) - self.test_sequence.add_log_lines( - ["send packet: $OK#00", - "read packet: $vCont;t:{}#00".format(ret["tid2"]), - "send packet: $E03#00", - ], True) - self.expect_gdbremote_sequence() - - @expectedFailureAll(archs=["arm"]) # TODO - @expectedFailureAll(archs=["aarch64"], - bugnumber="https://github.com/llvm/llvm-project/issues/56268";) - @add_test_categories(["llgs"]) - def test_vCont_then_partial_stop(self): - self.vCont_then_partial_stop_test(False) - - @expectedFailureAll(archs=["arm"]) # TODO - @expectedFailureAll(archs=["aarch64"], - bugnumber="https://github.com/llvm/llvm-project/issues/56268";) - @add_test_categories(["llgs"]) - def test_vCont_then_partial_stop_run_both(self): - self.vCont_then_partial_stop_test(True) diff --git a/lldb/test/API/tools/lldb-server/vCont-threads/TestPartialResume.py b/lldb/test/API/tools/lldb-server/vCont-threads/TestPartialResume.py index b98b63ea22e03..2cc60d3d0a5c0 100644 --- a/lldb/test/API/tools/lldb-server/vCont-threads/TestPartialResume.py +++ b/lldb/test/API/tools/lldb-server/vCont-threads/TestPartialResume.py @@ -75,3 +75,54 @@ def test_vCont_cxcxt(self): main_thread, "c:{:x};c:{:x};t".format(*all_subthreads_list[:2])), set(all_subthreads_list[:2])) + + @skipIfWindows + @add_test_categories(["llgs"]) + def test_vCont_txc(self): + main_thread, all_subthreads_list = ( + self.start_vCont_run_subset_of_threads_test()) + # stop one thread explicitly, resume others + self.assertEqual( + self.continue_and_get_threads_running( + main_thread, + "t:{:x};c".format(all_subthreads_list[-1])), + set(all_subthreads_list[:2])) + + @skipIfWindows + @add_test_categories(["llgs"]) + def test_vCont_cxtxc(self): + main_thread, all_subthreads_list = ( + self.start_vCont_run_subset_of_threads_test()) + # resume one thread explicitly, stop one explicitly, + # resume others + self.assertEqual( + self.continue_and_get_threads_running( + main_thread, + "c:{:x};t:{:x};c".format(*all_subthreads_list[-2:])), + set(all_subthreads_list[:2])) + + @skipIfWindows + @add_test_categories(["llgs"]) + def test_vCont_txcx(self): + main_thread, all_subthreads_list = ( + self.start_vCont_run_subset_of_threads_test()) + # resume one thread explicitly, stop one explicitly, + # stop others implicitly + self.assertEqual( + self.continue_and_get_threads_running( + main_thread, + "t:{:x};c:{:x}".format(*all_subthreads_list[:2])), + set(all_subthreads_list[1:2])) + + @skipIfWindows + @add_test_categories(["llgs"]) + def test_vCont_txcxt(self): + main_thread, all_subthreads_list = ( + self.start_vCont_run_subset_of_threads_test()) + # resume one thread explicitly, stop one explicitly, + # stop others explicitly + self.assertEqual( + self.continue_and_get_threads_running( + main_thread, + "t:{:x};c:{:x};t".format(*all_subthreads_list[:2])), + set(all_subthreads_list[1:2])) _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
