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

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

Hello Matthieu,

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.
>
> [1]: https://github.com/torvalds/linux/blob/2b414a95b8f7307d42173ba9e580d6
>      d3e2bcbfce/include/uapi/asm-generic/siginfo.h#L69-L100

Nit: https://github.com/torvalds/linux/ is a mirror. It's better to use
the official address at
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git

> [2]: https://lore.kernel.org/all/20160212210213.ABC488FA@viggo.jf.intel.com/
> ---
>  gdb/linux-tdep.c | 39 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> index a7381677498..5baad41a083 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -275,7 +275,7 @@ linux_get_siginfo_type (struct gdbarch *gdbarch)
>    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,9 +285,12 @@ 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;
> +  struct type *unsigned_long_type = builtin_types->builtin_unsigned_long;
> +  struct type *uint32_type = builtin_types->builtin_uint32;
>  
>    void_ptr_type = lookup_pointer_type (builtin_type (gdbarch)->builtin_void);
>  
> @@ -364,9 +367,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=BUS_MCEERR_AR or used when si_code=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", short_type);

The type of _dummy_bnd isn't that important in practice because the
field is just for padding, but its size is wrong AFAICS:

$ cat siginfo_t.c
#include <stdio.h>
#include <asm/siginfo.h>

int main (int argc, char *argv[]) {
    siginfo_t siginfo;

    printf ("sizeof dummy_bnd = %lu\n",
            sizeof(siginfo._sifields._sigfault._addr_bnd._dummy_bnd));

    return 0;
}
$ make siginfo_t
cc     siginfo_t.c   -o siginfo_t
$ ./siginfo_t 
sizeof dummy_bnd = 8

> +  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", short_type);

This one also has the wrong size, because <asm/siginfo.h> uses the same
__ADDR_BND_PKEY_PAD constant for the size of _dummy_bnd and of
_dummy_pkey.

-- 
Thiago
(he/him)