From 35bf30f5a7d865e18f90007bbec9d499d9872950 Mon Sep 17 00:00:00 2001 From: Bob131 Date: Sun, 30 May 2021 04:17:15 +1000 Subject: [PATCH 1/3] test: use REPLAY_ARGS environment variable Updates the test harness to use $REPLAY_ARGS as additional arguments to 'rr replay'. --- src/test/util.sh | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/test/util.sh b/src/test/util.sh index 38c172eef7d..6521ffc851c 100644 --- a/src/test/util.sh +++ b/src/test/util.sh @@ -17,7 +17,9 @@ # # Test runners may set the environment variable $RECORD_ARGS to pass # arguments to rr for recording. This is only useful for tweaking the -# scheduler, don't use it for anything else. +# scheduler, don't use it for anything else. Test runners may also +# set $REPLAY_ARGS to pass arguments to rr for replay, allowing the +# use of an alternate GDB, for instance. # # delay_kill @@ -277,7 +279,7 @@ function record_async_signal { sig=$1; delay_secs=$2; exe=$3; exeargs=$4; function replay { replayflags=$1 _RR_TRACE_DIR="$workdir" test-monitor $TIMEOUT replay.err \ - $RR_EXE $GLOBAL_OPTIONS replay -a $replayflags 1> replay.out 2> replay.err + $RR_EXE $GLOBAL_OPTIONS replay -a $replayflags $REPLAY_ARGS 1> replay.out 2> replay.err } function rerun { rerunflags=$1 @@ -296,7 +298,7 @@ function do_ps { psflags=$1 function debug { expectscript=$1; replayargs=$2 _RR_TRACE_DIR="$workdir" test-monitor $TIMEOUT debug.err \ python3 $TESTDIR/$expectscript.py \ - $RR_EXE $GLOBAL_OPTIONS replay -o-n -x $TESTDIR/test_setup.gdb $replayargs + $RR_EXE $GLOBAL_OPTIONS replay -o-n -x $TESTDIR/test_setup.gdb $replayargs $REPLAY_ARGS if [[ $? == 0 ]]; then passed else From a46fdbad0f1bb1aafc6ba679f6506d1ef8a11c7f Mon Sep 17 00:00:00 2001 From: Bob131 Date: Sun, 30 May 2021 04:20:26 +1000 Subject: [PATCH 2/3] GDB: add support for vReplayDiversion This commit adds support for the vReplayDiversion extension to the GDB remote protocol. This protocol extension moves control over diversion sessions to GDB, allowing it to explicitly model diversion state when considering things like UI heuristics or scheduling decisions, expose such state to end users and scripts, or make various aspects user-configurable. The benefit to rr in the future will be a smoothing out of several UI wrinkles and (perhaps eventually) the removal of some of the hackier, more ad-hoc parts of the rr/GDB interface. Some of these benefits will be had in the immediate-term; however, this will also come with some regressions until GDB all of the logic needed for feature-parity. (An instance of this is apparent in the number of test cases no longer passing due to this commit; GDB having control over diversion entrance/exit means rr commands can no longer trigger a diversion exit themselves. A future commit will have these commands present an error to the user instead of corrupting state, but this may well still present a regression to some users.) --- src/GdbConnection.cc | 23 +++++++++++++++++++- src/GdbConnection.h | 30 ++++++++++++++++++++++++- src/GdbServer.cc | 52 ++++++++++++++++++++++++++++++++++++-------- 3 files changed, 94 insertions(+), 11 deletions(-) diff --git a/src/GdbConnection.cc b/src/GdbConnection.cc index dbc99ccb493..422f75eb953 100644 --- a/src/GdbConnection.cc +++ b/src/GdbConnection.cc @@ -656,6 +656,7 @@ bool GdbConnection::query(char* payload) { multiprocess_supported_ = strstr(args, "multiprocess+") != nullptr; hwbreak_supported_ = strstr(args, "hwbreak+") != nullptr; swbreak_supported_ = strstr(args, "swbreak+") != nullptr; + replay_diversion_supported_ = strstr(args, "vReplayDiversion+") != nullptr; stringstream supported; // Encourage gdb to use very large packets since we support any packet size @@ -670,7 +671,8 @@ bool GdbConnection::query(char* payload) { ";hwbreak+" ";swbreak+" ";ConditionalBreakpoints+" - ";vContSupported+"; + ";vContSupported+" + ";vReplayDiversion+"; if (features().reverse_execution) { supported << ";ReverseContinue+" ";ReverseStep+"; @@ -1040,6 +1042,12 @@ bool GdbConnection::process_vpacket(char* payload) { } } + if (name == strstr(name, "ReplayDiversion:")) { + req = GdbRequest(DREQ_REPLAY_DIVERSION); + req.replay_diversion().want_diversion = name[16] == '1'; + return true; + } + UNHANDLED_REQ() << "Unhandled gdb vpacket: v" << name; return false; } @@ -1836,6 +1844,19 @@ void GdbConnection::reply_close(int err) { consume_request(); } +void GdbConnection::reply_replay_diversion(const char* error_message) { + DEBUG_ASSERT(DREQ_REPLAY_DIVERSION == req.type); + if (error_message != nullptr) { + stringstream ss; + ss << "E." << error_message; + write_packet(ss.str().c_str()); + } else { + write_packet("OK"); + } + + consume_request(); +} + void GdbConnection::send_file_error_reply(int system_errno) { int gdb_err; switch (system_errno) { diff --git a/src/GdbConnection.h b/src/GdbConnection.h index e3b56898d52..9789e85ab4d 100644 --- a/src/GdbConnection.h +++ b/src/GdbConnection.h @@ -142,6 +142,9 @@ enum GdbRequestType { DREQ_FILE_PREAD, // vFile:close packet, uses params.file_close. DREQ_FILE_CLOSE, + + // vReplayDiversion packet, uses .replay_diversion. + DREQ_REPLAY_DIVERSION, }; enum GdbRestartType { @@ -185,7 +188,8 @@ struct GdbRequest { file_setfs_(other.file_setfs_), file_open_(other.file_open_), file_pread_(other.file_pread_), - file_close_(other.file_close_) {} + file_close_(other.file_close_), + replay_diversion_(other.replay_diversion_) {} GdbRequest& operator=(const GdbRequest& other) { this->~GdbRequest(); new (this) GdbRequest(other); @@ -245,6 +249,9 @@ struct GdbRequest { struct FileClose { int fd; } file_close_; + struct ReplayDiversion { + bool want_diversion; + } replay_diversion_; Mem& mem() { DEBUG_ASSERT(type >= DREQ_MEM_FIRST && type <= DREQ_MEM_LAST); @@ -338,6 +345,14 @@ struct GdbRequest { DEBUG_ASSERT(type == DREQ_FILE_CLOSE); return file_close_; } + ReplayDiversion& replay_diversion() { + DEBUG_ASSERT(type == DREQ_REPLAY_DIVERSION); + return replay_diversion_; + } + const ReplayDiversion& replay_diversion() const { + DEBUG_ASSERT(type == DREQ_REPLAY_DIVERSION); + return replay_diversion_; + } /** * Return nonzero if this requires that program execution be resumed @@ -553,6 +568,13 @@ class GdbConnection { */ void reply_close(int err); + /** + * Respond to a vReplayDiversion packet. If |error_message| is + * NULL, respond with 'OK'; otherwise, use |error_message| as the + * reason for a verbose error response. + */ + void reply_replay_diversion(const char* error_message); + /** * Create a checkpoint of the given Session with the given id. Delete the * existing checkpoint with that id if there is one. @@ -603,6 +625,11 @@ class GdbConnection { bool hwbreak_supported() { return hwbreak_supported_; } bool swbreak_supported() { return swbreak_supported_; } + /** + * Returns true if the remote supports vReplayDiversion packets. + */ + bool replay_diversion_supported() { return replay_diversion_supported_; } + private: /** * read() incoming data exactly one time, successfully. May block. @@ -696,6 +723,7 @@ class GdbConnection { bool multiprocess_supported_; // client supports multiprocess extension bool hwbreak_supported_; // client supports hwbreak extension bool swbreak_supported_; // client supports swbreak extension + bool replay_diversion_supported_; // client supports vReplayDiversion extension }; } // namespace rr diff --git a/src/GdbServer.cc b/src/GdbServer.cc index 83cfbb005f4..c424296d7ee 100644 --- a/src/GdbServer.cc +++ b/src/GdbServer.cc @@ -839,8 +839,10 @@ bool GdbServer::diverter_process_debugger_requests( return false; case DREQ_READ_SIGINFO: { - LOG(debug) << "Adding ref to diversion session"; - ++diversion_refcount; + if (!dbg->replay_diversion_supported()) { + LOG(debug) << "Adding ref to diversion session"; + ++diversion_refcount; + } // TODO: maybe share with replayer.cc? vector si_bytes; si_bytes.resize(req->mem().len); @@ -860,15 +862,29 @@ bool GdbServer::diverter_process_debugger_requests( } case DREQ_WRITE_SIGINFO: - LOG(debug) << "Removing reference to diversion session ..."; - DEBUG_ASSERT(diversion_refcount > 0); - --diversion_refcount; - if (diversion_refcount == 0) { - LOG(debug) << " ... dying at next continue request"; + if (!dbg->replay_diversion_supported()) { + LOG(debug) << "Removing reference to diversion session ..."; + DEBUG_ASSERT(diversion_refcount > 0); + --diversion_refcount; + if (diversion_refcount == 0) { + LOG(debug) << " ... dying at next continue request"; + } } dbg->reply_write_siginfo(); continue; + case DREQ_REPLAY_DIVERSION: + if (req->replay_diversion().want_diversion) { + dbg->reply_replay_diversion("already-in-diversion"); + continue; + } + LOG(debug) << "GDB asked for replay diversion to end"; + DEBUG_ASSERT(diversion_refcount > 0); + diversion_refcount = 0; + dbg->reply_replay_diversion(nullptr); + *req = GdbRequest(DREQ_NONE); + return false; + case DREQ_RR_CMD: { DEBUG_ASSERT(req->type == DREQ_RR_CMD); Task* task = diversion_session.find_task(last_continue_tuid); @@ -1145,14 +1161,32 @@ GdbRequest GdbServer::process_debugger_requests(ReportState state) { // siginfo and then incorrectly start a diversion and go haywire :-(. // Ideally we'd come up with a better way to detect diversions so that // "print $_siginfo" works. - req = divert(timeline.current_session()); - if (req.type == DREQ_NONE) { + if (!dbg->replay_diversion_supported()) { + req = divert(timeline.current_session()); + if (req.type == DREQ_NONE) { + continue; + } + } else { continue; } // Carry on to process the request that was rejected by // the diversion session } + if (req.type == DREQ_REPLAY_DIVERSION) { + if (req.replay_diversion().want_diversion) { + LOG(debug) << "GDB requested diversion"; + dbg->reply_replay_diversion(nullptr); + req = divert(timeline.current_session()); + if (req.type == DREQ_NONE) { + continue; + } + } else { + dbg->reply_replay_diversion("not-in-diversion"); + continue; + } + } + if (req.is_resume_request()) { Task* t = current_session().find_task(last_continue_tuid); if (t) { From 41b04c45a23062cc095ef3f1028b92a434e79ea1 Mon Sep 17 00:00:00 2001 From: Bob131 Date: Sun, 30 May 2021 04:36:45 +1000 Subject: [PATCH 3/3] GdbCommandHandler: don't exit explicit diversions If the client GDB supports the vReplayDiversion protocol extension then diversion sessions are controlled explicitly by the client; allowing rr commands to end a diversion session under GDB's nose will corrupt the implicit shared state. This commit modifies the command handler to instead present an error message to the user if a command wanted to end a GDB-requested diversion. --- src/GdbCommandHandler.cc | 4 ++++ src/GdbServer.h | 4 ++++ src/test/when.py | 15 ++++++--------- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/GdbCommandHandler.cc b/src/GdbCommandHandler.cc index c8d124b944b..bcc7fb8f820 100644 --- a/src/GdbCommandHandler.cc +++ b/src/GdbCommandHandler.cc @@ -224,6 +224,10 @@ static vector parse_cmd(string& str) { if (resp == GdbCommandHandler::cmd_end_diversion()) { LOG(debug) << "cmd must run outside of diversion (" << resp << ")"; + if (gdb_server.remote_supports_replay_diversion()) { + return gdb_escape(string() + "Command '" + cmd->name() + + "' cannot run in a replay diversion.\n"); + } return resp; } diff --git a/src/GdbServer.h b/src/GdbServer.h index 2e8af5cbeb9..c61d74db297 100644 --- a/src/GdbServer.h +++ b/src/GdbServer.h @@ -120,6 +120,10 @@ class GdbServer { ReplayTimeline& get_timeline() { return timeline; } + bool remote_supports_replay_diversion() { + return dbg != nullptr && dbg->replay_diversion_supported(); + } + private: GdbServer(std::unique_ptr& dbg, Task* t); diff --git a/src/test/when.py b/src/test/when.py index 304713be447..fcf8b15e57b 100644 --- a/src/test/when.py +++ b/src/test/when.py @@ -46,23 +46,20 @@ # Ensure 'when' terminates a diversion send_gdb('call strlen("abcd")') send_gdb('when') -expect_gdb(re.compile(r'Current event: (\d+)')) -t3 = eval(last_match().group(1)); -if t3 != t2: +expect_gdb(re.compile(r'(Current event: (\d+)|cannot run in a replay diversion)')) +if last_match().group(2) and int(last_match().group(2)) != t2: failed('ERROR ... diversion changed event') send_gdb('call strlen("abcd")') send_gdb('when-ticks') -expect_gdb(re.compile(r'Current tick: (\d+)')) -ticks3 = eval(last_match().group(1)); -if ticks3 != ticks2: +expect_gdb(re.compile(r'(Current tick: (\d+)|cannot run in a replay diversion)')) +if last_match().group(2) and int(last_match().group(2)) != ticks2: failed('ERROR ... diversion changed ticks') send_gdb('call strlen("abcd")') send_gdb('when-tid') -expect_gdb(re.compile(r'Current tid: (\d+)')) -tid3 = eval(last_match().group(1)); -if tid3 != tid2: +expect_gdb(re.compile(r'(Current tid: (\d+)|cannot run in a replay diversion)')) +if last_match().group(2) and int(last_match().group(2)) != tid2: failed('ERROR ... diversion changed tid') ok()