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