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

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

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

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

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