* [PATCH] gdbstub: Switch to the thread receiving a signal @ 2021-09-30 9:51 Pavel Labath 2021-10-12 17:10 ` Pavel Labath ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Pavel Labath @ 2021-09-30 9:51 UTC (permalink / raw) To: qemu-devel; +Cc: Philippe Mathieu-Daudé, Pavel Labath, Alex Bennée Respond with Txxthread:yyyy; instead of a plain Sxx to indicate which thread received the signal. Otherwise, the debugger will associate it with the main one. Also automatically select this thread, as that is what gdb expects. Signed-off-by: Pavel Labath <pavel@labath.sk> --- gdbstub.c | 9 ++- tests/tcg/multiarch/Makefile.target | 7 +++ .../gdbstub/test-thread-breakpoint.py | 60 +++++++++++++++++++ 3 files changed, 74 insertions(+), 2 deletions(-) create mode 100644 tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py diff --git a/gdbstub.c b/gdbstub.c index 36b85aa..7bd4479 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -3138,8 +3138,13 @@ gdb_handlesig(CPUState *cpu, int sig) tb_flush(cpu); if (sig != 0) { - snprintf(buf, sizeof(buf), "S%02x", target_signal_to_gdb(sig)); - put_packet(buf); + gdbserver_state.c_cpu = cpu; + gdbserver_state.g_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(); } /* put_packet() might have detected that the peer terminated the connection. */ diff --git a/tests/tcg/multiarch/Makefile.target b/tests/tcg/multiarch/Makefile.target index 85a6fb7..c7b7e8b 100644 --- a/tests/tcg/multiarch/Makefile.target +++ b/tests/tcg/multiarch/Makefile.target @@ -73,6 +73,13 @@ run-gdbstub-qxfer-auxv-read: sha1 --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-qxfer-auxv-read.py, \ "basic gdbstub qXfer:auxv:read support") +run-gdbstub-thread-breakpoint: testthread + $(call run-test, $@, $(GDB_SCRIPT) \ + --gdb $(HAVE_GDB_BIN) \ + --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \ + --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-thread-breakpoint.py, \ + "hitting a breakpoint on non-main thread") + else run-gdbstub-%: $(call skip-test, "gdbstub test $*", "need working gdb") diff --git a/tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py b/tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py new file mode 100644 index 0000000..798d508 --- /dev/null +++ b/tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py @@ -0,0 +1,60 @@ +from __future__ import print_function +# +# Test auxiliary vector is loaded via gdbstub +# +# This is launched via tests/guest-debug/run-test.py +# + +import gdb +import sys + +failcount = 0 + +def report(cond, msg): + "Report success/fail of test" + if cond: + print ("PASS: %s" % (msg)) + else: + print ("FAIL: %s" % (msg)) + global failcount + failcount += 1 + +def run_test(): + "Run through the tests one by one" + + sym, ok = gdb.lookup_symbol("thread1_func") + gdb.execute("b thread1_func") + gdb.execute("c") + + frame = gdb.selected_frame() + report(str(frame.function()) == "thread1_func", "break @ %s"%frame) + +# +# This runs as the script it sourced (via -x, via run-test.py) +# +try: + inferior = gdb.selected_inferior() + arch = inferior.architecture() + print("ATTACHED: %s" % arch.name()) +except (gdb.error, AttributeError): + print("SKIPPING (not connected)", file=sys.stderr) + exit(0) + +if gdb.parse_and_eval('$pc') == 0: + print("SKIP: PC not set") + exit(0) + +try: + # These are not very useful in scripts + gdb.execute("set pagination off") + gdb.execute("set confirm off") + + # Run the actual tests + run_test() +except (gdb.error): + print ("GDB Exception: %s" % (sys.exc_info()[0])) + failcount += 1 + pass + +print("All tests complete: %d failures" % failcount) +exit(failcount) -- 2.32.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] gdbstub: Switch to the thread receiving a signal 2021-09-30 9:51 [PATCH] gdbstub: Switch to the thread receiving a signal Pavel Labath @ 2021-10-12 17:10 ` Pavel Labath 2021-10-15 15:59 ` Alex Bennée 2021-10-19 17:02 ` Alex Bennée 2021-10-19 17:49 ` [PATCH v2] " Pavel Labath 2 siblings, 1 reply; 12+ messages in thread From: Pavel Labath @ 2021-10-12 17:10 UTC (permalink / raw) To: qemu-devel, Alex Bennée, Philippe Mathieu-Daudé Ping. (This is my first qemu patch, so please let me know if I am doing something wrong.) regards, pavel On 30/09/2021 11:51, Pavel Labath wrote: > Respond with Txxthread:yyyy; instead of a plain Sxx to indicate which > thread received the signal. Otherwise, the debugger will associate it > with the main one. Also automatically select this thread, as that is > what gdb expects. > > Signed-off-by: Pavel Labath <pavel@labath.sk> > --- > gdbstub.c | 9 ++- > tests/tcg/multiarch/Makefile.target | 7 +++ > .../gdbstub/test-thread-breakpoint.py | 60 +++++++++++++++++++ > 3 files changed, 74 insertions(+), 2 deletions(-) > create mode 100644 tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py > > diff --git a/gdbstub.c b/gdbstub.c > index 36b85aa..7bd4479 100644 > --- a/gdbstub.c > +++ b/gdbstub.c > @@ -3138,8 +3138,13 @@ gdb_handlesig(CPUState *cpu, int sig) > tb_flush(cpu); > > if (sig != 0) { > - snprintf(buf, sizeof(buf), "S%02x", target_signal_to_gdb(sig)); > - put_packet(buf); > + gdbserver_state.c_cpu = cpu; > + gdbserver_state.g_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(); > } > /* put_packet() might have detected that the peer terminated the > connection. */ > diff --git a/tests/tcg/multiarch/Makefile.target b/tests/tcg/multiarch/Makefile.target > index 85a6fb7..c7b7e8b 100644 > --- a/tests/tcg/multiarch/Makefile.target > +++ b/tests/tcg/multiarch/Makefile.target > @@ -73,6 +73,13 @@ run-gdbstub-qxfer-auxv-read: sha1 > --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-qxfer-auxv-read.py, \ > "basic gdbstub qXfer:auxv:read support") > > +run-gdbstub-thread-breakpoint: testthread > + $(call run-test, $@, $(GDB_SCRIPT) \ > + --gdb $(HAVE_GDB_BIN) \ > + --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \ > + --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-thread-breakpoint.py, \ > + "hitting a breakpoint on non-main thread") > + > else > run-gdbstub-%: > $(call skip-test, "gdbstub test $*", "need working gdb") > diff --git a/tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py b/tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py > new file mode 100644 > index 0000000..798d508 > --- /dev/null > +++ b/tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py > @@ -0,0 +1,60 @@ > +from __future__ import print_function > +# > +# Test auxiliary vector is loaded via gdbstub > +# > +# This is launched via tests/guest-debug/run-test.py > +# > + > +import gdb > +import sys > + > +failcount = 0 > + > +def report(cond, msg): > + "Report success/fail of test" > + if cond: > + print ("PASS: %s" % (msg)) > + else: > + print ("FAIL: %s" % (msg)) > + global failcount > + failcount += 1 > + > +def run_test(): > + "Run through the tests one by one" > + > + sym, ok = gdb.lookup_symbol("thread1_func") > + gdb.execute("b thread1_func") > + gdb.execute("c") > + > + frame = gdb.selected_frame() > + report(str(frame.function()) == "thread1_func", "break @ %s"%frame) > + > +# > +# This runs as the script it sourced (via -x, via run-test.py) > +# > +try: > + inferior = gdb.selected_inferior() > + arch = inferior.architecture() > + print("ATTACHED: %s" % arch.name()) > +except (gdb.error, AttributeError): > + print("SKIPPING (not connected)", file=sys.stderr) > + exit(0) > + > +if gdb.parse_and_eval('$pc') == 0: > + print("SKIP: PC not set") > + exit(0) > + > +try: > + # These are not very useful in scripts > + gdb.execute("set pagination off") > + gdb.execute("set confirm off") > + > + # Run the actual tests > + run_test() > +except (gdb.error): > + print ("GDB Exception: %s" % (sys.exc_info()[0])) > + failcount += 1 > + pass > + > +print("All tests complete: %d failures" % failcount) > +exit(failcount) > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] gdbstub: Switch to the thread receiving a signal 2021-10-12 17:10 ` Pavel Labath @ 2021-10-15 15:59 ` Alex Bennée 2021-10-19 13:19 ` Pavel Labath 0 siblings, 1 reply; 12+ messages in thread From: Alex Bennée @ 2021-10-15 15:59 UTC (permalink / raw) To: Pavel Labath; +Cc: Philippe Mathieu-Daudé, qemu-devel Pavel Labath <pavel@labath.sk> writes: > Ping. > > (This is my first qemu patch, so please let me know if I am doing > something wrong.) Apologies it slipped though the cracks. I shall have a look on Monday. > > regards, > pavel > > On 30/09/2021 11:51, Pavel Labath wrote: >> Respond with Txxthread:yyyy; instead of a plain Sxx to indicate which >> thread received the signal. Otherwise, the debugger will associate it >> with the main one. Also automatically select this thread, as that is >> what gdb expects. >> Signed-off-by: Pavel Labath <pavel@labath.sk> >> --- >> gdbstub.c | 9 ++- >> tests/tcg/multiarch/Makefile.target | 7 +++ >> .../gdbstub/test-thread-breakpoint.py | 60 +++++++++++++++++++ >> 3 files changed, 74 insertions(+), 2 deletions(-) >> create mode 100644 tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py >> diff --git a/gdbstub.c b/gdbstub.c >> index 36b85aa..7bd4479 100644 >> --- a/gdbstub.c >> +++ b/gdbstub.c >> @@ -3138,8 +3138,13 @@ gdb_handlesig(CPUState *cpu, int sig) >> tb_flush(cpu); >> if (sig != 0) { >> - snprintf(buf, sizeof(buf), "S%02x", target_signal_to_gdb(sig)); >> - put_packet(buf); >> + gdbserver_state.c_cpu = cpu; >> + gdbserver_state.g_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(); >> } >> /* put_packet() might have detected that the peer terminated the >> connection. */ >> diff --git a/tests/tcg/multiarch/Makefile.target b/tests/tcg/multiarch/Makefile.target >> index 85a6fb7..c7b7e8b 100644 >> --- a/tests/tcg/multiarch/Makefile.target >> +++ b/tests/tcg/multiarch/Makefile.target >> @@ -73,6 +73,13 @@ run-gdbstub-qxfer-auxv-read: sha1 >> --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-qxfer-auxv-read.py, \ >> "basic gdbstub qXfer:auxv:read support") >> +run-gdbstub-thread-breakpoint: testthread >> + $(call run-test, $@, $(GDB_SCRIPT) \ >> + --gdb $(HAVE_GDB_BIN) \ >> + --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \ >> + --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-thread-breakpoint.py, \ >> + "hitting a breakpoint on non-main thread") >> + >> else >> run-gdbstub-%: >> $(call skip-test, "gdbstub test $*", "need working gdb") >> diff --git a/tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py b/tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py >> new file mode 100644 >> index 0000000..798d508 >> --- /dev/null >> +++ b/tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py >> @@ -0,0 +1,60 @@ >> +from __future__ import print_function >> +# >> +# Test auxiliary vector is loaded via gdbstub >> +# >> +# This is launched via tests/guest-debug/run-test.py >> +# >> + >> +import gdb >> +import sys >> + >> +failcount = 0 >> + >> +def report(cond, msg): >> + "Report success/fail of test" >> + if cond: >> + print ("PASS: %s" % (msg)) >> + else: >> + print ("FAIL: %s" % (msg)) >> + global failcount >> + failcount += 1 >> + >> +def run_test(): >> + "Run through the tests one by one" >> + >> + sym, ok = gdb.lookup_symbol("thread1_func") >> + gdb.execute("b thread1_func") >> + gdb.execute("c") >> + >> + frame = gdb.selected_frame() >> + report(str(frame.function()) == "thread1_func", "break @ %s"%frame) >> + >> +# >> +# This runs as the script it sourced (via -x, via run-test.py) >> +# >> +try: >> + inferior = gdb.selected_inferior() >> + arch = inferior.architecture() >> + print("ATTACHED: %s" % arch.name()) >> +except (gdb.error, AttributeError): >> + print("SKIPPING (not connected)", file=sys.stderr) >> + exit(0) >> + >> +if gdb.parse_and_eval('$pc') == 0: >> + print("SKIP: PC not set") >> + exit(0) >> + >> +try: >> + # These are not very useful in scripts >> + gdb.execute("set pagination off") >> + gdb.execute("set confirm off") >> + >> + # Run the actual tests >> + run_test() >> +except (gdb.error): >> + print ("GDB Exception: %s" % (sys.exc_info()[0])) >> + failcount += 1 >> + pass >> + >> +print("All tests complete: %d failures" % failcount) >> +exit(failcount) >> -- Alex Bennée ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] gdbstub: Switch to the thread receiving a signal 2021-10-15 15:59 ` Alex Bennée @ 2021-10-19 13:19 ` Pavel Labath 2021-10-19 17:13 ` Alex Bennée 0 siblings, 1 reply; 12+ messages in thread From: Pavel Labath @ 2021-10-19 13:19 UTC (permalink / raw) To: Alex Bennée; +Cc: Philippe Mathieu-Daudé, qemu-devel On 15/10/2021 17:59, Alex Bennée wrote: > > Pavel Labath <pavel@labath.sk> writes: > >> Ping. >> >> (This is my first qemu patch, so please let me know if I am doing >> something wrong.) > > Apologies it slipped though the cracks. I shall have a look on Monday. > I don't want to impose, but did this get buried over the weekend by any chance? regards, pl ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] gdbstub: Switch to the thread receiving a signal 2021-10-19 13:19 ` Pavel Labath @ 2021-10-19 17:13 ` Alex Bennée 0 siblings, 0 replies; 12+ messages in thread From: Alex Bennée @ 2021-10-19 17:13 UTC (permalink / raw) To: Pavel Labath; +Cc: Philippe Mathieu-Daudé, qemu-devel Pavel Labath <pavel@labath.sk> writes: > On 15/10/2021 17:59, Alex Bennée wrote: >> Pavel Labath <pavel@labath.sk> writes: >> >>> Ping. >>> >>> (This is my first qemu patch, so please let me know if I am doing >>> something wrong.) >> Apologies it slipped though the cracks. I shall have a look on >> Monday. >> > > I don't want to impose, but did this get buried over the weekend by > any chance? I had a quick look over and made some comments. I haven't been able to replicate the test manually so far but I might have broken tooling so I need to dig into that. > > regards, > pl -- Alex Bennée ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] gdbstub: Switch to the thread receiving a signal 2021-09-30 9:51 [PATCH] gdbstub: Switch to the thread receiving a signal Pavel Labath 2021-10-12 17:10 ` Pavel Labath @ 2021-10-19 17:02 ` Alex Bennée 2021-10-19 17:53 ` Pavel Labath 2021-10-19 17:49 ` [PATCH v2] " Pavel Labath 2 siblings, 1 reply; 12+ messages in thread From: Alex Bennée @ 2021-10-19 17:02 UTC (permalink / raw) To: Pavel Labath; +Cc: Philippe Mathieu-Daudé, qemu-devel Pavel Labath <pavel@labath.sk> writes: > Respond with Txxthread:yyyy; instead of a plain Sxx to indicate which > thread received the signal. Otherwise, the debugger will associate it > with the main one. Also automatically select this thread, as that is > what gdb expects. > > Signed-off-by: Pavel Labath <pavel@labath.sk> > --- > gdbstub.c | 9 ++- > tests/tcg/multiarch/Makefile.target | 7 +++ > .../gdbstub/test-thread-breakpoint.py | 60 +++++++++++++++++++ > 3 files changed, 74 insertions(+), 2 deletions(-) > create mode 100644 tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py > > diff --git a/gdbstub.c b/gdbstub.c > index 36b85aa..7bd4479 100644 > --- a/gdbstub.c > +++ b/gdbstub.c > @@ -3138,8 +3138,13 @@ gdb_handlesig(CPUState *cpu, int sig) > tb_flush(cpu); > > if (sig != 0) { > - snprintf(buf, sizeof(buf), "S%02x", target_signal_to_gdb(sig)); > - put_packet(buf); > + gdbserver_state.c_cpu = cpu; > + gdbserver_state.g_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(); > } > /* put_packet() might have detected that the peer terminated the > connection. */ > diff --git a/tests/tcg/multiarch/Makefile.target b/tests/tcg/multiarch/Makefile.target > index 85a6fb7..c7b7e8b 100644 > --- a/tests/tcg/multiarch/Makefile.target > +++ b/tests/tcg/multiarch/Makefile.target > @@ -73,6 +73,13 @@ run-gdbstub-qxfer-auxv-read: sha1 > --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-qxfer-auxv-read.py, \ > "basic gdbstub qXfer:auxv:read support") > > +run-gdbstub-thread-breakpoint: testthread > + $(call run-test, $@, $(GDB_SCRIPT) \ > + --gdb $(HAVE_GDB_BIN) \ > + --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \ > + --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-thread-breakpoint.py, \ > + "hitting a breakpoint on non-main thread") > + You also need to add the test to EXTRA_RUNS here (or just bellow in fact). > else > run-gdbstub-%: > $(call skip-test, "gdbstub test $*", "need working gdb") > diff --git a/tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py b/tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py > new file mode 100644 > index 0000000..798d508 > --- /dev/null > +++ b/tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py > @@ -0,0 +1,60 @@ > +from __future__ import print_function > +# > +# Test auxiliary vector is loaded via gdbstub > +# > +# This is launched via tests/guest-debug/run-test.py > +# > + > +import gdb > +import sys > + > +failcount = 0 > + > +def report(cond, msg): > + "Report success/fail of test" > + if cond: > + print ("PASS: %s" % (msg)) > + else: > + print ("FAIL: %s" % (msg)) > + global failcount > + failcount += 1 > + > +def run_test(): > + "Run through the tests one by one" > + > + sym, ok = gdb.lookup_symbol("thread1_func") > + gdb.execute("b thread1_func") > + gdb.execute("c") > + > + frame = gdb.selected_frame() > + report(str(frame.function()) == "thread1_func", "break @ > %s"%frame) Does this actually check the correct thread is reported? > + > +# > +# This runs as the script it sourced (via -x, via run-test.py) > +# > +try: > + inferior = gdb.selected_inferior() > + arch = inferior.architecture() > + print("ATTACHED: %s" % arch.name()) > +except (gdb.error, AttributeError): > + print("SKIPPING (not connected)", file=sys.stderr) > + exit(0) > + > +if gdb.parse_and_eval('$pc') == 0: > + print("SKIP: PC not set") > + exit(0) > + > +try: > + # These are not very useful in scripts > + gdb.execute("set pagination off") > + gdb.execute("set confirm off") > + > + # Run the actual tests > + run_test() > +except (gdb.error): > + print ("GDB Exception: %s" % (sys.exc_info()[0])) > + failcount += 1 > + pass > + > +print("All tests complete: %d failures" % failcount) > +exit(failcount) -- Alex Bennée ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] gdbstub: Switch to the thread receiving a signal 2021-10-19 17:02 ` Alex Bennée @ 2021-10-19 17:53 ` Pavel Labath 0 siblings, 0 replies; 12+ messages in thread From: Pavel Labath @ 2021-10-19 17:53 UTC (permalink / raw) To: Alex Bennée; +Cc: Philippe Mathieu-Daudé, qemu-devel Thanks for taking a look. I've sent out a new version of the patch. Besides the Makefile change, I have also replaced the direct gdbserver_state manipulation with a call to gdb_set_stop_cpu. Further responses inline. On 19/10/2021 19:02, Alex Bennée wrote: > > Pavel Labath <pavel@labath.sk> writes: > >> Respond with Txxthread:yyyy; instead of a plain Sxx to indicate which >> thread received the signal. Otherwise, the debugger will associate it >> with the main one. Also automatically select this thread, as that is >> what gdb expects. >> >> Signed-off-by: Pavel Labath <pavel@labath.sk> >> --- >> gdbstub.c | 9 ++- >> tests/tcg/multiarch/Makefile.target | 7 +++ >> .../gdbstub/test-thread-breakpoint.py | 60 +++++++++++++++++++ >> 3 files changed, 74 insertions(+), 2 deletions(-) >> create mode 100644 tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py >> >> diff --git a/gdbstub.c b/gdbstub.c >> index 36b85aa..7bd4479 100644 >> --- a/gdbstub.c >> +++ b/gdbstub.c >> @@ -3138,8 +3138,13 @@ gdb_handlesig(CPUState *cpu, int sig) >> tb_flush(cpu); >> >> if (sig != 0) { >> - snprintf(buf, sizeof(buf), "S%02x", target_signal_to_gdb(sig)); >> - put_packet(buf); >> + gdbserver_state.c_cpu = cpu; >> + gdbserver_state.g_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(); >> } >> /* put_packet() might have detected that the peer terminated the >> connection. */ >> diff --git a/tests/tcg/multiarch/Makefile.target b/tests/tcg/multiarch/Makefile.target >> index 85a6fb7..c7b7e8b 100644 >> --- a/tests/tcg/multiarch/Makefile.target >> +++ b/tests/tcg/multiarch/Makefile.target >> @@ -73,6 +73,13 @@ run-gdbstub-qxfer-auxv-read: sha1 >> --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-qxfer-auxv-read.py, \ >> "basic gdbstub qXfer:auxv:read support") >> >> +run-gdbstub-thread-breakpoint: testthread >> + $(call run-test, $@, $(GDB_SCRIPT) \ >> + --gdb $(HAVE_GDB_BIN) \ >> + --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \ >> + --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-thread-breakpoint.py, \ >> + "hitting a breakpoint on non-main thread") >> + > > You also need to add the test to EXTRA_RUNS here (or just bellow in fact). Done. > >> else >> run-gdbstub-%: >> $(call skip-test, "gdbstub test $*", "need working gdb") >> diff --git a/tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py b/tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py >> new file mode 100644 >> index 0000000..798d508 >> --- /dev/null >> +++ b/tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py >> @@ -0,0 +1,60 @@ >> +from __future__ import print_function >> +# >> +# Test auxiliary vector is loaded via gdbstub >> +# >> +# This is launched via tests/guest-debug/run-test.py >> +# >> + >> +import gdb >> +import sys >> + >> +failcount = 0 >> + >> +def report(cond, msg): >> + "Report success/fail of test" >> + if cond: >> + print ("PASS: %s" % (msg)) >> + else: >> + print ("FAIL: %s" % (msg)) >> + global failcount >> + failcount += 1 >> + >> +def run_test(): >> + "Run through the tests one by one" >> + >> + sym, ok = gdb.lookup_symbol("thread1_func") >> + gdb.execute("b thread1_func") >> + gdb.execute("c") >> + >> + frame = gdb.selected_frame() >> + report(str(frame.function()) == "thread1_func", "break @ >> %s"%frame) > > Does this actually check the correct thread is reported? It does it indirectly -- there is only one thread which can have thread1_func on the stack. If we do not report the correct thread, selected_frame() will point into the bowels of pthread_join, as that's where the main thread will be stuck at. I don't know of a direct way to check for the reported thread. gdb.selected_thread().name would be more direct, but I am not sure we would be able to correctly set the thread name on all supported platforms. > >> + >> +# >> +# This runs as the script it sourced (via -x, via run-test.py) >> +# >> +try: >> + inferior = gdb.selected_inferior() >> + arch = inferior.architecture() >> + print("ATTACHED: %s" % arch.name()) >> +except (gdb.error, AttributeError): >> + print("SKIPPING (not connected)", file=sys.stderr) >> + exit(0) >> + >> +if gdb.parse_and_eval('$pc') == 0: >> + print("SKIP: PC not set") >> + exit(0) >> + >> +try: >> + # These are not very useful in scripts >> + gdb.execute("set pagination off") >> + gdb.execute("set confirm off") >> + >> + # Run the actual tests >> + run_test() >> +except (gdb.error): >> + print ("GDB Exception: %s" % (sys.exc_info()[0])) >> + failcount += 1 >> + pass >> + >> +print("All tests complete: %d failures" % failcount) >> +exit(failcount) > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] gdbstub: Switch to the thread receiving a signal 2021-09-30 9:51 [PATCH] gdbstub: Switch to the thread receiving a signal Pavel Labath 2021-10-12 17:10 ` Pavel Labath 2021-10-19 17:02 ` Alex Bennée @ 2021-10-19 17:49 ` Pavel Labath 2021-10-20 8:35 ` Alex Bennée 2 siblings, 1 reply; 12+ messages in thread From: Pavel Labath @ 2021-10-19 17:49 UTC (permalink / raw) To: alex.bennee; +Cc: Pavel Labath, philmd, qemu-devel Respond with Txxthread:yyyy; instead of a plain Sxx to indicate which thread received the signal. Otherwise, the debugger will associate it with the main one. Also automatically select this thread, as that is what gdb expects. Signed-off-by: Pavel Labath <pavel@labath.sk> --- gdbstub.c | 8 ++- tests/tcg/multiarch/Makefile.target | 10 +++- .../gdbstub/test-thread-breakpoint.py | 60 +++++++++++++++++++ 3 files changed, 75 insertions(+), 3 deletions(-) create mode 100644 tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py diff --git a/gdbstub.c b/gdbstub.c index 36b85aa..23baaef 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -3138,8 +3138,12 @@ gdb_handlesig(CPUState *cpu, int sig) tb_flush(cpu); if (sig != 0) { - snprintf(buf, sizeof(buf), "S%02x", target_signal_to_gdb(sig)); - put_packet(buf); + 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(); } /* put_packet() might have detected that the peer terminated the connection. */ diff --git a/tests/tcg/multiarch/Makefile.target b/tests/tcg/multiarch/Makefile.target index 6ccb592..c84683f 100644 --- a/tests/tcg/multiarch/Makefile.target +++ b/tests/tcg/multiarch/Makefile.target @@ -70,11 +70,19 @@ run-gdbstub-qxfer-auxv-read: sha1 --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-qxfer-auxv-read.py, \ "basic gdbstub qXfer:auxv:read support") +run-gdbstub-thread-breakpoint: testthread + $(call run-test, $@, $(GDB_SCRIPT) \ + --gdb $(HAVE_GDB_BIN) \ + --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \ + --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-thread-breakpoint.py, \ + "hitting a breakpoint on non-main thread") + else run-gdbstub-%: $(call skip-test, "gdbstub test $*", "need working gdb") endif -EXTRA_RUNS += run-gdbstub-sha1 run-gdbstub-qxfer-auxv-read +EXTRA_RUNS += run-gdbstub-sha1 run-gdbstub-qxfer-auxv-read \ + run-gdbstub-thread-breakpoint # ARM Compatible Semi Hosting Tests # diff --git a/tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py b/tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py new file mode 100644 index 0000000..798d508 --- /dev/null +++ b/tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py @@ -0,0 +1,60 @@ +from __future__ import print_function +# +# Test auxiliary vector is loaded via gdbstub +# +# This is launched via tests/guest-debug/run-test.py +# + +import gdb +import sys + +failcount = 0 + +def report(cond, msg): + "Report success/fail of test" + if cond: + print ("PASS: %s" % (msg)) + else: + print ("FAIL: %s" % (msg)) + global failcount + failcount += 1 + +def run_test(): + "Run through the tests one by one" + + sym, ok = gdb.lookup_symbol("thread1_func") + gdb.execute("b thread1_func") + gdb.execute("c") + + frame = gdb.selected_frame() + report(str(frame.function()) == "thread1_func", "break @ %s"%frame) + +# +# This runs as the script it sourced (via -x, via run-test.py) +# +try: + inferior = gdb.selected_inferior() + arch = inferior.architecture() + print("ATTACHED: %s" % arch.name()) +except (gdb.error, AttributeError): + print("SKIPPING (not connected)", file=sys.stderr) + exit(0) + +if gdb.parse_and_eval('$pc') == 0: + print("SKIP: PC not set") + exit(0) + +try: + # These are not very useful in scripts + gdb.execute("set pagination off") + gdb.execute("set confirm off") + + # Run the actual tests + run_test() +except (gdb.error): + print ("GDB Exception: %s" % (sys.exc_info()[0])) + failcount += 1 + pass + +print("All tests complete: %d failures" % failcount) +exit(failcount) -- 2.32.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2] gdbstub: Switch to the thread receiving a signal 2021-10-19 17:49 ` [PATCH v2] " Pavel Labath @ 2021-10-20 8:35 ` Alex Bennée 2021-10-20 17:04 ` Pavel Labath 0 siblings, 1 reply; 12+ messages in thread From: Alex Bennée @ 2021-10-20 8:35 UTC (permalink / raw) To: Pavel Labath; +Cc: philmd, qemu-devel Pavel Labath <pavel@labath.sk> writes: > Respond with Txxthread:yyyy; instead of a plain Sxx to indicate which > thread received the signal. Otherwise, the debugger will associate it > with the main one. Also automatically select this thread, as that is > what gdb expects. Just for reference it's best to post vN's in a new thread as the Replied-to field can confuse some of the automatic tools (b4, patchew etc). > Signed-off-by: Pavel Labath <pavel@labath.sk> > --- > gdbstub.c | 8 ++- > tests/tcg/multiarch/Makefile.target | 10 +++- > .../gdbstub/test-thread-breakpoint.py | 60 +++++++++++++++++++ > 3 files changed, 75 insertions(+), 3 deletions(-) > create mode 100644 tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py > > diff --git a/gdbstub.c b/gdbstub.c > index 36b85aa..23baaef 100644 > --- a/gdbstub.c > +++ b/gdbstub.c > @@ -3138,8 +3138,12 @@ gdb_handlesig(CPUState *cpu, int sig) > tb_flush(cpu); > > if (sig != 0) { > - snprintf(buf, sizeof(buf), "S%02x", target_signal_to_gdb(sig)); > - put_packet(buf); > + 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(); > } > /* put_packet() might have detected that the peer terminated the > connection. */ > diff --git a/tests/tcg/multiarch/Makefile.target b/tests/tcg/multiarch/Makefile.target > index 6ccb592..c84683f 100644 > --- a/tests/tcg/multiarch/Makefile.target > +++ b/tests/tcg/multiarch/Makefile.target > @@ -70,11 +70,19 @@ run-gdbstub-qxfer-auxv-read: sha1 > --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-qxfer-auxv-read.py, \ > "basic gdbstub qXfer:auxv:read support") > > +run-gdbstub-thread-breakpoint: testthread > + $(call run-test, $@, $(GDB_SCRIPT) \ > + --gdb $(HAVE_GDB_BIN) \ > + --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \ > + --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-thread-breakpoint.py, \ > + "hitting a breakpoint on non-main thread") > + > else > run-gdbstub-%: > $(call skip-test, "gdbstub test $*", "need working gdb") > endif > -EXTRA_RUNS += run-gdbstub-sha1 run-gdbstub-qxfer-auxv-read > +EXTRA_RUNS += run-gdbstub-sha1 run-gdbstub-qxfer-auxv-read \ > + run-gdbstub-thread-breakpoint > > # ARM Compatible Semi Hosting Tests > # > diff --git a/tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py b/tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py > new file mode 100644 > index 0000000..798d508 > --- /dev/null > +++ b/tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py > @@ -0,0 +1,60 @@ > +from __future__ import print_function > +# > +# Test auxiliary vector is loaded via gdbstub I'm fairly sure this isn't what the test is doing... > +# > +# This is launched via tests/guest-debug/run-test.py > +# > + > +import gdb > +import sys > + > +failcount = 0 > + > +def report(cond, msg): > + "Report success/fail of test" > + if cond: > + print ("PASS: %s" % (msg)) > + else: > + print ("FAIL: %s" % (msg)) > + global failcount > + failcount += 1 > + > +def run_test(): > + "Run through the tests one by one" > + > + sym, ok = gdb.lookup_symbol("thread1_func") > + gdb.execute("b thread1_func") > + gdb.execute("c") > + > + frame = gdb.selected_frame() > + report(str(frame.function()) == "thread1_func", "break @ > %s"%frame) I think we can do better here by checking gdb.selected_thread() and ensuring the num (or global_num) is not 1. Also maybe check the is_stopped() status: https://sourceware.org/gdb/current/onlinedocs/gdb/Threads-In-Python.html I noticed while running the test that output still continued for some time from the other thread but it was still doing that pre this change so I'm not quite sure what was going on there. > + > +# > +# This runs as the script it sourced (via -x, via run-test.py) > +# > +try: > + inferior = gdb.selected_inferior() > + arch = inferior.architecture() > + print("ATTACHED: %s" % arch.name()) > +except (gdb.error, AttributeError): > + print("SKIPPING (not connected)", file=sys.stderr) > + exit(0) > + > +if gdb.parse_and_eval('$pc') == 0: > + print("SKIP: PC not set") > + exit(0) > + > +try: > + # These are not very useful in scripts > + gdb.execute("set pagination off") > + gdb.execute("set confirm off") > + > + # Run the actual tests > + run_test() > +except (gdb.error): > + print ("GDB Exception: %s" % (sys.exc_info()[0])) > + failcount += 1 > + pass > + > +print("All tests complete: %d failures" % failcount) > +exit(failcount) I also tried some manual testing: ➜ ./qemu-aarch64 -g 1234 tests/tcg/aarch64-linux-user/testthread fish: “./qemu-aarch64 -g 1234 tests/tc…” terminated by signal SIGSEGV (Address boundary error) 🕙12:33:49 alex@zen:qemu.git/builds/arm.all on gdbstub/next [$!?⇡] took 12s [⚡ SEGV] ✗ where in the other window I did: 0x00000000004005d0 in _start () (gdb) hbreak thread2_func Hardware assisted breakpoint 1 at 0x400824: file /home/alex/lsrc/qemu.git/tests/tcg/multiarch/testthread.c, line 34. (gdb) hbreak thread1_func Hardware assisted breakpoint 2 at 0x400798: file /home/alex/lsrc/qemu.git/tests/tcg/multiarch/testthread.c, line 22. (gdb) c Continuing. [New Thread 1.2748248] Remote connection closed which seems to indicate some problems with breaking on multiple threads. Maybe this is related to the weird output I was seeing above? -- Alex Bennée ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] gdbstub: Switch to the thread receiving a signal 2021-10-20 8:35 ` Alex Bennée @ 2021-10-20 17:04 ` Pavel Labath 2021-10-20 17:57 ` Alex Bennée 0 siblings, 1 reply; 12+ messages in thread From: Pavel Labath @ 2021-10-20 17:04 UTC (permalink / raw) To: Alex Bennée; +Cc: philmd, qemu-devel, stanshebs On 20/10/2021 10:35, Alex Bennée wrote: > > Pavel Labath <pavel@labath.sk> writes: > >> Respond with Txxthread:yyyy; instead of a plain Sxx to indicate which >> thread received the signal. Otherwise, the debugger will associate it >> with the main one. Also automatically select this thread, as that is >> what gdb expects. > > Just for reference it's best to post vN's in a new thread as the > Replied-to field can confuse some of the automatic tools (b4, patchew > etc). Got it. > >> Signed-off-by: Pavel Labath <pavel@labath.sk> >> --- >> gdbstub.c | 8 ++- >> tests/tcg/multiarch/Makefile.target | 10 +++- >> .../gdbstub/test-thread-breakpoint.py | 60 +++++++++++++++++++ >> 3 files changed, 75 insertions(+), 3 deletions(-) >> create mode 100644 tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py >> >> diff --git a/gdbstub.c b/gdbstub.c >> index 36b85aa..23baaef 100644 >> --- a/gdbstub.c >> +++ b/gdbstub.c >> @@ -3138,8 +3138,12 @@ gdb_handlesig(CPUState *cpu, int sig) >> tb_flush(cpu); >> >> if (sig != 0) { >> - snprintf(buf, sizeof(buf), "S%02x", target_signal_to_gdb(sig)); >> - put_packet(buf); >> + 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(); >> } >> /* put_packet() might have detected that the peer terminated the >> connection. */ >> diff --git a/tests/tcg/multiarch/Makefile.target b/tests/tcg/multiarch/Makefile.target >> index 6ccb592..c84683f 100644 >> --- a/tests/tcg/multiarch/Makefile.target >> +++ b/tests/tcg/multiarch/Makefile.target >> @@ -70,11 +70,19 @@ run-gdbstub-qxfer-auxv-read: sha1 >> --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-qxfer-auxv-read.py, \ >> "basic gdbstub qXfer:auxv:read support") >> >> +run-gdbstub-thread-breakpoint: testthread >> + $(call run-test, $@, $(GDB_SCRIPT) \ >> + --gdb $(HAVE_GDB_BIN) \ >> + --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \ >> + --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-thread-breakpoint.py, \ >> + "hitting a breakpoint on non-main thread") >> + >> else >> run-gdbstub-%: >> $(call skip-test, "gdbstub test $*", "need working gdb") >> endif >> -EXTRA_RUNS += run-gdbstub-sha1 run-gdbstub-qxfer-auxv-read >> +EXTRA_RUNS += run-gdbstub-sha1 run-gdbstub-qxfer-auxv-read \ >> + run-gdbstub-thread-breakpoint >> >> # ARM Compatible Semi Hosting Tests >> # >> diff --git a/tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py b/tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py >> new file mode 100644 >> index 0000000..798d508 >> --- /dev/null >> +++ b/tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py >> @@ -0,0 +1,60 @@ >> +from __future__ import print_function >> +# >> +# Test auxiliary vector is loaded via gdbstub > > I'm fairly sure this isn't what the test is doing... Oops. I'll fix this in the next version. > >> +# >> +# This is launched via tests/guest-debug/run-test.py >> +# >> + >> +import gdb >> +import sys >> + >> +failcount = 0 >> + >> +def report(cond, msg): >> + "Report success/fail of test" >> + if cond: >> + print ("PASS: %s" % (msg)) >> + else: >> + print ("FAIL: %s" % (msg)) >> + global failcount >> + failcount += 1 >> + >> +def run_test(): >> + "Run through the tests one by one" >> + >> + sym, ok = gdb.lookup_symbol("thread1_func") >> + gdb.execute("b thread1_func") >> + gdb.execute("c") >> + >> + frame = gdb.selected_frame() >> + report(str(frame.function()) == "thread1_func", "break @ >> %s"%frame) > > I think we can do better here by checking gdb.selected_thread() and > ensuring the num (or global_num) is not 1. Also maybe check the > is_stopped() status: Checking `num` is a good idea. Checking is_stopped() doesn't hurt either, though I believe that (in all-stop mode) gdb just hardwires this to True for all threads, even those that are not actually stopped (more on that below). However, if that's ok with you, I think it'd still be nice to keep the frame check as well. > > https://sourceware.org/gdb/current/onlinedocs/gdb/Threads-In-Python.html > > I noticed while running the test that output still continued for some > time from the other thread but it was still doing that pre this change > so I'm not quite sure what was going on there. > >> + >> +# >> +# This runs as the script it sourced (via -x, via run-test.py) >> +# >> +try: >> + inferior = gdb.selected_inferior() >> + arch = inferior.architecture() >> + print("ATTACHED: %s" % arch.name()) >> +except (gdb.error, AttributeError): >> + print("SKIPPING (not connected)", file=sys.stderr) >> + exit(0) >> + >> +if gdb.parse_and_eval('$pc') == 0: >> + print("SKIP: PC not set") >> + exit(0) >> + >> +try: >> + # These are not very useful in scripts >> + gdb.execute("set pagination off") >> + gdb.execute("set confirm off") >> + >> + # Run the actual tests >> + run_test() >> +except (gdb.error): >> + print ("GDB Exception: %s" % (sys.exc_info()[0])) >> + failcount += 1 >> + pass >> + >> +print("All tests complete: %d failures" % failcount) >> +exit(failcount) > > I also tried some manual testing: > > ➜ ./qemu-aarch64 -g 1234 tests/tcg/aarch64-linux-user/testthread > fish: “./qemu-aarch64 -g 1234 tests/tc…” terminated by signal SIGSEGV (Address boundary error) > 🕙12:33:49 alex@zen:qemu.git/builds/arm.all on gdbstub/next [$!?⇡] took 12s [⚡ SEGV] > ✗ > > where in the other window I did: > > 0x00000000004005d0 in _start () > (gdb) hbreak thread2_func > Hardware assisted breakpoint 1 at 0x400824: file /home/alex/lsrc/qemu.git/tests/tcg/multiarch/testthread.c, line 34. > (gdb) hbreak thread1_func > Hardware assisted breakpoint 2 at 0x400798: file /home/alex/lsrc/qemu.git/tests/tcg/multiarch/testthread.c, line 22. > (gdb) c > Continuing. > [New Thread 1.2748248] > Remote connection closed > > which seems to indicate some problems with breaking on multiple threads. > Maybe this is related to the weird output I was seeing above? > Yes, that's definitely related. What's happening is that the qemu does not stop other thread when one of them hits a breakpoint (or stops for any other reason) -- as far as I can tell it does not have any code which would even attempt to do that. This is why you're seeing the output even after the process is purportedly stopped. Things get even more interesting when you have two threads hitting a breakpoint simultaneously. At that point both of them will enter their gdb stubs and attempt to talk to gdb at the same time. As you can imagine, this cannot end well, and eventually the connection will become so messed up that one side just gives up and terminates the link. I am aware of this issue, and I (well, Stan (cc'ed) is, for the most part) looking for a way to fix it. If you have any ideas, we'd very much like to hear them. The way I see it, we need to implement some kind of a "stop the world" mechanism, to stop/interrupt all threads whenever the gdb stub becomes active (and make sure it can handle simultaneous debug events). However, I am don't know enough about qemu internals to tell how to actually go about doing that. My plan was to "get my feet wet" with a simple patch that improves the situation for the case when there are no simultaneous debug events, and eventually hopefully figure out a way how to address the bigger problem. regards, Pavel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] gdbstub: Switch to the thread receiving a signal 2021-10-20 17:04 ` Pavel Labath @ 2021-10-20 17:57 ` Alex Bennée 2021-10-21 17:36 ` Pavel Labath 0 siblings, 1 reply; 12+ messages in thread From: Alex Bennée @ 2021-10-20 17:57 UTC (permalink / raw) To: Pavel Labath; +Cc: philmd, qemu-devel, stanshebs Pavel Labath <pavel@labath.sk> writes: > On 20/10/2021 10:35, Alex Bennée wrote: >> Pavel Labath <pavel@labath.sk> writes: >> >>> Respond with Txxthread:yyyy; instead of a plain Sxx to indicate which >>> thread received the signal. Otherwise, the debugger will associate it >>> with the main one. Also automatically select this thread, as that is >>> what gdb expects. >> Just for reference it's best to post vN's in a new thread as the >> Replied-to field can confuse some of the automatic tools (b4, patchew >> etc). > > Got it. >> >>> Signed-off-by: Pavel Labath <pavel@labath.sk> >>> --- >>> gdbstub.c | 8 ++- >>> tests/tcg/multiarch/Makefile.target | 10 +++- >>> .../gdbstub/test-thread-breakpoint.py | 60 +++++++++++++++++++ >>> 3 files changed, 75 insertions(+), 3 deletions(-) >>> create mode 100644 tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py >>> >>> diff --git a/gdbstub.c b/gdbstub.c >>> index 36b85aa..23baaef 100644 >>> --- a/gdbstub.c >>> +++ b/gdbstub.c >>> @@ -3138,8 +3138,12 @@ gdb_handlesig(CPUState *cpu, int sig) >>> tb_flush(cpu); >>> if (sig != 0) { >>> - snprintf(buf, sizeof(buf), "S%02x", target_signal_to_gdb(sig)); >>> - put_packet(buf); >>> + 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(); >>> } >>> /* put_packet() might have detected that the peer terminated the >>> connection. */ >>> diff --git a/tests/tcg/multiarch/Makefile.target b/tests/tcg/multiarch/Makefile.target >>> index 6ccb592..c84683f 100644 >>> --- a/tests/tcg/multiarch/Makefile.target >>> +++ b/tests/tcg/multiarch/Makefile.target >>> @@ -70,11 +70,19 @@ run-gdbstub-qxfer-auxv-read: sha1 >>> --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-qxfer-auxv-read.py, \ >>> "basic gdbstub qXfer:auxv:read support") >>> +run-gdbstub-thread-breakpoint: testthread >>> + $(call run-test, $@, $(GDB_SCRIPT) \ >>> + --gdb $(HAVE_GDB_BIN) \ >>> + --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \ >>> + --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-thread-breakpoint.py, \ >>> + "hitting a breakpoint on non-main thread") >>> + >>> else >>> run-gdbstub-%: >>> $(call skip-test, "gdbstub test $*", "need working gdb") >>> endif >>> -EXTRA_RUNS += run-gdbstub-sha1 run-gdbstub-qxfer-auxv-read >>> +EXTRA_RUNS += run-gdbstub-sha1 run-gdbstub-qxfer-auxv-read \ >>> + run-gdbstub-thread-breakpoint >>> # ARM Compatible Semi Hosting Tests >>> # >>> diff --git a/tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py b/tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py >>> new file mode 100644 >>> index 0000000..798d508 >>> --- /dev/null >>> +++ b/tests/tcg/multiarch/gdbstub/test-thread-breakpoint.py >>> @@ -0,0 +1,60 @@ >>> +from __future__ import print_function >>> +# >>> +# Test auxiliary vector is loaded via gdbstub >> I'm fairly sure this isn't what the test is doing... > Oops. I'll fix this in the next version. > >> >>> +# >>> +# This is launched via tests/guest-debug/run-test.py >>> +# >>> + >>> +import gdb >>> +import sys >>> + >>> +failcount = 0 >>> + >>> +def report(cond, msg): >>> + "Report success/fail of test" >>> + if cond: >>> + print ("PASS: %s" % (msg)) >>> + else: >>> + print ("FAIL: %s" % (msg)) >>> + global failcount >>> + failcount += 1 >>> + >>> +def run_test(): >>> + "Run through the tests one by one" >>> + >>> + sym, ok = gdb.lookup_symbol("thread1_func") >>> + gdb.execute("b thread1_func") >>> + gdb.execute("c") >>> + >>> + frame = gdb.selected_frame() >>> + report(str(frame.function()) == "thread1_func", "break @ >>> %s"%frame) >> I think we can do better here by checking gdb.selected_thread() and >> ensuring the num (or global_num) is not 1. Also maybe check the >> is_stopped() status: > > Checking `num` is a good idea. Checking is_stopped() doesn't hurt > either, though I believe that (in all-stop mode) gdb just hardwires > this to True for all threads, even those that are not actually stopped > (more on that below). > > However, if that's ok with you, I think it'd still be nice to keep the > frame check as well. That's fine. >> https://sourceware.org/gdb/current/onlinedocs/gdb/Threads-In-Python.html >> I noticed while running the test that output still continued for >> some >> time from the other thread but it was still doing that pre this change >> so I'm not quite sure what was going on there. >> >>> + >>> +# >>> +# This runs as the script it sourced (via -x, via run-test.py) >>> +# >>> +try: >>> + inferior = gdb.selected_inferior() >>> + arch = inferior.architecture() >>> + print("ATTACHED: %s" % arch.name()) >>> +except (gdb.error, AttributeError): >>> + print("SKIPPING (not connected)", file=sys.stderr) >>> + exit(0) >>> + >>> +if gdb.parse_and_eval('$pc') == 0: >>> + print("SKIP: PC not set") >>> + exit(0) >>> + >>> +try: >>> + # These are not very useful in scripts >>> + gdb.execute("set pagination off") >>> + gdb.execute("set confirm off") >>> + >>> + # Run the actual tests >>> + run_test() >>> +except (gdb.error): >>> + print ("GDB Exception: %s" % (sys.exc_info()[0])) >>> + failcount += 1 >>> + pass >>> + >>> +print("All tests complete: %d failures" % failcount) >>> +exit(failcount) >> I also tried some manual testing: >> ➜ ./qemu-aarch64 -g 1234 tests/tcg/aarch64-linux-user/testthread >> fish: “./qemu-aarch64 -g 1234 tests/tc…” terminated by signal SIGSEGV (Address boundary error) >> 🕙12:33:49 alex@zen:qemu.git/builds/arm.all on gdbstub/next [$!?⇡] took 12s [⚡ SEGV] >> ✗ >> where in the other window I did: >> 0x00000000004005d0 in _start () >> (gdb) hbreak thread2_func >> Hardware assisted breakpoint 1 at 0x400824: file /home/alex/lsrc/qemu.git/tests/tcg/multiarch/testthread.c, line 34. >> (gdb) hbreak thread1_func >> Hardware assisted breakpoint 2 at 0x400798: file /home/alex/lsrc/qemu.git/tests/tcg/multiarch/testthread.c, line 22. >> (gdb) c >> Continuing. >> [New Thread 1.2748248] >> Remote connection closed >> which seems to indicate some problems with breaking on multiple >> threads. >> Maybe this is related to the weird output I was seeing above? >> > > Yes, that's definitely related. What's happening is that the qemu does > not stop other thread when one of them hits a breakpoint (or stops for > any other reason) -- as far as I can tell it does not have any code > which would even attempt to do that. This is why you're seeing the > output even after the process is purportedly stopped. > > Things get even more interesting when you have two threads hitting a > breakpoint simultaneously. At that point both of them will enter their > gdb stubs and attempt to talk to gdb at the same time. As you can > imagine, this cannot end well, and eventually the connection will > become so messed up that one side just gives up and terminates the > link. > > I am aware of this issue, and I (well, Stan (cc'ed) is, for the most > part) looking for a way to fix it. If you have any ideas, we'd very > much like to hear them. The way I see it, we need to implement some > kind of a "stop the world" mechanism, to stop/interrupt all threads > whenever the gdb stub becomes active (and make sure it can handle > simultaneous debug events). vm_stop(RUN_STATE_PAUSED) should do the trick. We do it elsewhere in the gdbstub. > However, I am don't know enough about qemu > internals to tell how to actually go about doing that. > > My plan was to "get my feet wet" with a simple patch that improves the > situation for the case when there are no simultaneous debug events, > and eventually hopefully figure out a way how to address the bigger > problem. Great. Once you've made the changes I think the patch is ready to go in and the larger questions can be dealt with later. > > regards, > Pavel -- Alex Bennée ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] gdbstub: Switch to the thread receiving a signal 2021-10-20 17:57 ` Alex Bennée @ 2021-10-21 17:36 ` Pavel Labath 0 siblings, 0 replies; 12+ messages in thread From: Pavel Labath @ 2021-10-21 17:36 UTC (permalink / raw) To: Alex Bennée; +Cc: philmd, qemu-devel, stanshebs On 20/10/2021 19:57, Alex Bennée wrote: > > Pavel Labath <pavel@labath.sk> writes: > >> On 20/10/2021 10:35, Alex Bennée wrote: >>> Maybe this is related to the weird output I was seeing above? >>> >> >> Yes, that's definitely related. What's happening is that the qemu does >> not stop other thread when one of them hits a breakpoint (or stops for >> any other reason) -- as far as I can tell it does not have any code >> which would even attempt to do that. This is why you're seeing the >> output even after the process is purportedly stopped. >> >> Things get even more interesting when you have two threads hitting a >> breakpoint simultaneously. At that point both of them will enter their >> gdb stubs and attempt to talk to gdb at the same time. As you can >> imagine, this cannot end well, and eventually the connection will >> become so messed up that one side just gives up and terminates the >> link. >> >> I am aware of this issue, and I (well, Stan (cc'ed) is, for the most >> part) looking for a way to fix it. If you have any ideas, we'd very >> much like to hear them. The way I see it, we need to implement some >> kind of a "stop the world" mechanism, to stop/interrupt all threads >> whenever the gdb stub becomes active (and make sure it can handle >> simultaneous debug events). > > vm_stop(RUN_STATE_PAUSED) should do the trick. We do it elsewhere in > the gdbstub. Unfortunately, it seems that vm_stop is only available in softmmu targets. Is there a user-mode equivalent by any chance? > >> However, I am don't know enough about qemu >> internals to tell how to actually go about doing that. >> >> My plan was to "get my feet wet" with a simple patch that improves the >> situation for the case when there are no simultaneous debug events, >> and eventually hopefully figure out a way how to address the bigger >> problem. > > Great. Once you've made the changes I think the patch is ready to go in > and the larger questions can be dealt with later. Cool. I've sent out v3 of the patch. Let me know if there's anything else I need to do. regards, Pavel ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-10-21 17:48 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-30 9:51 [PATCH] gdbstub: Switch to the thread receiving a signal Pavel Labath 2021-10-12 17:10 ` Pavel Labath 2021-10-15 15:59 ` Alex Bennée 2021-10-19 13:19 ` Pavel Labath 2021-10-19 17:13 ` Alex Bennée 2021-10-19 17:02 ` Alex Bennée 2021-10-19 17:53 ` Pavel Labath 2021-10-19 17:49 ` [PATCH v2] " Pavel Labath 2021-10-20 8:35 ` Alex Bennée 2021-10-20 17:04 ` Pavel Labath 2021-10-20 17:57 ` Alex Bennée 2021-10-21 17:36 ` Pavel Labath
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).