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

[Luis <luis.machado.foss@gmail.com>]

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:
>>> GDB's current definition of siginfo_t is missing many fields present in
>>> the Linux kernel definition [1].
>>>
>>> These fields are useful for providing detailed, user-friendly diagnostics
>>> when a fault occurs. Some new AArch64 extensions, such as Permission
>>> Overlay Enhancement used to implement Protection Keys [2], require the
>>> debugger to inspect 'si_pkey' alongside 'si_addr' to help the user identify
>>> the problematic key.
>>>
>>> This patch aligns GDB's definition of the __sifields._sigfault member of
>>> siginfo_t with the definition from the Linux kernel master branch.
>>>
>>> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
>>>        tree/include/uapi/asm-generic/siginfo.h#n69
>>> [2]: https://lore.kernel.org/all/20160212210213.ABC488FA@viggo.jf.intel.com/
>>>
>>> Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
>>> ---
>>>    gdb/linux-tdep.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
>>>    1 file changed, 44 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
>>> index a7381677498..d5b0e6e7011 100644
>>> --- a/gdb/linux-tdep.c
>>> +++ b/gdb/linux-tdep.c
>>> @@ -275,7 +275,7 @@ linux_get_siginfo_type (struct gdbarch *gdbarch)
>>>      struct type *void_ptr_type;
>>>      struct type *uid_type, *pid_type;
>>>      struct type *sigval_type, *clock_type;
>>> -  struct type *siginfo_type, *sifields_type;
>>> +  struct type *siginfo_type, *sifields_type, *sigfault_union_type;
>>>      struct type *type;
>>>        linux_gdbarch_data = get_linux_gdbarch_data (gdbarch);
>>> @@ -285,11 +285,21 @@ linux_get_siginfo_type (struct gdbarch *gdbarch)
>>>      type_allocator alloc (gdbarch);
>>>        const struct builtin_type *builtin_types = builtin_type (gdbarch);
>>> +  struct type *short_type = builtin_types->builtin_short;
>>>      struct type *int_type = builtin_types->builtin_int;
>>>      struct type *uint_type = builtin_types->builtin_unsigned_int;
>>>      struct type *long_type = builtin_types->builtin_long;
>>> +  struct type *unsigned_long_type = builtin_types->builtin_unsigned_long;
>>> +  struct type *uint32_type = builtin_types->builtin_uint32;
>>>        void_ptr_type = lookup_pointer_type (builtin_type (gdbarch)->builtin_void);
>>
>> Nit: Blank line here for cleanliness.
>>
> 
> Fixed.
> 
>>> +  /* Compute padding length, i.e. __ADDR_BND_PKEY_PAD.  */
>>> +  unsigned alignof_void_ptr = type_align (void_ptr_type);
>>> +  unsigned padding_size = (alignof_void_ptr < sizeof(short)
>>
>> Logic: Are we mixing host-side sizeof (short) with target-side type_align (void_ptr_type) here?
>>
> 
> Yes, you're right. Sorry, I missed that. The below should be better.
> 
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -295,8 +295,8 @@ linux_get_siginfo_type (struct gdbarch *gdbarch)
> 
>     /* Compute padding length, i.e. __ADDR_BND_PKEY_PAD.  */
>     unsigned alignof_void_ptr = type_align (void_ptr_type);
> -  unsigned padding_size = (alignof_void_ptr < sizeof(short)
> -                          ? sizeof(short)
> +  unsigned padding_size = (alignof_void_ptr < short_type->length ()
> +                          ? short_type->length ()
>                             : alignof_void_ptr);
>     struct type *addr_bnd_pkey_padding_type
>       = init_vector_type (builtin_types->builtin_uint8, padding_size);
> 
>> Formatting: Space between sizeof and parens. Multiple cases of this one.
>>
>>> +               ? sizeof(short)
>>> +               : alignof_void_ptr);
>>> +  struct type *addr_bnd_pkey_padding_type
>>> +    = init_vector_type (builtin_types->builtin_uint8, padding_size);
>>>        /* sival_t */
>>>      sigval_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_UNION);
>>> @@ -364,9 +374,41 @@ linux_get_siginfo_type (struct gdbarch *gdbarch)
>>>      append_composite_type_field (type, "si_stime", clock_type);
>>>      append_composite_type_field (sifields_type, "_sigchld", type);
>>>    -  /* _sigfault */
>>> +  /* Begin _sigfault's anonymous union.  */
>>> +  sigfault_union_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_UNION);
>>> +  /* used on alpha and sparc */
>>> +  append_composite_type_field (sigfault_union_type, "si_trapno", int_type);
>>> +  /* used when si_code=BUS_MCEERR_AR or used when si_code=BUS_MCEERR_AO */
>>
>> Nit: Odd phrase. Maybe rewrite as...
>>
>> "used when si_code is BUS_MCEERR_AR or BUS_MCEERR_AO"
>>
> 
> Fixed.
> 
>>> +  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?

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

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

> diff --git a/gdb/testsuite/gdb.base/siginfo-obj.c b/gdb/testsuite/gdb.base/siginfo-obj.c
> index 43dc979bc50..960e5b8e9cd 100644
> --- a/gdb/testsuite/gdb.base/siginfo-obj.c
> +++ b/gdb/testsuite/gdb.base/siginfo-obj.c
> @@ -35,6 +35,7 @@ handler (int sig, siginfo_t *info, void *context)
>     int ssi_signo = info->si_signo;
>     int ssi_code = info->si_code;
>     void *ssi_addr = info->si_addr;
> +  unsigned int ssi_pkey = info->si_pkey;
> 
>     _exit (0); /* set breakpoint here */
>   }
> diff --git a/gdb/testsuite/gdb.base/siginfo-obj.exp b/gdb/testsuite/gdb.base/siginfo-obj.exp
> index a94bf0e33ba..6c43d30b7b8 100644
> --- a/gdb/testsuite/gdb.base/siginfo-obj.exp
> +++ b/gdb/testsuite/gdb.base/siginfo-obj.exp
> @@ -78,6 +78,14 @@ gdb_test_multiple "p \$_siginfo" "$test" {
>       }
>   }
> 
> +set test "extract si_pkey"
> +gdb_test_multiple "p \$_siginfo" "$test" {
> +    -re "si_pkey = (\[0-9\]\+).*$gdb_prompt $" {
> +       set ssi_pkey $expect_out(1,string)
> +       pass "$test"
> +    }
> +}
> +
>   set bp_location [gdb_get_line_number "set breakpoint here"]
> 
>   with_test_prefix "validate siginfo fields" {
> @@ -87,6 +95,7 @@ with_test_prefix "validate siginfo fields" {
>       gdb_test "p ssi_errno" " = $ssi_errno"
>       gdb_test "p ssi_code" " = $ssi_code"
>       gdb_test "p ssi_signo" " = $ssi_signo"
> +    gdb_test "p ssi_pkey" " = $ssi_pkey"
>   }
> 
>   # Again, but this time, patch si_addr and check that the inferior sees
> @@ -106,6 +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"
> 
>   with_test_prefix "validate modified siginfo fields" {
>       gdb_test "break $bp_location"
> @@ -114,6 +124,7 @@ with_test_prefix "validate modified siginfo fields" {
>       gdb_test "p ssi_errno" " = 666"
>       gdb_test "p ssi_code" " = 999"
>       gdb_test "p ssi_signo" " = 11"
> +    gdb_test "p ssi_pkey" " = 123"
>   }
> 
>   # Test siginfo preservation in core files.
> @@ -132,4 +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" \
> +       " = $ssi_pkey" \
> +       "p \$_siginfo._sifields._sigfault._union._addr_pkey.si_pkey from core file"
>   }
> 
> 
> Matthieu