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

On 8/13/26 10:48 AM, Matthieu Longo wrote:
> 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",

Ok, my thinking was to also get rid of `_sifields`, `_sigfault`,
`_addr_bnd`, `_addr_pkey` and `_perf` (and maybe others), so that a user
would access the fields like:

  $_siginfo.si_perf_flags

just like you access it in the source code (through the macro "magic"):

  siginfo_obj->si_perf_flags

But I had not realized that _sifields and _siginfo is pre-existing, so
we can't simply change it, that would be a backwards incompatible
change.

So for now, for consistency, it makes sense to continue in the existing
direction, with the named fields.  Removing `_anon_union` is good
though.

>      };
>      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" \

That looks consistent with what currently exists, so that's good.

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

Agreed, that documentation change would be welcome.

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

It's not the name of the type that matters, it's the name of the field,
when added with append_composite_type_field, that does.  When a struct
or union field has no name, it is anonymous, and its fields are visible
directly in the parent scope.

I mean, it's true that passing no name creates an anonymous type, but
that's not what matters in the problem at hand.  Also, it should be
clearer: "no name" means empty string or nullptr?

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

As I said above, we can't just rename the existing fields, there is
probably code out there relying on those names.

We could add new fields, so the structure would just contain both the
named hierarchy and the anonymous hierarchy, the fields would just be
duplicated.  That's fine if the structure is read-only, but if for some
reason some user code needs to change a field (I don't really know why
it would do that), then that would be awkward, since it would only
update one copy.

Anyway, that's for later (if ever).

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

Hmm, I don't think we need to touch this nat code.  I am only talking
about the type of the $_siginfo convenience variable, which we build
manually with those arch_composite_type & co calls.

Simon