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

On 24/07/2026 10:18, Matthieu Longo wrote:
> 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" ?
> 

That reads fine to me.

>> 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 ?
> 

I meant when users go and try to print this struct by hand. The other 
cases where gdb uses these we don´t care too much about the length of 
the names, right?

> 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.
Just a suggestion. If it makes it easier this could be a unit test to 
make sure we have the proper fields at least. But checking runtime 
values would need something more involved.