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 <shinga...@labware.com>
Maintainer: Boris Shingarov <shinga...@labware.com>
Tested-by: kokoro <noreply+kok...@google.com>
---
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 @@
clearSingleStep();
- /*
- * The first entry to this function is normally through
- * a breakpoint trap in kgdb_connect(), in which case we
- * must advance past the breakpoint because gdb will not.
- *
- * On the first entry here, we expect that gdb is not yet
- * listening to us, so just enter the interaction loop.
- * After the debugger is "active" (connected) it will be
- * waiting for a "signaled" message from us.
- */
- if (!active) {
- active = true;
- } else if (threadSwitching) {
+ if (threadSwitching) {
threadSwitching = false;
// Tell GDB the thread switch has completed.
send("OK");
@@ -523,6 +541,19 @@
}
void
+BaseRemoteGDB::incomingConnection(int revent)
+{
+ if (connectEvent.scheduled()) {
+ warn("GDB connect event has already been scheduled!");
+ return;
+ }
+
+ if (revent & POLLIN) {
+ scheduleInstCommitEvent(&connectEvent, 0);
+ }
+}
+
+void
BaseRemoteGDB::incomingData(int revent)
{
if (trapEvent.scheduled()) {
@@ -536,7 +567,7 @@
scheduleInstCommitEvent(&trapEvent, 0);
} else if (revent & POLLNVAL) {
descheduleInstCommitEvent(&trapEvent);
- detach();
+ scheduleInstCommitEvent(&disconnectEvent, 0);
}
}
diff --git a/src/base/remote_gdb.hh b/src/base/remote_gdb.hh
index f04bc9d..2248d71 100644
--- a/src/base/remote_gdb.hh
+++ b/src/base/remote_gdb.hh
@@ -190,8 +190,19 @@
/*
* Connection to the external GDB.
*/
+
+ /*
+ * Asynchronous socket events and event handlers.
+ *
+ * These events occur asynchronously and are handled asynchronously
+ * to main simulation loop - therefore they *shall not* interact with
+ * rest of gem5.
+ *
+ * The only thing they do is to schedule a synchronous event at
instruction
+ * boundary to deal with the request.
+ */
void incomingData(int revent);
- void connectWrapper(int revent) { connect(); }
+ void incomingConnection(int revent);
template <void (BaseRemoteGDB::*F)(int revent)>
class SocketEvent : public PollEvent
@@ -207,14 +218,16 @@
void process(int revent) { (gdb->*F)(revent); }
};
- typedef SocketEvent<&BaseRemoteGDB::connectWrapper> ConnectEvent;
- typedef SocketEvent<&BaseRemoteGDB::incomingData> DataEvent;
+ typedef SocketEvent<&BaseRemoteGDB::incomingConnection>
+ IncomingConnectionEvent;
+ typedef SocketEvent<&BaseRemoteGDB::incomingData>
+ IncomingDataEvent;
- friend ConnectEvent;
- friend DataEvent;
+ friend IncomingConnectionEvent;
+ friend IncomingDataEvent;
- ConnectEvent *connectEvent;
- DataEvent *dataEvent;
+ IncomingConnectionEvent *incomingConnectionEvent;
+ IncomingDataEvent *incomingDataEvent;
ListenSocket listener;
int _port;
@@ -248,7 +261,6 @@
/*
* Simulator side debugger state.
*/
- bool active = false;
bool attached = false;
bool threadSwitching = false;
@@ -259,6 +271,9 @@
BaseGdbRegCache *regCachePtr = nullptr;
+ EventWrapper<BaseRemoteGDB, &BaseRemoteGDB::connect> connectEvent;
+ EventWrapper<BaseRemoteGDB, &BaseRemoteGDB::detach> disconnectEvent;
+
class TrapEvent : public Event
{
protected:
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/48182
To unsubscribe, or for help writing mail filters, visit
https://gem5-review.googlesource.com/settings
Gerrit-Project: public/gem5
Gerrit-Branch: develop
Gerrit-Change-Id: I33b2922ba017205acabd51b6a8be3e6fb2d6409a
Gerrit-Change-Number: 48182
Gerrit-PatchSet: 5
Gerrit-Owner: Jan Vrany <jan.vr...@labware.com>
Gerrit-Reviewer: Bobby R. Bruce <bbr...@ucdavis.edu>
Gerrit-Reviewer: Boris Shingarov <shinga...@labware.com>
Gerrit-Reviewer: Ciro Santilli <ciro.santi...@arm.com>
Gerrit-Reviewer: Daniel Carvalho <oda...@yahoo.com.br>
Gerrit-Reviewer: Gabe Black <gabe.bl...@gmail.com>
Gerrit-Reviewer: Jan Vrany <jan.vr...@labware.com>
Gerrit-Reviewer: kokoro <noreply+kok...@google.com>
Gerrit-MessageType: merged
_______________________________________________
gem5-dev mailing list -- gem5-dev@gem5.org
To unsubscribe send an email to gem5-dev-le...@gem5.org
%(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
