* [PATCH v3 2/3] gdbstub: only send stop-reply packets when allowed to
2022-09-20 12:47 [PATCH v3 0/3] gdbstub: avoid untimely stop-reply msgs Matheus Tavares Bernardino
2022-09-20 12:47 ` [PATCH v3 1/3] configure: make sure tcg tests can see HAVE_GDB_BIN Matheus Tavares Bernardino
@ 2022-09-20 12:47 ` Matheus Tavares Bernardino
2022-09-20 12:47 ` [PATCH v3 3/3] gdbstub: add test for untimely stop-reply packets Matheus Tavares Bernardino
2 siblings, 0 replies; 4+ messages in thread
From: Matheus Tavares Bernardino @ 2022-09-20 12:47 UTC (permalink / raw)
To: qemu-devel; +Cc: bcain, alex.bennee, f4bug, peter.maydell
GDB's remote serial protocol allows stop-reply messages to be sent by
the stub either as a notification packet or as a reply to a GDB command
(provided that the cmd accepts such a response). QEMU currently does not
implement notification packets, so it should only send stop-replies
synchronously and when requested. Nevertheless, it may still issue
unsolicited stop messages through gdb_vm_state_change().
Although this behavior doesn't seem to cause problems with GDB itself,
it does with other debuggers that implement the GDB remote serial
protocol, like hexagon-lldb. In this case, the debugger fails upon an
unexpected stop-reply message from QEMU when lldb attaches to it.
Instead, let's change the gdbstub to send stop messages only as a
response to a previous GDB command that accepts such a reply.
Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
---
gdbstub.c | 64 ++++++++++++++++++++++++++++++++++++++++---------------
1 file changed, 47 insertions(+), 17 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index cf869b10e3..14b4348c18 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -369,6 +369,7 @@ typedef struct GDBState {
GByteArray *mem_buf;
int sstep_flags;
int supported_sstep_flags;
+ bool allow_stop_reply;
} GDBState;
static GDBState gdbserver_state;
@@ -412,6 +413,7 @@ static void reset_gdbserver_state(void)
g_free(gdbserver_state.processes);
gdbserver_state.processes = NULL;
gdbserver_state.process_num = 0;
+ gdbserver_state.allow_stop_reply = 0;
}
#endif
@@ -1484,6 +1486,7 @@ typedef struct GdbCmdParseEntry {
const char *cmd;
bool cmd_startswith;
const char *schema;
+ bool allow_stop_reply;
} GdbCmdParseEntry;
static inline int startswith(const char *string, const char *pattern)
@@ -1517,6 +1520,7 @@ static int process_string_cmd(void *user_ctx, const char *data,
}
}
+ gdbserver_state.allow_stop_reply = cmd->allow_stop_reply;
cmd->handler(params, user_ctx);
return 0;
}
@@ -2013,11 +2017,14 @@ static void handle_v_attach(GArray *params, void *user_ctx)
gdbserver_state.g_cpu = cpu;
gdbserver_state.c_cpu = cpu;
- g_string_printf(gdbserver_state.str_buf, "T%02xthread:", GDB_SIGNAL_TRAP);
- gdb_append_thread_id(cpu, gdbserver_state.str_buf);
- g_string_append_c(gdbserver_state.str_buf, ';');
+ if (gdbserver_state.allow_stop_reply) {
+ g_string_printf(gdbserver_state.str_buf, "T%02xthread:", GDB_SIGNAL_TRAP);
+ gdb_append_thread_id(cpu, gdbserver_state.str_buf);
+ g_string_append_c(gdbserver_state.str_buf, ';');
+ gdbserver_state.allow_stop_reply = 0;
cleanup:
- put_strbuf();
+ put_strbuf();
+ }
}
static void handle_v_kill(GArray *params, void *user_ctx)
@@ -2040,12 +2047,14 @@ static const GdbCmdParseEntry gdb_v_commands_table[] = {
.handler = handle_v_cont,
.cmd = "Cont",
.cmd_startswith = 1,
+ .allow_stop_reply = 1,
.schema = "s0"
},
{
.handler = handle_v_attach,
.cmd = "Attach;",
.cmd_startswith = 1,
+ .allow_stop_reply = 1,
.schema = "l0"
},
{
@@ -2546,10 +2555,13 @@ static void handle_gen_set(GArray *params, void *user_ctx)
static void handle_target_halt(GArray *params, void *user_ctx)
{
- g_string_printf(gdbserver_state.str_buf, "T%02xthread:", GDB_SIGNAL_TRAP);
- gdb_append_thread_id(gdbserver_state.c_cpu, gdbserver_state.str_buf);
- g_string_append_c(gdbserver_state.str_buf, ';');
- put_strbuf();
+ if (gdbserver_state.allow_stop_reply) {
+ g_string_printf(gdbserver_state.str_buf, "T%02xthread:", GDB_SIGNAL_TRAP);
+ gdb_append_thread_id(gdbserver_state.c_cpu, gdbserver_state.str_buf);
+ g_string_append_c(gdbserver_state.str_buf, ';');
+ put_strbuf();
+ gdbserver_state.allow_stop_reply = 0;
+ }
/*
* Remove all the breakpoints when this query is issued,
* because gdb is doing an initial connect and the state
@@ -2573,7 +2585,8 @@ static int gdb_handle_packet(const char *line_buf)
static const GdbCmdParseEntry target_halted_cmd_desc = {
.handler = handle_target_halt,
.cmd = "?",
- .cmd_startswith = 1
+ .cmd_startswith = 1,
+ .allow_stop_reply = 1,
};
cmd_parser = &target_halted_cmd_desc;
}
@@ -2584,6 +2597,7 @@ static int gdb_handle_packet(const char *line_buf)
.handler = handle_continue,
.cmd = "c",
.cmd_startswith = 1,
+ .allow_stop_reply = 1,
.schema = "L0"
};
cmd_parser = &continue_cmd_desc;
@@ -2595,6 +2609,7 @@ static int gdb_handle_packet(const char *line_buf)
.handler = handle_cont_with_sig,
.cmd = "C",
.cmd_startswith = 1,
+ .allow_stop_reply = 1,
.schema = "l0"
};
cmd_parser = &cont_with_sig_cmd_desc;
@@ -2633,6 +2648,7 @@ static int gdb_handle_packet(const char *line_buf)
.handler = handle_step,
.cmd = "s",
.cmd_startswith = 1,
+ .allow_stop_reply = 1,
.schema = "L0"
};
cmd_parser = &step_cmd_desc;
@@ -2843,6 +2859,10 @@ static void gdb_vm_state_change(void *opaque, bool running, RunState state)
return;
}
+ if (!gdbserver_state.allow_stop_reply) {
+ return;
+ }
+
gdb_append_thread_id(cpu, tid);
switch (state) {
@@ -2908,6 +2928,7 @@ static void gdb_vm_state_change(void *opaque, bool running, RunState state)
send_packet:
put_packet(buf->str);
+ gdbserver_state.allow_stop_reply = 0;
/* disable single step if it was enabled */
cpu_single_step(cpu, 0);
@@ -3000,6 +3021,7 @@ static void gdb_read_byte(uint8_t ch)
{
uint8_t reply;
+ gdbserver_state.allow_stop_reply = 0;
#ifndef CONFIG_USER_ONLY
if (gdbserver_state.last_packet->len) {
/* Waiting for a response to the last packet. If we see the start
@@ -3162,8 +3184,11 @@ void gdb_exit(int code)
trace_gdbstub_op_exiting((uint8_t)code);
- snprintf(buf, sizeof(buf), "W%02x", (uint8_t)code);
- put_packet(buf);
+ if (gdbserver_state.allow_stop_reply) {
+ snprintf(buf, sizeof(buf), "W%02x", (uint8_t)code);
+ put_packet(buf);
+ gdbserver_state.allow_stop_reply = 0;
+ }
#ifndef CONFIG_USER_ONLY
qemu_chr_fe_deinit(&gdbserver_state.chr, true);
@@ -3212,11 +3237,14 @@ gdb_handlesig(CPUState *cpu, int sig)
if (sig != 0) {
gdb_set_stop_cpu(cpu);
- g_string_printf(gdbserver_state.str_buf,
- "T%02xthread:", target_signal_to_gdb(sig));
- gdb_append_thread_id(cpu, gdbserver_state.str_buf);
- g_string_append_c(gdbserver_state.str_buf, ';');
- put_strbuf();
+ if (gdbserver_state.allow_stop_reply) {
+ g_string_printf(gdbserver_state.str_buf,
+ "T%02xthread:", target_signal_to_gdb(sig));
+ gdb_append_thread_id(cpu, gdbserver_state.str_buf);
+ g_string_append_c(gdbserver_state.str_buf, ';');
+ put_strbuf();
+ gdbserver_state.allow_stop_reply = 0;
+ }
}
/* put_packet() might have detected that the peer terminated the
connection. */
@@ -3255,12 +3283,14 @@ void gdb_signalled(CPUArchState *env, int sig)
{
char buf[4];
- if (!gdbserver_state.init || gdbserver_state.fd < 0) {
+ if (!gdbserver_state.init || gdbserver_state.fd < 0 ||
+ !gdbserver_state.allow_stop_reply) {
return;
}
snprintf(buf, sizeof(buf), "X%02x", target_signal_to_gdb(sig));
put_packet(buf);
+ gdbserver_state.allow_stop_reply = 0;
}
static void gdb_accept_init(int fd)
--
2.37.2
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH v3 3/3] gdbstub: add test for untimely stop-reply packets
2022-09-20 12:47 [PATCH v3 0/3] gdbstub: avoid untimely stop-reply msgs Matheus Tavares Bernardino
2022-09-20 12:47 ` [PATCH v3 1/3] configure: make sure tcg tests can see HAVE_GDB_BIN Matheus Tavares Bernardino
2022-09-20 12:47 ` [PATCH v3 2/3] gdbstub: only send stop-reply packets when allowed to Matheus Tavares Bernardino
@ 2022-09-20 12:47 ` Matheus Tavares Bernardino
2 siblings, 0 replies; 4+ messages in thread
From: Matheus Tavares Bernardino @ 2022-09-20 12:47 UTC (permalink / raw)
To: qemu-devel; +Cc: bcain, alex.bennee, f4bug, peter.maydell
In the previous commit, we modified gdbstub.c to only send stop-reply
packets as a response to GDB commands that accept it. Now, let's add a
test for this intended behavior. Running this test before the fix from
the previous commit fails as QEMU sends a stop-reply packet
asynchronously, when GDB was in fact waiting an ACK.
Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
---
tests/guest-debug/run-test.py | 16 ++++++++++++----
.../tcg/multiarch/system/Makefile.softmmu-target | 16 +++++++++++++++-
2 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/tests/guest-debug/run-test.py b/tests/guest-debug/run-test.py
index d865e46ecd..de6106a5e5 100755
--- a/tests/guest-debug/run-test.py
+++ b/tests/guest-debug/run-test.py
@@ -26,11 +26,12 @@ def get_args():
parser.add_argument("--qargs", help="Qemu arguments for test")
parser.add_argument("--binary", help="Binary to debug",
required=True)
- parser.add_argument("--test", help="GDB test script",
- required=True)
+ parser.add_argument("--test", help="GDB test script")
parser.add_argument("--gdb", help="The gdb binary to use",
default=None)
+ parser.add_argument("--gdb-args", help="Additional gdb arguments")
parser.add_argument("--output", help="A file to redirect output to")
+ parser.add_argument("--stderr", help="A file to redirect stderr to")
return parser.parse_args()
@@ -58,6 +59,10 @@ def log(output, msg):
output = open(args.output, "w")
else:
output = None
+ if args.stderr:
+ stderr = open(args.stderr, "w")
+ else:
+ stderr = None
socket_dir = TemporaryDirectory("qemu-gdbstub")
socket_name = os.path.join(socket_dir.name, "gdbstub.socket")
@@ -77,6 +82,8 @@ def log(output, msg):
# Now launch gdb with our test and collect the result
gdb_cmd = "%s %s" % (args.gdb, args.binary)
+ if args.gdb_args:
+ gdb_cmd += " %s" % (args.gdb_args)
# run quietly and ignore .gdbinit
gdb_cmd += " -q -n -batch"
# disable prompts in case of crash
@@ -84,13 +91,14 @@ def log(output, msg):
# connect to remote
gdb_cmd += " -ex 'target remote %s'" % (socket_name)
# finally the test script itself
- gdb_cmd += " -x %s" % (args.test)
+ if args.test:
+ gdb_cmd += " -x %s" % (args.test)
sleep(1)
log(output, "GDB CMD: %s" % (gdb_cmd))
- result = subprocess.call(gdb_cmd, shell=True, stdout=output)
+ result = subprocess.call(gdb_cmd, shell=True, stdout=output, stderr=stderr)
# A result of greater than 128 indicates a fatal signal (likely a
# crash due to gdb internal failure). That's a problem for GDB and
diff --git a/tests/tcg/multiarch/system/Makefile.softmmu-target b/tests/tcg/multiarch/system/Makefile.softmmu-target
index 625ed792c6..1bb6d89097 100644
--- a/tests/tcg/multiarch/system/Makefile.softmmu-target
+++ b/tests/tcg/multiarch/system/Makefile.softmmu-target
@@ -27,9 +27,23 @@ run-gdbstub-memory: memory
--bin $< --test $(MULTIARCH_SRC)/gdbstub/memory.py, \
"softmmu gdbstub support")
+run-gdbstub-untimely-packet: hello
+ $(call run-test, $@, $(GDB_SCRIPT) \
+ --gdb $(HAVE_GDB_BIN) \
+ --gdb-args "-ex 'set debug remote 1'" \
+ --output untimely-packet.gdb.out \
+ --stderr untimely-packet.gdb.err \
+ --qemu $(QEMU) \
+ --bin $< --qargs \
+ "-monitor none -display none -chardev file$(COMMA)path=untimely-packet.out$(COMMA)id=output $(QEMU_OPTS)", \
+ "softmmu gdbstub untimely packets")
+ $(call quiet-command, \
+ (! grep -Fq 'Packet instead of Ack, ignoring it' untimely-packet.gdb.err), \
+ "GREP", "file untimely-packet.gdb.err")
+
else
run-gdbstub-%:
$(call skip-test, "gdbstub test $*", "need working gdb")
endif
-MULTIARCH_RUNS += run-gdbstub-memory
+MULTIARCH_RUNS += run-gdbstub-memory run-gdbstub-untimely-packet
--
2.37.2
^ permalink raw reply related [flat|nested] 4+ messages in thread