[Lldb-commits] [PATCH] D101933: If an interrupt fails, don't try to fetch any more packets from the server
jingham added a comment. I moved the eStateExited test as Greg suggested on commit. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101933/new/ https://reviews.llvm.org/D101933 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D101933: If an interrupt fails, don't try to fetch any more packets from the server
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG72ba78c29e92: When SendContinuePacketAndWaitForResponse
returns eStateInvalid, don't fetch… (authored by jingham).
Changed prior to commit:
https://reviews.llvm.org/D101933?vs=343131&id=343499#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D101933/new/
https://reviews.llvm.org/D101933
Files:
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/test/API/functionalities/gdb_remote_client/TestHaltFails.py
Index: lldb/test/API/functionalities/gdb_remote_client/TestHaltFails.py
===
--- /dev/null
+++ lldb/test/API/functionalities/gdb_remote_client/TestHaltFails.py
@@ -0,0 +1,72 @@
+from __future__ import print_function
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from gdbclientutils import *
+
+
+class TestHaltFails(GDBRemoteTestBase):
+
+class MyResponder(MockGDBServerResponder):
+
+def setBreakpoint(self, packet):
+return "OK"
+
+def interrupt(self):
+# Simulate process waiting longer than the interrupt
+# timeout to stop, then sending the reply.
+time.sleep(14)
+return "T02reason:signal"
+
+def cont(self):
+# No response, wait for the client to interrupt us.
+return None
+
+def wait_for_and_check_event(self, wait_time, value):
+event = lldb.SBEvent()
+got_event = self.dbg.GetListener().WaitForEvent(wait_time, event)
+self.assertTrue(got_event, "Failed to get event after wait")
+self.assertTrue(lldb.SBProcess.EventIsProcessEvent(event), "Event was not a process event")
+event_type = lldb.SBProcess.GetStateFromEvent(event)
+self.assertEqual(event_type, value)
+
+def get_to_running(self):
+self.server.responder = self.MyResponder()
+self.target = self.createTarget("a.yaml")
+process = self.connect(self.target)
+self.dbg.SetAsync(True)
+
+# There should be a stopped event, consume that:
+self.wait_for_and_check_event(2, lldb.eStateStopped)
+process.Continue()
+
+# There should be a running event, consume that:
+self.wait_for_and_check_event(2, lldb.eStateRunning)
+return process
+
+@skipIfReproducer # FIXME: Unexpected packet during (passive) replay
+def test_destroy_while_running(self):
+process = self.get_to_running()
+process.Destroy()
+
+# Again pretend that after failing to be interrupted, we delivered the stop
+# and make sure we still exit properly.
+self.wait_for_and_check_event(14, lldb.eStateExited)
+
+@skipIfReproducer # FIXME: Unexpected packet during (passive) replay
+def test_async_interrupt(self):
+"""
+Test that explicitly calling AsyncInterrupt, which then fails, leads
+to an "eStateExited" state.
+"""
+process = self.get_to_running()
+# Now do the interrupt:
+process.SendAsyncInterrupt()
+
+# That should have caused the Halt to time out and we should
+# be in eStateExited:
+self.wait_for_and_check_event(15, lldb.eStateExited)
+
+
+
+
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -3689,12 +3689,25 @@
__FUNCTION__, arg, process->GetID());
EventSP event_sp;
+
+ // We need to ignore any packets that come in after we have
+ // have decided the process has exited. There are some
+ // situations, for instance when we try to interrupt a running
+ // process and the interrupt fails, where another packet might
+ // get delivered after we've decided to give up on the process.
+ // But once we've decided we are done with the process we will
+ // not be in a state to do anything useful with new packets.
+ // So it is safer to simply ignore any remaining packets by
+ // explicitly checking for eStateExited before reentering the
+ // fetch loop.
+
bool done = false;
- while (!done) {
+ while (!done && process->GetPrivateState() != eStateExited) {
LLDB_LOGF(log,
"ProcessGDBRemote::%s (arg = %p, pid = %" PRIu64
") listener.WaitForEvent (NULL, event_sp)...",
__FUNCTION__, arg, process->GetID());
+
if (process->m_async_listener_sp->GetEvent(event_sp, llvm::None)) {
const uint32_t event_type = event_sp->GetType();
if (event_sp->BroadcasterIs(&process->m_async_broadcaster)) {
@@ -3793,6 +3806,7 @@
} else {
[Lldb-commits] [PATCH] D101933: If an interrupt fails, don't try to fetch any more packets from the server
clayborg accepted this revision. clayborg added inline comments. This revision is now accepted and ready to land. Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:3698-3701 +// If the process has exited, don't try to fetch more events from the +// server for it. We wouldn't be in a correct state to handle them. +if (process->GetPrivateState() == eStateExited) + break; You could put this up into the while condition? ``` while (!done && process->GetPrivateState() != eStateExited) ``` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101933/new/ https://reviews.llvm.org/D101933 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D101933: If an interrupt fails, don't try to fetch any more packets from the server
jingham created this revision.
jingham added reviewers: clayborg, JDevlieghere, labath.
jingham requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
When a gdb-remote plugin process doesn't respond to an interrupt, it returns
from
SendContinuePacketAndWaitForResponse with eStateInvalid. The AsyncThread
code responds to this return by setting the state to eStateExited. But it
doesn't
immediately exit the ProcessGDBRemote::AsyncThread's packet fetch loop.
This looks like just an oversight in the AsyncThread function. It's important
not to
go back to wait for another packet. If it arrives while you are tearing down
the
process, the internal-state-thread might try to handle it when the process in
not
in a good state.
We weren't going to do anything useful with the extra packet anyway, we've
already
declared the process exited. So rather than put more checks into all the
shutdown paths
to make sure this extra packet doesn't cause problems, just don't fetch it.
The main part of the patch is setting "done = true" when we get the
eStateInvalid.
I also added a check at the beginning of the while(done) loop to prevent
another error
from getting us to fetch packets for an exited process.
I also added a test case to ensure that if an Interrupt fails, we call the
process
exited. I can't test exactly the error I'm fixing, there's no good way to know
that the stop reply for the failed interrupt wasn't fetched. But at least this
asserts that the overall behavior is correct.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D101933
Files:
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/test/API/functionalities/gdb_remote_client/TestHaltFails.py
Index: lldb/test/API/functionalities/gdb_remote_client/TestHaltFails.py
===
--- /dev/null
+++ lldb/test/API/functionalities/gdb_remote_client/TestHaltFails.py
@@ -0,0 +1,72 @@
+from __future__ import print_function
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from gdbclientutils import *
+
+
+class TestHaltFails(GDBRemoteTestBase):
+
+class MyResponder(MockGDBServerResponder):
+
+def setBreakpoint(self, packet):
+return "OK"
+
+def interrupt(self):
+# Simulate process waiting longer than the interrupt
+# timeout to stop, then sending the reply.
+time.sleep(14)
+return "T02reason:signal"
+
+def cont(self):
+# No response, wait for the client to interrupt us.
+return None
+
+def wait_for_and_check_event(self, wait_time, value):
+event = lldb.SBEvent()
+got_event = self.dbg.GetListener().WaitForEvent(wait_time, event)
+self.assertTrue(got_event, "Failed to get event after wait")
+self.assertTrue(lldb.SBProcess.EventIsProcessEvent(event), "Event was not a process event")
+event_type = lldb.SBProcess.GetStateFromEvent(event)
+self.assertEqual(event_type, value)
+
+def get_to_running(self):
+self.server.responder = self.MyResponder()
+self.target = self.createTarget("a.yaml")
+process = self.connect(self.target)
+self.dbg.SetAsync(True)
+
+# There should be a stopped event, consume that:
+self.wait_for_and_check_event(2, lldb.eStateStopped)
+process.Continue()
+
+# There should be a running event, consume that:
+self.wait_for_and_check_event(2, lldb.eStateRunning)
+return process
+
+@skipIfReproducer # FIXME: Unexpected packet during (passive) replay
+def test_destroy_while_running(self):
+process = self.get_to_running()
+process.Destroy()
+
+# Again pretend that after failing to be interrupted, we delivered the stop
+# and make sure we still exit properly.
+self.wait_for_and_check_event(14, lldb.eStateExited)
+
+@skipIfReproducer # FIXME: Unexpected packet during (passive) replay
+def test_async_interrupt(self):
+"""
+Test that explicitly calling AsyncInterrupt, which then fails, leads
+to an "eStateExited" state.
+"""
+process = self.get_to_running()
+# Now do the interrupt:
+process.SendAsyncInterrupt()
+
+# That should have caused the Halt to time out and we should
+# be in eStateExited:
+self.wait_for_and_check_event(15, lldb.eStateExited)
+
+
+
+
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -3695,6 +3695,11 @@
"ProcessGDBRemote::%s (arg = %p, pid = %" PRIu64
