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

[Andrew Burgess <aburgess@redhat.com>]

Matthieu Longo <matthieu.longo@arm.com> writes:

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

Hi Matthieu,

Please see this message:

  https://sourceware.org/pipermail/bunsen/2026q3/001504.html

This is an LLM generated analysis of this patch which makes the claim
that si_pkey is not accessible at the glibc level on every
architecture.

I tried on a couple of compiler farm boxes, but ran into some TCL
version issues so wasn't able to actually make-check, but on a ppc box,
if I try to compile gdb.base/siginfo-obj.c manually I get:

  $ gcc -o siginfo-obj siginfo-obj.c
  siginfo-obj.c: In function ‘handler’:
  siginfo-obj.c:38:31: error: ‘siginfo_t’ has no member named ‘si_pkey’
     unsigned int ssi_pkey = info->si_pkey;
                               ^

This test file did compile before commit 9c99987237dcaca0c6c493d6a.

It would be great if you could take a look at this.  Should the si_pkey
parts of the test be optional maybe?

Thanks,
Andrew


>
> [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/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);
>    append_composite_type_field (sifields_type, "_sigfault", type);
>  
>    /* _sigpoll */
> diff --git a/gdb/linux-tdep.h b/gdb/linux-tdep.h
> index c19839fde2c..43ed38c6633 100644
> --- a/gdb/linux-tdep.h
> +++ b/gdb/linux-tdep.h
> @@ -98,4 +98,64 @@ 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);
>  
> +namespace gdb {
> +
> +/* Maps each siginfo_type::key to the corresponding field-access expression
> +   in $_siginfo.
> +
> +   Keep the order of key values synchronized with the entries in get()'s
> +   paths array.  Each key is used directly as an array index.  */
> +
> +struct siginfo_type
> +{
> +  /* Identifies a field within siginfo_t that may be referenced by name.  */
> +  enum class key
> +  {
> +    siginfo_signo = 0,
> +    siginfo_errno,
> +    siginfo_code,
> +
> +    /* SIGILL, SIGFPE, SIGSEGV, SIGBUS, SIGTRAP, SIGEMT */
> +    siginfo_addr,
> +    siginfo_trapno,
> +    siginfo_addr_lsb,
> +    siginfo_lower,
> +    siginfo_upper,
> +    siginfo_pkey,
> +    siginfo_perf_data,
> +    siginfo_perf_type,
> +    siginfo_perf_flags,
> +
> +    /* Sentinel used to determine the number of mapped fields.  */
> +    SIGINFO_ATTR_END
> +  };
> +
> +  /* Return the $_siginfo access expression associated with ATTR_.
> +
> +     ATTR_ must be a valid si_* key other than SIGINFO_ATTR_END.  The array
> +     order must exactly match the declaration order of the keys above.  */
> +  static constexpr const char *get (key attr_)
> +  {
> +    const char *paths[static_cast<size_t> (key::SIGINFO_ATTR_END)] = {
> +      "$_siginfo.si_signo",
> +      "$_siginfo.si_errno",
> +      "$_siginfo.si_code",
> +
> +      /* 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",
> +    };
> +    return paths[static_cast<size_t> (attr_)];
> +  }
> +};
> +
> +} /* namespace gdb */
> +
>  #endif /* GDB_LINUX_TDEP_H */
> diff --git a/gdb/sparc64-linux-tdep.c b/gdb/sparc64-linux-tdep.c
> index cb7ce41e5cb..b6245f64bae 100644
> --- a/gdb/sparc64-linux-tdep.c
> +++ b/gdb/sparc64-linux-tdep.c
> @@ -134,11 +134,13 @@ sparc64_linux_report_signal_info (struct gdbarch *gdbarch, struct ui_out *uiout,
>  
>    try
>      {
> +      using gdb_si = gdb::siginfo_type;
> +      using si_key = gdb::siginfo_type::key;
>        /* Evaluate si_code to see if the segfault is ADI related.  */
> -      si_code = parse_and_eval_long ("$_siginfo.si_code\n");
> +      si_code = parse_and_eval_long (gdb_si::get (si_key::siginfo_code));
>  
>        if (si_code >= SEGV_ACCADI && si_code <= SEGV_ADIPERR)
> -	addr = parse_and_eval_long ("$_siginfo._sifields._sigfault.si_addr");
> +	addr = parse_and_eval_long (gdb_si::get (si_key::siginfo_addr));
>      }
>    catch (const gdb_exception_error &exception)
>      {
> 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..5e36b334068 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._anon_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._anon_union._addr_pkey.si_pkey" \
> +	" = $ssi_pkey" \
> +	"p \$_siginfo._sifields._sigfault._anon_union._addr_pkey.si_pkey from core file"
>  }
> -- 
> 2.55.0