Re: [PATCH v5] gdb: align siginfo_t with the Linux kernel definition
Matthieu Longo <[email protected]>
| Newsgroups | gmane.comp.gdb.patches |
|---|---|
| Message-ID | <[email protected]> |
On 13/08/2026 15:48, Matthieu Longo wrote: > On 12/08/2026 20:08, Simon Marchi wrote: >> On 7/28/26 8:32 AM, 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. >>> >>> To avoid hardcoding the field access paths throughout the codebase, this >>> patch also introduces compile-time accessors for the siginfo_t attributes, >>> centralizing their definitions in a single location and making future >>> updates easier. >>> >>> Finally, extend the testsuite to verify access to the new si_pkey field >>> and its preservation when modifying $_siginfo and when reading core files. >>> The tests in siginfo-obj.exp rely on the siginfo_t definition provided by >>> glibc's <signal.h>, which does not yet expose all of the fields present in >>> the kernel definition. As a result, the tests cannot exercise every newly >>> added field and therefore focus on si_pkey, the field motivating this change. >>> The test validates that GDB can read and modify the field correctly; it does >>> not attempt to generate a real protection-key fault. >>> >>> [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/[email protected]/ >>> >>> Reviewed-by: Thiago Jung Bauermann <[email protected]> >>> --- >>> gdb/aarch64-linux-tdep.c | 8 ++-- >>> gdb/linux-tdep.c | 52 +++++++++++++++++++--- >>> gdb/linux-tdep.h | 60 ++++++++++++++++++++++++++ >>> gdb/sparc64-linux-tdep.c | 6 ++- >>> gdb/testsuite/gdb.base/siginfo-obj.c | 1 + >>> gdb/testsuite/gdb.base/siginfo-obj.exp | 14 ++++++ >>> 6 files changed, 131 insertions(+), 10 deletions(-) >>> >>> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c >>> index f11eccc1bc1..235b35bcfb4 100644 >>> --- a/gdb/aarch64-linux-tdep.c >>> +++ b/gdb/aarch64-linux-tdep.c >>> @@ -2683,13 +2683,15 @@ aarch64_linux_report_signal_info (struct gdbarch *gdbarch, >>> >>> try >>> { >>> + using gdb_si = gdb::siginfo_type; >>> + using si_key = gdb::siginfo_type::key; >>> /* Sigcode tells us if the segfault is actually a memory tag >>> violation. */ >>> - si_code = parse_and_eval_long ("$_siginfo.si_code"); >>> - si_errno = parse_and_eval_long ("$_siginfo.si_errno"); >>> + si_code = parse_and_eval_long (gdb_si::get (si_key::siginfo_code)); >>> + si_errno = parse_and_eval_long (gdb_si::get (si_key::siginfo_errno)); >>> >>> fault_addr >>> - = parse_and_eval_long ("$_siginfo._sifields._sigfault.si_addr"); >>> + = parse_and_eval_long (gdb_si::get (si_key::siginfo_addr)); >>> } >>> catch (const gdb_exception_error &exception) >>> { >>> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c >>> index 25d625db595..740043a9292 100644 >>> --- a/gdb/linux-tdep.c >>> +++ b/gdb/linux-tdep.c >>> @@ -272,10 +272,9 @@ static struct type * >>> linux_get_siginfo_type (struct gdbarch *gdbarch) >>> { >>> struct linux_gdbarch_data *linux_gdbarch_data; >>> - 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 +284,22 @@ 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; >>> - >>> - void_ptr_type = lookup_pointer_type (builtin_type (gdbarch)->builtin_void); >>> + struct type *unsigned_long_type = builtin_types->builtin_unsigned_long; >>> + struct type *uint32_type = builtin_types->builtin_uint32; >>> + struct type *void_ptr_type >>> + = lookup_pointer_type (builtin_type (gdbarch)->builtin_void); >>> + >>> + /* Compute padding length, i.e. __ADDR_BND_PKEY_PAD. */ >>> + unsigned alignof_void_ptr = type_align (void_ptr_type); >>> + 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); >>> >>> /* 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 is BUS_MCEERR_AR or BUS_MCEERR_AO. */ >>> + 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 '_anon_union' instead. */ >>> + append_composite_type_field (type, "_anon_union", sigfault_union_type); > > I set the name to "", and it works as you mentioned below. > I also fixed the usages in gdb/testsuite/gdb.base/siginfo-obj.exp > > diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c > index 4660772752d..bbc4009d43c 100644 > --- a/gdb/linux-tdep.c > +++ b/gdb/linux-tdep.c > @@ -406,9 +406,8 @@ linux_get_siginfo_type (struct gdbarch *gdbarch) > /* _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 '_anon_union' instead. */ > - append_composite_type_field (type, "_anon_union", sigfault_union_type); > + /* Note: this is an anonymous union. */ > + append_composite_type_field (type, "", sigfault_union_type); > append_composite_type_field (sifields_type, "_sigfault", type); > > /* _sigpoll */ > diff --git a/gdb/linux-tdep.h b/gdb/linux-tdep.h > index 43ed38c6633..1f40756eb2f 100644 > --- a/gdb/linux-tdep.h > +++ b/gdb/linux-tdep.h > @@ -143,14 +143,14 @@ struct siginfo_type > > /* SIGILL, SIGFPE, SIGSEGV, SIGBUS, SIGTRAP, SIGEMT */ > "$_siginfo._sifields._sigfault.si_addr", > - "$_siginfo._sifields._sigfault._anon_union.si_trapno", > - "$_siginfo._sifields._sigfault._anon_union.si_addr_lsb", > - "$_siginfo._sifields._sigfault._anon_union._addr_bnd.si_lower", > - "$_siginfo._sifields._sigfault._anon_union._addr_bnd.si_upper", > - "$_siginfo._sifields._sigfault._anon_union._addr_pkey.si_pkey", > - "$_siginfo._sifields._sigfault._anon_union._perf.si_perf_data", > - "$_siginfo._sifields._sigfault._anon_union._perf.si_perf_type", > - "$_siginfo._sifields._sigfault._anon_union._perf.si_perf_flags", > + "$_siginfo._sifields._sigfault.si_trapno", > + "$_siginfo._sifields._sigfault.si_addr_lsb", > + "$_siginfo._sifields._sigfault._addr_bnd.si_lower", > + "$_siginfo._sifields._sigfault._addr_bnd.si_upper", > + "$_siginfo._sifields._sigfault._addr_pkey.si_pkey", > + "$_siginfo._sifields._sigfault._perf.si_perf_data", > + "$_siginfo._sifields._sigfault._perf.si_perf_type", > + "$_siginfo._sifields._sigfault._perf.si_perf_flags", > }; > return paths[static_cast<size_t> (attr_)]; > } > diff --git a/gdb/testsuite/gdb.base/siginfo-obj.exp b/gdb/testsuite/gdb.base/siginfo-obj.exp > index 5e36b334068..a724a634dc0 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._anon_union._addr_pkey.si_pkey = 123" " = 123" > +gdb_test "p \$_siginfo._sifields._sigfault._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._anon_union._addr_pkey.si_pkey" \ > + gdb_test "p \$_siginfo._sifields._sigfault._addr_pkey.si_pkey" \ > " = $ssi_pkey" \ > - "p \$_siginfo._sifields._sigfault._anon_union._addr_pkey.si_pkey from core file" > + "p \$_siginfo._sifields._sigfault._addr_pkey.si_pkey from core file" > } > > >>> Can you expand on why it's not possible to have an anonymous union? It >> is certainly possible to have anonymous unions described in DWARF, which >> are then translated to struct types. >> > > I simply could not find how to implement it. > It might be useful to add to the documentation of append_composite_type_field_aligned() or maybe > better, to the comment in gdb/gdbtypes.h above the declaration of append_composite_type_field(), a > sentence explaining what is the effect of providing an empty name. > > diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h > index dd2d24fa8e2..e4fedd886c4 100644 > --- a/gdb/gdbtypes.h > +++ b/gdb/gdbtypes.h > @@ -2431,11 +2431,12 @@ extern struct type *init_pointer_type (type_allocator &alloc, int bit, > extern struct type *init_fixed_point_type (type_allocator &, int, int, > const char *); > > -/* Helper functions to construct a struct or record type. An > - initially empty type is created using arch_composite_type(). > - Fields are then added using append_composite_type_field*(). A union > - type has its size set to the largest field. A struct type has each > - field packed against the previous. */ > +/* Helper functions to construct a struct or record type. An initially empty > + type is created using arch_composite_type(). Fields are then added using > + append_composite_type_field*(). > + A union type has its size set to the largest field. A struct type has each > + field packed against the previous. > + If no name is specified, the type is anonymous. */ > > extern struct type *arch_composite_type (struct gdbarch *gdbarch, > const char *name, enum type_code code); > >> I think that the ideal user experience would be for users to be able to >> access fields the same way that they do in the code, that is >> `si.si_pkey`. All the _sigfault/_addr_pkey/etc parts are implementation >> details that could change. >> >> On top of your patch, if I just delete all the internal field names, it >> seems to work just fine, see patch below. In the end it models >> something like this in C: >> > > I am not against it. > However, could this suggestion be addressed in a different patch ? > > Simplifying the existing pathes to si_* values with anonymous structs would increase the impact of > the original patch, with potentially additional testing and carefulness required for others > architectures (for example, see gdb/nat/amd64-linux-siginfo.c L269). > > Matthieu > >> struct siginto_t >> { >> union >> { >> // kill >> struct >> { >> int si_pid; >> int si_uid; >> }; >> >> // timer >> struct >> { >> int si_tid; >> int si_overrun; >> int si_sys_private; >> }; >> >> ... >> }; >> }; >> >> >> diff --git i/gdb/linux-tdep.c w/gdb/linux-tdep.c >> index 4660772752d6..f21fa1e18578 100644 >> --- i/gdb/linux-tdep.c >> +++ w/gdb/linux-tdep.c >> @@ -349,21 +349,21 @@ linux_get_siginfo_type (struct gdbarch *gdbarch) >> type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT); >> append_composite_type_field (type, "si_pid", pid_type); >> append_composite_type_field (type, "si_uid", uid_type); >> - append_composite_type_field (sifields_type, "_kill", type); >> + append_composite_type_field (sifields_type, "", type); >> >> /* _timer */ >> type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT); >> append_composite_type_field (type, "si_tid", int_type); >> append_composite_type_field (type, "si_overrun", int_type); >> append_composite_type_field (type, "si_sigval", sigval_type); >> - append_composite_type_field (sifields_type, "_timer", type); >> + append_composite_type_field (sifields_type, "", type); >> >> /* _rt */ >> type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT); >> append_composite_type_field (type, "si_pid", pid_type); >> append_composite_type_field (type, "si_uid", uid_type); >> append_composite_type_field (type, "si_sigval", sigval_type); >> - append_composite_type_field (sifields_type, "_rt", type); >> + append_composite_type_field (sifields_type, "", type); >> >> /* _sigchld */ >> type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT); >> @@ -372,7 +372,7 @@ linux_get_siginfo_type (struct gdbarch *gdbarch) >> append_composite_type_field (type, "si_status", int_type); >> append_composite_type_field (type, "si_utime", clock_type); >> append_composite_type_field (type, "si_stime", clock_type); >> - append_composite_type_field (sifields_type, "_sigchld", type); >> + append_composite_type_field (sifields_type, "", type); >> >> /* Begin _sigfault's anonymous union. */ >> sigfault_union_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_UNION); >> @@ -386,20 +386,20 @@ linux_get_siginfo_type (struct gdbarch *gdbarch) >> 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); >> + append_composite_type_field (sigfault_union_type, "", 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); >> + append_composite_type_field (sigfault_union_type, "", 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); >> + append_composite_type_field (sigfault_union_type, "", type); >> >> /* End _sigfault's anonymous union. */ >> >> @@ -408,21 +408,21 @@ linux_get_siginfo_type (struct gdbarch *gdbarch) >> append_composite_type_field (type, "si_addr", void_ptr_type); >> /* Since there is no possibility to declare an anonymous union, >> using '_anon_union' instead. */ >> - append_composite_type_field (type, "_anon_union", sigfault_union_type); >> - append_composite_type_field (sifields_type, "_sigfault", type); >> + append_composite_type_field (type, "", sigfault_union_type); >> + append_composite_type_field (sifields_type, "", type); >> >> /* _sigpoll */ >> type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT); >> append_composite_type_field (type, "si_band", long_type); >> append_composite_type_field (type, "si_fd", int_type); >> - append_composite_type_field (sifields_type, "_sigpoll", type); >> + append_composite_type_field (sifields_type, "", type); >> >> /* _sigsys */ >> type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT); >> append_composite_type_field (type, "_call_addr", void_ptr_type); >> append_composite_type_field (type, "_syscall", int_type); >> append_composite_type_field (type, "_arch", uint_type); >> - append_composite_type_field (sifields_type, "_sigsys", type); >> + append_composite_type_field (sifields_type, "", type); >> >> /* struct siginfo */ >> siginfo_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT); >> @@ -431,7 +431,7 @@ linux_get_siginfo_type (struct gdbarch *gdbarch) >> append_composite_type_field (siginfo_type, "si_errno", int_type); >> append_composite_type_field (siginfo_type, "si_code", int_type); >> append_composite_type_field_aligned (siginfo_type, >> - "_sifields", sifields_type, >> + "", sifields_type, >> long_type->length ()); >> >> linux_gdbarch_data->siginfo_type = siginfo_type; >> diff --git i/gdb/testsuite/gdb.base/siginfo-obj.exp w/gdb/testsuite/gdb.base/siginfo-obj.exp >> index 5e36b3340680..272d74ac2805 100644 >> --- i/gdb/testsuite/gdb.base/siginfo-obj.exp >> +++ w/gdb/testsuite/gdb.base/siginfo-obj.exp >> @@ -111,11 +111,11 @@ gdb_test "continue" ".*Program received signal SIGSEGV.*" \ >> "continue to signal, 2nd" >> >> set test "set si_addr" >> -gdb_test "p \$_siginfo._sifields._sigfault.si_addr = 0x666" " = \\(void \\*\\) 0x666" >> +gdb_test "p \$_siginfo.si_addr = 0x666" " = \\(void \\*\\) 0x666" >> 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._anon_union._addr_pkey.si_pkey = 123" " = 123" >> +gdb_test "p \$_siginfo.si_pkey = 123" " = 123" >> >> with_test_prefix "validate modified siginfo fields" { >> gdb_test "break $bp_location" >> @@ -140,10 +140,10 @@ if {$gcore_created} { >> "p \$_siginfo.si_errno from core file" >> gdb_test "p \$_siginfo.si_code" " = $ssi_code" \ >> "p \$_siginfo.si_code from core file" >> - gdb_test "p \$_siginfo._sifields._sigfault.si_addr" \ >> + gdb_test "p \$_siginfo.si_addr" \ >> " = \\(void \\*\\) $ssi_addr" \ >> - "p \$_siginfo._sifields._sigfault.si_addr from core file" >> - gdb_test "p \$_siginfo._sifields._sigfault._anon_union._addr_pkey.si_pkey" \ >> + "p \$_siginfo.si_addr from core file" >> + gdb_test "p \$_siginfo.si_pkey" \ >> " = $ssi_pkey" \ >> - "p \$_siginfo._sifields._sigfault._anon_union._addr_pkey.si_pkey from core file" >> + "p \$_siginfo.si_pkey from core file" >> } >> >> Simon > Hi Simon, Do you want me to publish a new revision of the patch ? The things you raised looked minor to me, so I would assume that I don't need to publish a new revision. Please let me know if you think otherwise. Regards, Matthieu