Re: [PATCH V3 3/6] Use linux_get_siginfo_type_with_fields for x86.

[Pedro Alves <palves at redhat dot com>]

On 01/18/2016 08:25 AM, Walfred Tedeschi wrote:
> Use linux_get_siginfo_type_with_fields for adding bound fields on
> segmentation fault for i386/amd64 siginfo.
> 
> 
> 2016-01-15  Walfred Tedeschi  <walfred.tedeschi@intel.com>
> 
> gdb/ChangeLog:
> 
> 	* linux-tdep.h (linux_get_siginfo_type_with_fields): Make
> 	function linux_get_siginfo_type_with_fields public.

As mentioned in the v1 review, it's linkage you're changing, not
class visibility.  Drop redundant name reference.  Thus:

 	* linux-tdep.h (linux_get_siginfo_type_with_fields): Make
 	extern.


> 	* linux-tdep.c (linux_get_siginfo_type_with_fields): Make
> 	function linux_get_siginfo_type_with_fields public.

Likewise.

> 	* i386-linux-tdep.h (x86_get_siginfo_type_with_fields): New
> 	function.
> 	* amd64-linux-tdep.c (amd64_linux_init_abi_common): Add
> 	x86_get_siginfo_type_with_fields for the amd64 abi.
> 	* i386-linux-tdep.c (x86_get_siginfo_type_with_fields): New
> 	Function.
> 	(i386_linux_init_abi): Add new function at the i386 ABI
> 	initialization.
> 
> ---
>  gdb/amd64-linux-tdep.c | 2 ++
>  gdb/i386-linux-tdep.c  | 8 ++++++++
>  gdb/i386-linux-tdep.h  | 3 +++
>  gdb/linux-tdep.c       | 2 +-
>  gdb/linux-tdep.h       | 5 +++++
>  5 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
> index b948ea7..55aad81 100644
> --- a/gdb/amd64-linux-tdep.c
> +++ b/gdb/amd64-linux-tdep.c
> @@ -1838,6 +1838,8 @@ amd64_linux_init_abi_common(struct gdbarch_info info, struct gdbarch *gdbarch)
>  
>    set_gdbarch_process_record (gdbarch, i386_process_record);
>    set_gdbarch_process_record_signal (gdbarch, amd64_linux_record_signal);
> +
> +  set_gdbarch_get_siginfo_type (gdbarch, x86_get_siginfo_type_with_fields);
>  }
>  
>  static void
> diff --git a/gdb/i386-linux-tdep.c b/gdb/i386-linux-tdep.c
> index 1e491e7..353688b 100644
> --- a/gdb/i386-linux-tdep.c
> +++ b/gdb/i386-linux-tdep.c
> @@ -656,6 +656,12 @@ i386_linux_supply_xstateregset (const struct regset *regset,
>    i387_supply_xsave (regcache, regnum, xstateregs);
>  }
>  
> +struct type *
> +x86_get_siginfo_type_with_fields (struct gdbarch *gdbarch)
> +{
> +  return linux_get_siginfo_type_with_fields (gdbarch, LINUX_SIGINFO_FIELD_ADDR_BND);
> +}
> +
>  /* Similar to i386_collect_fpregset, but use XSAVE extended state.  */
>  
>  static void
> @@ -994,6 +1000,8 @@ i386_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
>    set_xml_syscall_file_name (gdbarch, XML_SYSCALL_FILENAME_I386);
>    set_gdbarch_get_syscall_number (gdbarch,
>                                    i386_linux_get_syscall_number);
> +
> +  set_gdbarch_get_siginfo_type (gdbarch, x86_get_siginfo_type_with_fields);
>  }
>  
>  /* Provide a prototype to silence -Wmissing-prototypes.  */
> diff --git a/gdb/i386-linux-tdep.h b/gdb/i386-linux-tdep.h
> index ee6abff..f8e0074 100644
> --- a/gdb/i386-linux-tdep.h
> +++ b/gdb/i386-linux-tdep.h
> @@ -72,4 +72,7 @@ extern struct target_desc *tdesc_i386_avx512_linux;
>  
>  extern int i386_linux_gregset_reg_offset[];
>  
> +/* Returns x86 siginfo type with extra fields.  */
> +extern struct type *x86_get_siginfo_type_with_fields (struct gdbarch *gdbarch);

As I mentioned in the previous review, "with_fields" is implementation detail.
Callers of this function shouldn't have to care.  Can you rename this
to x86_linux_get_siginfo_type please?  (Note the "linux").


> +
>  #endif /* i386-linux-tdep.h */
> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> index 48bed11..a49057f 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -246,7 +246,7 @@ get_linux_inferior_data (void)
>  /* This function is suitable for architectures that
>     extend/override the standard siginfo in an specific way.  */
>  
> -static struct type *
> +struct type *
>  linux_get_siginfo_type_with_fields (struct gdbarch *gdbarch,
>  				    linux_siginfo_extra_fields extra_fields)
>  {
> diff --git a/gdb/linux-tdep.h b/gdb/linux-tdep.h
> index 98a17fa..fbd0817 100644
> --- a/gdb/linux-tdep.h
> +++ b/gdb/linux-tdep.h
> @@ -34,6 +34,11 @@ enum linux_siginfo_extra_field_values
>  /* Defines a type for the values defined in linux_siginfo_extra_field_values.  */
>  DEF_ENUM_FLAGS_TYPE (enum linux_siginfo_extra_field_values, linux_siginfo_extra_fields);
>  
> +/* Return the siginfo type with additional fields.  */
> +struct type *
> +linux_get_siginfo_type_with_fields (struct gdbarch *gdbarch,
> +				    linux_siginfo_extra_fields);

Function names in declarations shouldn't be at column 0.  Only in
definitions.  Add two leading spaces to the function name, or,
alternatively, break the line before the (, and then add two spaces
to the parameter list.

Thanks,
Pedro Alves