Re: [PATCH v3] gdb: align siginfo_t with the Linux kernel definition

[Matthieu Longo <matthieu.longo@arm.com>]

On 23/07/2026 00:09, Luis wrote:
> Hi Matthieu,
> 
> On 22/07/2026 16:00, Matthieu Longo wrote:
>> On 21/07/2026 22:09, Luis wrote:
>>> On 02/07/2026 17:52, Matthieu Longo wrote:
>> --- a/gdb/linux-tdep.c
>> +++ b/gdb/linux-tdep.c
>>>> +  append_composite_type_field (sigfault_union_type, "si_addr_lsb", short_type);
>>>> +
>>>> +  /* used when si_code=SEGV_BNDERR */
>>>> +  type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT);
>>>> +  append_composite_type_field (type, "_dummy_bnd", addr_bnd_pkey_padding_type);
>>>> +  append_composite_type_field (type, "si_lower", void_ptr_type);
>>>> +  append_composite_type_field (type, "si_upper", void_ptr_type);
>>>> +  append_composite_type_field (sigfault_union_type, "_addr_bnd", type);
>>>> +
>>>> +  /* used when si_code=SEGV_PKUERR */
>>>> +  type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT);
>>>> +  append_composite_type_field (type, "_dummy_pkey", addr_bnd_pkey_padding_type);
>>>> +  append_composite_type_field (type, "si_pkey", uint32_type);
>>>> +  append_composite_type_field (sigfault_union_type, "_addr_pkey", type);
>>>> +
>>>> +  /* used when si_code=TRAP_PERF */
>>>> +  type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT);
>>>> +  append_composite_type_field (type, "si_perf_data", unsigned_long_type);
>>>> +  append_composite_type_field (type, "si_perf_type", uint32_type);
>>>> +  append_composite_type_field (type, "si_perf_flags", uint32_type);
>>>> +  append_composite_type_field (sigfault_union_type, "_perf", type);
>>>> +
>>>> +  /* End _sigfault's anonymous union.  */
>>>> +
>>>> +  /* _sigfault is set by SIGILL, SIGFPE, SIGSEGV, SIGBUS, SIGTRAP, SIGEMT */
>>>>      type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT);
>>>>      append_composite_type_field (type, "si_addr", void_ptr_type);
>>>> +  /* Since there is no possibility to declare an anonymous union,
>>>> +     using '_' instead.  */
>>>> +  append_composite_type_field (type, "_", sigfault_union_type);
>>>
>>> Could we name this in a better way? Simply using _ is a bit strange.
>>>
>>
>> What about "_union" ?
>>
> 
> Naming is hard. Given it is an anonymous union, should it have something anonymous in the name?
> 

I understand your point, but the name is going to start getting very lengthy.
What about "_anon_union" ?

> If we're going to access this by hand, it might be worth having some easy to use too.
> 

What type of accessors do you have in mind ? Compile-time ones ? Or runtime ones at the destination
of the GDB users when accessing _siginfo ?

For the compile-time ones, I propose to add those defines:

diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
index f11eccc1bc1..1a03e6897d6 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -2689,7 +2689,7 @@ aarch64_linux_report_signal_info (struct gdbarch *gdbarch,
       si_errno = parse_and_eval_long ("$_siginfo.si_errno");

       fault_addr
-       = parse_and_eval_long ("$_siginfo._sifields._sigfault.si_addr");
+       = parse_and_eval_long ("$_siginfo."si_addr);
     }
   catch (const gdb_exception_error &exception)
     {
diff --git a/gdb/linux-tdep.h b/gdb/linux-tdep.h
index c19839fde2c..f739cc69111 100644
--- a/gdb/linux-tdep.h
+++ b/gdb/linux-tdep.h
@@ -98,4 +98,18 @@ extern CORE_ADDR linux_get_hwcap2 ();
 extern bool linux_address_in_shadow_stack_mem_range
   (CORE_ADDR addr, std::pair<CORE_ADDR, CORE_ADDR> *range);

+/* How the fields from siginfo_t's _sigfault can be accessed.  */
+#ifdef si_addr
+#error "Matthieu: this should not happen"
+#endif
+#define si_addr       "_sifields._sigfault.si_addr"
+#define si_trapno     "_sifields._sigfault._anon_union.si_trapno"
+#define si_addr_lsb   "_sifields._sigfault._anon_union.si_addr_lsb"
+#define si_lower      "_sifields._sigfault._anon_union._addr_bnd.si_lower"
+#define si_upper      "_sifields._sigfault._anon_union._addr_bnd.si_upper"
+#define si_pkey       "_sifields._sigfault._anon_union._addr_pkey.si_pkey"
+#define si_perf_data  "_sifields._sigfault._anon_union._perf.si_perf_data"
+#define si_perf_type  "_sifields._sigfault._anon_union._perf.si_perf_type"
+#define si_perf_flags "_sifields._sigfault._anon_union._perf.si_perf_flags"
+
 #endif /* GDB_LINUX_TDEP_H */
diff --git a/gdb/testsuite/gdb.base/siginfo-obj.exp b/gdb/testsuite/gdb.base/siginfo-obj.exp
index 6c43d30b7b8..5e36b334068 100644
--- a/gdb/testsuite/gdb.base/siginfo-obj.exp
+++ b/gdb/testsuite/gdb.base/siginfo-obj.exp
@@ -115,7 +115,7 @@ gdb_test "p \$_siginfo._sifields._sigfault.si_addr = 0x666" " = \\(void \\*\\) 0
 gdb_test "p \$_siginfo.si_errno = 666" " = 666"
 gdb_test "p \$_siginfo.si_code = 999" " = 999"
 gdb_test "p \$_siginfo.si_signo = 11" " = 11"
-gdb_test "p \$_siginfo._sifields._sigfault._union._addr_pkey.si_pkey = 123" " = 123"
+gdb_test "p \$_siginfo._sifields._sigfault._anon_union._addr_pkey.si_pkey = 123" " = 123"

 with_test_prefix "validate modified siginfo fields" {
     gdb_test "break $bp_location"
@@ -143,7 +143,7 @@ if {$gcore_created} {
     gdb_test "p \$_siginfo._sifields._sigfault.si_addr" \
        " = \\(void \\*\\) $ssi_addr" \
        "p \$_siginfo._sifields._sigfault.si_addr from core file"
-    gdb_test "p \$_siginfo._sifields._sigfault._union._addr_pkey.si_pkey" \
+    gdb_test "p \$_siginfo._sifields._sigfault._anon_union._addr_pkey.si_pkey" \
        " = $ssi_pkey" \
-       "p \$_siginfo._sifields._sigfault._union._addr_pkey.si_pkey from core file"
+       "p \$_siginfo._sifields._sigfault._anon_union._addr_pkey.si_pkey from core file"
 }

>>>>      append_composite_type_field (sifields_type, "_sigfault", type);
>>>>        /* _sigpoll */
>>>
>>> Do we need to add some extra tests to validate that gdb can read these new fields?
>>
>> Indeed, I can add some, but I would like to avoid adding a test for all the new fields.
>> Can we stick with the original purpose of this change, i.e. adding si_pkey ?
>>
> 
> Sure. I'm fine with having the proper coverage as a follow up patch. But it would be nice to at
> least have the coverage for what you plan to use, like si_pkey.
> 

The proper coverage is not easy. The test gdb/testsuite/gdb.base/siginfo-obj.exp relies on
definitions of those fields via the glibc header <signal.h>. It creates local variables that GDB can
print to test the values. Since the definitions of siginfo_t are not aligned, and some fields are
missing, I don't think there is an easy way to do a full coverage.

I propose to add a few fields from the anonymous union, like si_pkey, and the rest should implicitly
work if si_pkey works.

PS: I will publish a new revision once I get your confirmation about the naming, the compile-time
accessors and whether I need to implement runtime accessors.
For those last ones, please point me into the right direction for the implementation.

Matthieu