[PATCH 1/2] gdbstub: Add Xfer:siginfo:read stub

[PATCH 1/2] gdbstub: Add Xfer:siginfo:read stub

[Gustavo Romero]

Add stub to handle Xfer:siginfo:read query that requests the machine's
siginfo data.

This is used when GDB users execute 'print $_siginfo' and when the
machine stops due to a signal, like on a SIGSEGV. The information in
siginfo allows GDB to determine further details on the signal, like the
fault address/insn when the SIGSEGV is caught. The siginfo is also used
by GDB to find out the si_code automatically and show additional info to
the user in some cases.

This is only a QEMU user mode and Linux-only feature.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
 gdbstub/gdbstub.c     |  9 +++++++++
 gdbstub/internals.h   |  1 +
 gdbstub/user-target.c | 31 +++++++++++++++++++++++++++++++
 3 files changed, 41 insertions(+)

diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 2909bc8c69..54c1f6fb3c 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -1650,7 +1650,10 @@ static void handle_query_supported(GArray *params, void *user_ctx)
     if (gdbserver_state.c_cpu->opaque) {
         g_string_append(gdbserver_state.str_buf, ";qXfer:auxv:read+");
     }
+
     g_string_append(gdbserver_state.str_buf, ";QCatchSyscalls+");
+
+    g_string_append(gdbserver_state.str_buf, ";qXfer:siginfo:read+");
 #endif
     g_string_append(gdbserver_state.str_buf, ";qXfer:exec-file:read+");
 #endif
@@ -1799,6 +1802,12 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = {
         .cmd_startswith = 1,
         .schema = "l,l0"
     },
+    {
+        .handler = gdb_handle_query_xfer_siginfo,
+        .cmd = "Xfer:siginfo:read::",
+        .cmd_startswith = 1,
+        .schema = "l,l0"
+     },
 #endif
     {
         .handler = gdb_handle_query_xfer_exec_file,
diff --git a/gdbstub/internals.h b/gdbstub/internals.h
index 56b7c13b75..fcfe7c2d26 100644
--- a/gdbstub/internals.h
+++ b/gdbstub/internals.h
@@ -190,6 +190,7 @@ typedef union GdbCmdVariant {
 void gdb_handle_query_rcmd(GArray *params, void *user_ctx); /* softmmu */
 void gdb_handle_query_offsets(GArray *params, void *user_ctx); /* user */
 void gdb_handle_query_xfer_auxv(GArray *params, void *user_ctx); /*user */
+void gdb_handle_query_xfer_siginfo(GArray *params, void *user_ctx); /*user */
 void gdb_handle_v_file_open(GArray *params, void *user_ctx); /* user */
 void gdb_handle_v_file_close(GArray *params, void *user_ctx); /* user */
 void gdb_handle_v_file_pread(GArray *params, void *user_ctx); /* user */
diff --git a/gdbstub/user-target.c b/gdbstub/user-target.c
index b7d4c37cd8..3a4cf96622 100644
--- a/gdbstub/user-target.c
+++ b/gdbstub/user-target.c
@@ -284,6 +284,37 @@ void gdb_handle_query_xfer_auxv(GArray *params, void *user_ctx)
     gdb_put_packet_binary(gdbserver_state.str_buf->str,
                       gdbserver_state.str_buf->len, true);
 }
+
+void gdb_handle_query_xfer_siginfo(GArray *params, void *user_ctx)
+{
+    TaskState *ts;
+    unsigned long offset, len;
+    target_siginfo_t tmp_siginfo;
+    uint8_t *siginfo_offset;
+
+    offset = get_param(params, 0)->val_ul;
+    len = get_param(params, 1)->val_ul;
+
+    if (offset + len > sizeof(target_siginfo_t)) {
+        /* Invalid offset and/or requested length. */
+        gdb_put_packet("E01");
+        return;
+    }
+
+    ts = gdbserver_state.c_cpu->opaque;
+
+    /* Filter out si_type from si_code. See comment in siginfo_noswap(). */
+    tmp_siginfo = ts->sync_signal.info;
+    tmp_siginfo.si_code = sextract32(tmp_siginfo.si_code, 0, 16);
+
+    siginfo_offset = (uint8_t *)&tmp_siginfo + offset;
+
+    /* Reply */
+    g_string_assign(gdbserver_state.str_buf, "l");
+    gdb_memtox(gdbserver_state.str_buf, (const char *)siginfo_offset, len);
+    gdb_put_packet_binary(gdbserver_state.str_buf->str,
+                          gdbserver_state.str_buf->len, true);
+}
 #endif
 
 static const char *get_filename_param(GArray *params, int i)
-- 
2.34.1

[PATCH 2/2] tests/tcg: Add multiarch test for Xfer:siginfo:read stub

[Gustavo Romero]

Add multiarch test for testing if Xfer:siginfo:read query is properly
handled by gdbstub.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
 tests/tcg/multiarch/Makefile.target           | 10 ++++++-
 .../gdbstub/test-qxfer-siginfo-read.py        | 26 +++++++++++++++++++
 tests/tcg/multiarch/segfault.c                | 14 ++++++++++
 3 files changed, 49 insertions(+), 1 deletion(-)
 create mode 100644 tests/tcg/multiarch/gdbstub/test-qxfer-siginfo-read.py
 create mode 100644 tests/tcg/multiarch/segfault.c

diff --git a/tests/tcg/multiarch/Makefile.target b/tests/tcg/multiarch/Makefile.target
index e10951a801..61cda9640e 100644
--- a/tests/tcg/multiarch/Makefile.target
+++ b/tests/tcg/multiarch/Makefile.target
@@ -80,6 +80,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-qxfer-siginfo-read: segfault
+	$(call run-test, $@, $(GDB_SCRIPT) \
+		--gdb $(GDB) \
+		--qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
+		--bin "$< -s" --test $(MULTIARCH_SRC)/gdbstub/test-qxfer-siginfo-read.py, \
+	basic gdbstub qXfer:siginfo:read support)
+
 run-gdbstub-proc-mappings: sha1
 	$(call run-test, $@, $(GDB_SCRIPT) \
 		--gdb $(GDB) \
@@ -122,7 +129,8 @@ endif
 EXTRA_RUNS += run-gdbstub-sha1 run-gdbstub-qxfer-auxv-read \
 	      run-gdbstub-proc-mappings run-gdbstub-thread-breakpoint \
 	      run-gdbstub-registers run-gdbstub-prot-none \
-	      run-gdbstub-catch-syscalls
+	      run-gdbstub-catch-syscalls \
+	      run-gdbstub-qxfer-siginfo-read
 
 # ARM Compatible Semi Hosting Tests
 #
diff --git a/tests/tcg/multiarch/gdbstub/test-qxfer-siginfo-read.py b/tests/tcg/multiarch/gdbstub/test-qxfer-siginfo-read.py
new file mode 100644
index 0000000000..862596b07a
--- /dev/null
+++ b/tests/tcg/multiarch/gdbstub/test-qxfer-siginfo-read.py
@@ -0,0 +1,26 @@
+from __future__ import print_function
+#
+# Test gdbstub Xfer:siginfo:read stub.
+#
+# The test runs a binary that causes a SIGSEGV and then looks for additional
+# info about the signal through printing GDB's '$_siginfo' special variable,
+# which sends a Xfer:siginfo:read query to the gdbstub.
+#
+# The binary causes a SIGSEGV at dereferencing a pointer with value 0xdeadbeef,
+# so the test looks for and checks if this address is correctly reported by the
+# gdbstub.
+#
+# This is launched via tests/guest-debug/run-test.py
+#
+
+import gdb
+from test_gdbstub import main, report
+
+def run_test():
+    "Run through the test"
+
+    gdb.execute("continue", False, True)
+    resp = gdb.execute("print/x $_siginfo", False, True)
+    report(resp.find("si_addr = 0xdeadbeef"), "Found fault address.")
+
+main(run_test)
diff --git a/tests/tcg/multiarch/segfault.c b/tests/tcg/multiarch/segfault.c
new file mode 100644
index 0000000000..e6c8ff31ca
--- /dev/null
+++ b/tests/tcg/multiarch/segfault.c
@@ -0,0 +1,14 @@
+#include <stdio.h>
+#include <string.h>
+
+/* Cause a segfault for testing purposes. */
+
+int main(int argc, char *argv[])
+{
+    int *ptr = (void *)0xdeadbeef;
+
+    if (argc == 2 && strcmp(argv[1], "-s") == 0) {
+        /* Cause segfault. */
+        printf("%d\n", *ptr);
+    }
+}
-- 
2.34.1

Re: [PATCH 1/2] gdbstub: Add Xfer:siginfo:read stub

[Richard Henderson]

On 3/3/24 09:26, Gustavo Romero wrote:
> +    /* Filter out si_type from si_code. See comment in siginfo_noswap(). */ > +    tmp_siginfo = ts->sync_signal.info;
> +    tmp_siginfo.si_code = sextract32(tmp_siginfo.si_code, 0, 16);


This is incorrect, as it only handles synchronous signals.

In handle_pending_signal(), struct emulated_sigtable is passed, which has the correct 
siginfo (all of it, so no need for the adjustment).  I think you need to pass that in to 
gdb_handlesig so that a copy can be made for later xfer.


r~

Re: [PATCH 2/2] tests/tcg: Add multiarch test for Xfer:siginfo:read stub

[Richard Henderson]

On 3/3/24 09:26, Gustavo Romero wrote:
> Add multiarch test for testing if Xfer:siginfo:read query is properly
> handled by gdbstub.
> 
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
>   tests/tcg/multiarch/Makefile.target           | 10 ++++++-
>   .../gdbstub/test-qxfer-siginfo-read.py        | 26 +++++++++++++++++++
>   tests/tcg/multiarch/segfault.c                | 14 ++++++++++
>   3 files changed, 49 insertions(+), 1 deletion(-)
>   create mode 100644 tests/tcg/multiarch/gdbstub/test-qxfer-siginfo-read.py
>   create mode 100644 tests/tcg/multiarch/segfault.c
> 
> diff --git a/tests/tcg/multiarch/Makefile.target b/tests/tcg/multiarch/Makefile.target
> index e10951a801..61cda9640e 100644
> --- a/tests/tcg/multiarch/Makefile.target
> +++ b/tests/tcg/multiarch/Makefile.target
> @@ -80,6 +80,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-qxfer-siginfo-read: segfault
> +	$(call run-test, $@, $(GDB_SCRIPT) \
> +		--gdb $(GDB) \
> +		--qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
> +		--bin "$< -s" --test $(MULTIARCH_SRC)/gdbstub/test-qxfer-siginfo-read.py, \
> +	basic gdbstub qXfer:siginfo:read support)
> +
>   run-gdbstub-proc-mappings: sha1
>   	$(call run-test, $@, $(GDB_SCRIPT) \
>   		--gdb $(GDB) \
> @@ -122,7 +129,8 @@ endif
>   EXTRA_RUNS += run-gdbstub-sha1 run-gdbstub-qxfer-auxv-read \
>   	      run-gdbstub-proc-mappings run-gdbstub-thread-breakpoint \
>   	      run-gdbstub-registers run-gdbstub-prot-none \
> -	      run-gdbstub-catch-syscalls
> +	      run-gdbstub-catch-syscalls \
> +	      run-gdbstub-qxfer-siginfo-read
>   
>   # ARM Compatible Semi Hosting Tests
>   #
> diff --git a/tests/tcg/multiarch/gdbstub/test-qxfer-siginfo-read.py b/tests/tcg/multiarch/gdbstub/test-qxfer-siginfo-read.py
> new file mode 100644
> index 0000000000..862596b07a
> --- /dev/null
> +++ b/tests/tcg/multiarch/gdbstub/test-qxfer-siginfo-read.py
> @@ -0,0 +1,26 @@
> +from __future__ import print_function
> +#
> +# Test gdbstub Xfer:siginfo:read stub.
> +#
> +# The test runs a binary that causes a SIGSEGV and then looks for additional
> +# info about the signal through printing GDB's '$_siginfo' special variable,
> +# which sends a Xfer:siginfo:read query to the gdbstub.
> +#
> +# The binary causes a SIGSEGV at dereferencing a pointer with value 0xdeadbeef,
> +# so the test looks for and checks if this address is correctly reported by the
> +# gdbstub.
> +#
> +# This is launched via tests/guest-debug/run-test.py
> +#
> +
> +import gdb
> +from test_gdbstub import main, report
> +
> +def run_test():
> +    "Run through the test"
> +
> +    gdb.execute("continue", False, True)
> +    resp = gdb.execute("print/x $_siginfo", False, True)
> +    report(resp.find("si_addr = 0xdeadbeef"), "Found fault address.")
> +
> +main(run_test)
> diff --git a/tests/tcg/multiarch/segfault.c b/tests/tcg/multiarch/segfault.c
> new file mode 100644
> index 0000000000..e6c8ff31ca
> --- /dev/null
> +++ b/tests/tcg/multiarch/segfault.c
> @@ -0,0 +1,14 @@
> +#include <stdio.h>
> +#include <string.h>
> +
> +/* Cause a segfault for testing purposes. */
> +
> +int main(int argc, char *argv[])
> +{
> +    int *ptr = (void *)0xdeadbeef;
> +
> +    if (argc == 2 && strcmp(argv[1], "-s") == 0) {
> +        /* Cause segfault. */
> +        printf("%d\n", *ptr);
> +    }
> +}

Any reason SIGSEGV is interesting?
Perhaps just abort for SIGABRT instead?

A test using setitimer to raise SIGALRM would test the async path.


r~

Re: [PATCH 2/2] tests/tcg: Add multiarch test for Xfer:siginfo:read stub

[Gustavo Romero]

Hi Richard!

On 3/4/24 2:21 PM, Richard Henderson wrote:
> On 3/3/24 09:26, Gustavo Romero wrote:
>> Add multiarch test for testing if Xfer:siginfo:read query is properly
>> handled by gdbstub.
>>
>> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
>> ---
>>   tests/tcg/multiarch/Makefile.target           | 10 ++++++-
>>   .../gdbstub/test-qxfer-siginfo-read.py        | 26 +++++++++++++++++++
>>   tests/tcg/multiarch/segfault.c                | 14 ++++++++++
>>   3 files changed, 49 insertions(+), 1 deletion(-)
>>   create mode 100644 tests/tcg/multiarch/gdbstub/test-qxfer-siginfo-read.py
>>   create mode 100644 tests/tcg/multiarch/segfault.c
>>
>> diff --git a/tests/tcg/multiarch/Makefile.target b/tests/tcg/multiarch/Makefile.target
>> index e10951a801..61cda9640e 100644
>> --- a/tests/tcg/multiarch/Makefile.target
>> +++ b/tests/tcg/multiarch/Makefile.target
>> @@ -80,6 +80,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-qxfer-siginfo-read: segfault
>> +    $(call run-test, $@, $(GDB_SCRIPT) \
>> +        --gdb $(GDB) \
>> +        --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
>> +        --bin "$< -s" --test $(MULTIARCH_SRC)/gdbstub/test-qxfer-siginfo-read.py, \
>> +    basic gdbstub qXfer:siginfo:read support)
>> +
>>   run-gdbstub-proc-mappings: sha1
>>       $(call run-test, $@, $(GDB_SCRIPT) \
>>           --gdb $(GDB) \
>> @@ -122,7 +129,8 @@ endif
>>   EXTRA_RUNS += run-gdbstub-sha1 run-gdbstub-qxfer-auxv-read \
>>             run-gdbstub-proc-mappings run-gdbstub-thread-breakpoint \
>>             run-gdbstub-registers run-gdbstub-prot-none \
>> -          run-gdbstub-catch-syscalls
>> +          run-gdbstub-catch-syscalls \
>> +          run-gdbstub-qxfer-siginfo-read
>>   # ARM Compatible Semi Hosting Tests
>>   #
>> diff --git a/tests/tcg/multiarch/gdbstub/test-qxfer-siginfo-read.py b/tests/tcg/multiarch/gdbstub/test-qxfer-siginfo-read.py
>> new file mode 100644
>> index 0000000000..862596b07a
>> --- /dev/null
>> +++ b/tests/tcg/multiarch/gdbstub/test-qxfer-siginfo-read.py
>> @@ -0,0 +1,26 @@
>> +from __future__ import print_function
>> +#
>> +# Test gdbstub Xfer:siginfo:read stub.
>> +#
>> +# The test runs a binary that causes a SIGSEGV and then looks for additional
>> +# info about the signal through printing GDB's '$_siginfo' special variable,
>> +# which sends a Xfer:siginfo:read query to the gdbstub.
>> +#
>> +# The binary causes a SIGSEGV at dereferencing a pointer with value 0xdeadbeef,
>> +# so the test looks for and checks if this address is correctly reported by the
>> +# gdbstub.
>> +#
>> +# This is launched via tests/guest-debug/run-test.py
>> +#
>> +
>> +import gdb
>> +from test_gdbstub import main, report
>> +
>> +def run_test():
>> +    "Run through the test"
>> +
>> +    gdb.execute("continue", False, True)
>> +    resp = gdb.execute("print/x $_siginfo", False, True)
>> +    report(resp.find("si_addr = 0xdeadbeef"), "Found fault address.")
>> +
>> +main(run_test)
>> diff --git a/tests/tcg/multiarch/segfault.c b/tests/tcg/multiarch/segfault.c
>> new file mode 100644
>> index 0000000000..e6c8ff31ca
>> --- /dev/null
>> +++ b/tests/tcg/multiarch/segfault.c
>> @@ -0,0 +1,14 @@
>> +#include <stdio.h>
>> +#include <string.h>
>> +
>> +/* Cause a segfault for testing purposes. */
>> +
>> +int main(int argc, char *argv[])
>> +{
>> +    int *ptr = (void *)0xdeadbeef;
>> +
>> +    if (argc == 2 && strcmp(argv[1], "-s") == 0) {
>> +        /* Cause segfault. */
>> +        printf("%d\n", *ptr);
>> +    }
>> +}
> 
> Any reason SIGSEGV is interesting?

I'm particularly interested in the SIGSEGV because that's the signal
generated on a MTE tag mismatch. GDB uses the si_code to show
additional info on the fault, for instance:

gromero@arm64:~$ gdb -q
(gdb) target remote amd:1234
Remote debugging using amd:1234
Reading /home/gromero/git/qemu/build/mte_t from remote target...
warning: File transfers from remote targets can be slow. Use "set sysroot" to access files locally instead.
Reading /home/gromero/git/qemu/build/mte_t from remote target...
Reading symbols from target:/home/gromero/git/qemu/build/mte_t...
Failed to read a valid object file image from memory.
0x0000000000400580 in _start ()
(gdb) c
Continuing.

Program received signal SIGSEGV, Segmentation fault
Memory tag violation           <============ (the info I'm keen on)
Fault address unavailable.
0x0000000000407290 in puts ()
(gdb)


> Perhaps just abort for SIGABRT instead?

Although this can make a simpler test, the test can't control
the si_addr value easily, which I think is interesting to be tested.

Why do you prefer SIGABRT?


> A test using setitimer to raise SIGALRM would test the async path.

SIGLARM doesn't generate any interesting siginfo?

gromero@arm64:~$ gdb -q ./sigalrm
Reading symbols from ./sigalrm...
(gdb) run
Starting program: /home/gromero/sigalrm

Program terminated with signal SIGALRM, Alarm clock.
The program no longer exists.
(gdb) p $_siginfo
$1 = void
(gdb)


Cheers,
Gustavo

Re: [PATCH 2/2] tests/tcg: Add multiarch test for Xfer:siginfo:read stub

[Richard Henderson]

On 3/4/24 10:59, Gustavo Romero wrote:
>> Perhaps just abort for SIGABRT instead?
> 
> Although this can make a simpler test, the test can't control
> the si_addr value easily, which I think is interesting to be tested.
> 
> Why do you prefer SIGABRT?

I missed that you were testing si_addr -- in which case SIGSEGV is a good match.


>> A test using setitimer to raise SIGALRM would test the async path.
> 
> SIGLARM doesn't generate any interesting siginfo?

It should at minimum have si_sig = SIGALRM.

> 
> gromero@arm64:~$ gdb -q ./sigalrm
> Reading symbols from ./sigalrm...
> (gdb) run
> Starting program: /home/gromero/sigalrm
> 
> Program terminated with signal SIGALRM, Alarm clock.
> The program no longer exists.
> (gdb) p $_siginfo
> $1 = void

Well that's because the program died.
Do you need to have gdb handle the signal?


r~

Re: [PATCH 2/2] tests/tcg: Add multiarch test for Xfer:siginfo:read stub

[Gustavo Romero]

Hi Richard,

On 3/4/24 7:51 PM, Richard Henderson wrote:
> On 3/4/24 10:59, Gustavo Romero wrote:
>>> Perhaps just abort for SIGABRT instead?
>>
>> Although this can make a simpler test, the test can't control
>> the si_addr value easily, which I think is interesting to be tested.
>>
>> Why do you prefer SIGABRT?
> 
> I missed that you were testing si_addr -- in which case SIGSEGV is a good match.
> 
> 
>>> A test using setitimer to raise SIGALRM would test the async path.
>>
>> SIGLARM doesn't generate any interesting siginfo?
> 
> It should at minimum have si_sig = SIGALRM.
> 
>>
>> gromero@arm64:~$ gdb -q ./sigalrm
>> Reading symbols from ./sigalrm...
>> (gdb) run
>> Starting program: /home/gromero/sigalrm
>>
>> Program terminated with signal SIGALRM, Alarm clock.
>> The program no longer exists.
>> (gdb) p $_siginfo
>> $1 = void
> 
> Well that's because the program died.
> Do you need to have gdb handle the signal?

ouch, right :)

However, on a remote target, even if I catch that signal using
'catch signal SIGALRM' the GDBstub only closes the connection
when SIGALRM is delivered. That's odd, I don't understand why.

I'm using the same binary that pretty much works on GDB locally.


[Remote target]

gromero@arm64:~$ gdb -q
gromero@arm64:~/qemu_tests$ gdb -q ./sigalrm
Reading symbols from ./sigalrm...
(gdb) catch signal SIGALRM
Catchpoint 1 (signal SIGALRM)
(gdb) c
The program is not being run.
(gdb) run
Starting program: /home/gromero/qemu_tests/sigalrm
[Inferior 1 (process 12732) exited normally]
(gdb) quit

on the QEMU gdbstub side it reports "Alarm clock":

gromero@amd:~/git/qemu/build$ ./qemu-aarch64 -g 1234 ./sigalrm -s
Alarm clock
gromero@amd:~/git/qemu/build$


[Locally]

gromero@arm64:~/qemu_tests$ gdb -q ./sigalrm
Reading symbols from ./sigalrm...
(gdb) catch signal SIGALRM
Catchpoint 1 (signal SIGALRM)
(gdb) run -s
Starting program: /home/gromero/qemu_tests/sigalrm -s

Catchpoint 1 (signal SIGALRM), 0x000000000041a410 in ualarm ()
(gdb) quit


I'd like to add for the async path using SIGALRM but I need more
time to understand what's going on regarding SIGLARM. I understand
that's nothing wrong with the Xfer:siginfo:read stub itself, and
because the main goal of the test is to test the stub, if you don't
mind, I'd like to keep only the test with SIGSEGV for v2 and leave
the async test as a follow-up.


Cheers,
Gustavo

Re: [PATCH 1/2] gdbstub: Add Xfer:siginfo:read stub

[Gustavo Romero]

On 3/4/24 2:18 PM, Richard Henderson wrote:
> On 3/3/24 09:26, Gustavo Romero wrote:
>> +    /* Filter out si_type from si_code. See comment in siginfo_noswap(). */ > +    tmp_siginfo = ts->sync_signal.info;
>> +    tmp_siginfo.si_code = sextract32(tmp_siginfo.si_code, 0, 16);
> 
> 
> This is incorrect, as it only handles synchronous signals.
> 
> In handle_pending_signal(), struct emulated_sigtable is passed, which has the correct siginfo (all of it, so no need for the adjustment).  I think you need to pass that in to gdb_handlesig so that a copy can be made for later xfer.

Thanks, I'm sending v2 that fixes it.


Cheers,
Gustavo

Re: [PATCH 2/2] tests/tcg: Add multiarch test for Xfer:siginfo:read stub

[Richard Henderson]

On 3/7/24 07:50, Gustavo Romero wrote:
> Hi Richard,
> 
> On 3/4/24 7:51 PM, Richard Henderson wrote:
>> On 3/4/24 10:59, Gustavo Romero wrote:
>>>> Perhaps just abort for SIGABRT instead?
>>>
>>> Although this can make a simpler test, the test can't control
>>> the si_addr value easily, which I think is interesting to be tested.
>>>
>>> Why do you prefer SIGABRT?
>>
>> I missed that you were testing si_addr -- in which case SIGSEGV is a good match.
>>
>>
>>>> A test using setitimer to raise SIGALRM would test the async path.
>>>
>>> SIGLARM doesn't generate any interesting siginfo?
>>
>> It should at minimum have si_sig = SIGALRM.
>>
>>>
>>> gromero@arm64:~$ gdb -q ./sigalrm
>>> Reading symbols from ./sigalrm...
>>> (gdb) run
>>> Starting program: /home/gromero/sigalrm
>>>
>>> Program terminated with signal SIGALRM, Alarm clock.
>>> The program no longer exists.
>>> (gdb) p $_siginfo
>>> $1 = void
>>
>> Well that's because the program died.
>> Do you need to have gdb handle the signal?
> 
> ouch, right :)
> 
> However, on a remote target, even if I catch that signal using
> 'catch signal SIGALRM' the GDBstub only closes the connection
> when SIGALRM is delivered. That's odd, I don't understand why.
> 
> I'm using the same binary that pretty much works on GDB locally.
> 
> 
> [Remote target]
> 
> gromero@arm64:~$ gdb -q
> gromero@arm64:~/qemu_tests$ gdb -q ./sigalrm
> Reading symbols from ./sigalrm...
> (gdb) catch signal SIGALRM
> Catchpoint 1 (signal SIGALRM)
> (gdb) c
> The program is not being run.
> (gdb) run
> Starting program: /home/gromero/qemu_tests/sigalrm
> [Inferior 1 (process 12732) exited normally]
> (gdb) quit
> 
> on the QEMU gdbstub side it reports "Alarm clock":
> 
> gromero@amd:~/git/qemu/build$ ./qemu-aarch64 -g 1234 ./sigalrm -s
> Alarm clock
> gromero@amd:~/git/qemu/build$
> 
> 
> [Locally]
> 
> gromero@arm64:~/qemu_tests$ gdb -q ./sigalrm
> Reading symbols from ./sigalrm...
> (gdb) catch signal SIGALRM
> Catchpoint 1 (signal SIGALRM)
> (gdb) run -s
> Starting program: /home/gromero/qemu_tests/sigalrm -s
> 
> Catchpoint 1 (signal SIGALRM), 0x000000000041a410 in ualarm ()
> (gdb) quit
> 
> 
> I'd like to add for the async path using SIGALRM but I need more
> time to understand what's going on regarding SIGLARM. I understand
> that's nothing wrong with the Xfer:siginfo:read stub itself, and
> because the main goal of the test is to test the stub, if you don't
> mind, I'd like to keep only the test with SIGSEGV for v2 and leave
> the async test as a follow-up.

Well that's certainly surprising.
Would you please file a bug report about this?
I think I know what the problem is, but let's track it anyway.


r~

Re: [PATCH 2/2] tests/tcg: Add multiarch test for Xfer:siginfo:read stub

[Gustavo Romero]

On 3/7/24 4:31 PM, Richard Henderson wrote:
> On 3/7/24 07:50, Gustavo Romero wrote:
>> Hi Richard,
>>
>> On 3/4/24 7:51 PM, Richard Henderson wrote:
>>> On 3/4/24 10:59, Gustavo Romero wrote:
>>>>> Perhaps just abort for SIGABRT instead?
>>>>
>>>> Although this can make a simpler test, the test can't control
>>>> the si_addr value easily, which I think is interesting to be tested.
>>>>
>>>> Why do you prefer SIGABRT?
>>>
>>> I missed that you were testing si_addr -- in which case SIGSEGV is a good match.
>>>
>>>
>>>>> A test using setitimer to raise SIGALRM would test the async path.
>>>>
>>>> SIGLARM doesn't generate any interesting siginfo?
>>>
>>> It should at minimum have si_sig = SIGALRM.
>>>
>>>>
>>>> gromero@arm64:~$ gdb -q ./sigalrm
>>>> Reading symbols from ./sigalrm...
>>>> (gdb) run
>>>> Starting program: /home/gromero/sigalrm
>>>>
>>>> Program terminated with signal SIGALRM, Alarm clock.
>>>> The program no longer exists.
>>>> (gdb) p $_siginfo
>>>> $1 = void
>>>
>>> Well that's because the program died.
>>> Do you need to have gdb handle the signal?
>>
>> ouch, right :)
>>
>> However, on a remote target, even if I catch that signal using
>> 'catch signal SIGALRM' the GDBstub only closes the connection
>> when SIGALRM is delivered. That's odd, I don't understand why.
>>
>> I'm using the same binary that pretty much works on GDB locally.
>>
>>
>> [Remote target]
>>
>> gromero@arm64:~$ gdb -q
>> gromero@arm64:~/qemu_tests$ gdb -q ./sigalrm
>> Reading symbols from ./sigalrm...
>> (gdb) catch signal SIGALRM
>> Catchpoint 1 (signal SIGALRM)
>> (gdb) c
>> The program is not being run.
>> (gdb) run
>> Starting program: /home/gromero/qemu_tests/sigalrm
>> [Inferior 1 (process 12732) exited normally]
>> (gdb) quit
>>
>> on the QEMU gdbstub side it reports "Alarm clock":
>>
>> gromero@amd:~/git/qemu/build$ ./qemu-aarch64 -g 1234 ./sigalrm -s
>> Alarm clock
>> gromero@amd:~/git/qemu/build$
>>
>>
>> [Locally]
>>
>> gromero@arm64:~/qemu_tests$ gdb -q ./sigalrm
>> Reading symbols from ./sigalrm...
>> (gdb) catch signal SIGALRM
>> Catchpoint 1 (signal SIGALRM)
>> (gdb) run -s
>> Starting program: /home/gromero/qemu_tests/sigalrm -s
>>
>> Catchpoint 1 (signal SIGALRM), 0x000000000041a410 in ualarm ()
>> (gdb) quit
>>
>>
>> I'd like to add for the async path using SIGALRM but I need more
>> time to understand what's going on regarding SIGLARM. I understand
>> that's nothing wrong with the Xfer:siginfo:read stub itself, and
>> because the main goal of the test is to test the stub, if you don't
>> mind, I'd like to keep only the test with SIGSEGV for v2 and leave
>> the async test as a follow-up.
> 
> Well that's certainly surprising.
> Would you please file a bug report about this?
> I think I know what the problem is, but let's track it anyway.

Yeah.. I filed an issue here:

https://gitlab.com/qemu-project/qemu/-/issues/2214


Cheers,
Gustavo