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

On 13/08/2026 16:49, Simon Marchi wrote:
> On 8/13/26 10:48 AM, Matthieu Longo wrote:
>> 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 moved this to another patch since it is not directly related to this patch.
https://inbox.sourceware.org/gdb-patches/[email protected]/

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

So, if I understood you well, you don't want to touch the current definition in
linux_get_siginfo_type(). Instead, you propose to define a new siginfo type as the data structure
below. Then, siginfo data should be cast to the new user-facing type before being returned.
Is this correct ?


#define __ARCH_SI_CLOCK_T unsigned long
#define __ADDR_BND_PKEY_PAD  (__alignof__(void *) < sizeof(short) ? \
			      sizeof(short) : __alignof__(void *))

struct siginfo {
  int si_signo;
  int si_errno;
  int si_code;

  /* Beginning of __sifields.  */
  union {

    /* _kill, signals, _sigchld and _timer are tangled, so should be flattened
       together.  */
    struct {
      union {
        int si_pid; // _kill, _rt, _sigchld
        int si_tid; // _timer
      };
      union {
        uint32_t si_uid; // _kill, _rt, _sigchld
        int si_overrun; // _timer
      };
      union {
        struct {
          int si_status;
          __ARCH_SI_CLOCK_T si_utime;
          __ARCH_SI_CLOCK_T si_stime;
        }; // _sigchld

        struct {
          union {
            int si_int;
            void *si_ptr;
          } si_value; // _rt, _timer
          int si_sys_private; // _timer
        };
      };
    };

    /* _sigfault, _sigpoll and _sigsys are not sharing anything, so are
       flattened on their own.  */

    struct {
      void *si_addr;
      union {
        int si_trapno;
        short si_addr_lsb;
        struct {
          char _dummy_padding_1[__ADDR_BND_PKEY_PAD];
          void *si_lower;
          void *si_upper;
        }; /* _addr_bnd */
        struct {
          char _dummy_padding_2[__ADDR_BND_PKEY_PAD];
          uint32_t si_pkey;
        }; /* _addr_pkey */
        struct {
          unsigned long si_perf_data;
          uint32_t si_perf_type;
          uint32_t si_perf_flags;
        }; /* _perf */
      };
    }; /* _sigfault */

    struct {
      long si_band;
      int si_fd;
    }; /* _sigpoll */

    struct {
      void *si_call_addr;
      int si_syscall;
      unsigned int si_arch;
    }; /* _sigsys */

  }; /* End of __sifields.  */
};

Matthieu