Jan Vrany has submitted this change. (
https://gem5-review.googlesource.com/c/public/gem5/+/48182 )
Change subject: base: handle initial communication with GDB in `attach()`
..
base: handle initial communication with GDB in `attach()`
When remote GDB attaches to gem5, handle the initial communication
(`qSupported` and alike) right away instead of scheduling a `DataEvent`
and firing the simulation loop in hope that GDB will be quick enough to
send initial packets before instructions are dispatched.
This requires attach() to be always called at instruction boundary
to make it safe to interact with the rest of gem5.
When `--wait-gdb` is used, connect() is called from workflow startup,
therefore on an instruction boundary and therefore needs not special
handling.
To handle case the GDB connects while simulation is already running,
we arrange (new) assynchronous IncommingConnectionEvent on listening
socket that, when there's a new connection being made, *only* schedules
*synchronous* ConnectEvent that handles the rest, *including* calling
an accept() on listening socket. This way it is safe to process commands
in attach().
In order to make the code more systematic and easier to understands,
detach() is also made to be called only synchronously (that is, at
intruction boundary). Asynchronous events and event handlers are
prefixed with "incoming".
This seems to fix the race described in 44612 [1]
[1]: https://gem5-review.googlesource.com/c/public/gem5/+/44612
Change-Id: I33b2922ba017205acabd51b6a8be3e6fb2d6409a
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/48182
Reviewed-by: Boris Shingarov
Maintainer: Boris Shingarov
Tested-by: kokoro
---
M src/base/remote_gdb.cc
M src/base/remote_gdb.hh
2 files changed, 79 insertions(+), 33 deletions(-)
Approvals:
Boris Shingarov: Looks good to me, approved; Looks good to me, approved
kokoro: Regressions pass
diff --git a/src/base/remote_gdb.cc b/src/base/remote_gdb.cc
index b53cbc0..1437b75 100644
--- a/src/base/remote_gdb.cc
+++ b/src/base/remote_gdb.cc
@@ -356,14 +356,16 @@
}
BaseRemoteGDB::BaseRemoteGDB(System *_system, int _port) :
-connectEvent(nullptr), dataEvent(nullptr), _port(_port), fd(-1),
-sys(_system), trapEvent(this), singleStepEvent(*this)
+incomingConnectionEvent(nullptr), incomingDataEvent(nullptr),
+_port(_port), fd(-1), sys(_system),
+connectEvent(this), disconnectEvent(this), trapEvent(this),
+singleStepEvent(*this)
{}
BaseRemoteGDB::~BaseRemoteGDB()
{
-delete connectEvent;
-delete dataEvent;
+delete incomingConnectionEvent;
+delete incomingDataEvent;
}
std::string
@@ -385,8 +387,9 @@
_port++;
}
-connectEvent = new ConnectEvent(this, listener.getfd(), POLLIN);
-pollQueue.schedule(connectEvent);
+incomingConnectionEvent =
+new IncomingConnectionEvent(this, listener.getfd(), POLLIN);
+pollQueue.schedule(incomingConnectionEvent);
ccprintf(std::cerr, "%d: %s: listening for remote gdb on port %d\n",
curTick(), name(), _port);
@@ -398,6 +401,8 @@
panic_if(!listener.islistening(),
"Can't accept GDB connections without any threads!");
+pollQueue.remove(incomingConnectionEvent);
+
int sfd = listener.accept(true);
if (sfd != -1) {
@@ -421,23 +426,48 @@
{
fd = f;
-dataEvent = new DataEvent(this, fd, POLLIN);
-pollQueue.schedule(dataEvent);
-
attached = true;
DPRINTFN("remote gdb attached\n");
+
+processCommands();
+
+if (isAttached()) {
+// At this point an initial communication with GDB is handled
+// and we're ready to continue. Here we arrange IncomingDataEvent
+// to get notified when GDB breaks in.
+//
+// However, GDB can decide to disconnect during that initial
+// communication. In that case, we cannot arrange data event
because
+// the socket is already closed (not that it makes any sense,
anyways).
+//
+// Hence the check above.
+incomingDataEvent = new IncomingDataEvent(this, fd, POLLIN);
+pollQueue.schedule(incomingDataEvent);
+}
}
void
BaseRemoteGDB::detach()
{
attached = false;
-active = false;
clearSingleStep();
close(fd);
fd = -1;
-pollQueue.remove(dataEvent);
+if (incomingDataEvent) {
+// incomingDataEvent gets scheduled in attach() after
+// initial communication with GDB is handled and GDB tells
+// gem5 to continue.
+//
+// GDB can disconnect before that in which case `incomingDataEvent`
+// is NULL.
+//
+// Hence the check above.
+
+pollQueue.remove(incomingDataEvent);
+incomingDataEvent = nullptr;
+}
+pollQueue.schedule(incomingConnectionEvent);
DPRINTFN("remote gdb detached\n");
}
@@ -498,19 +528,7 @@
c
