Re: [PATCH 3/4] Add target descriptor for avx-avx512.

[Pedro Alves <palves at redhat dot com>]

s/descriptor/description/g

On 05/13/2016 01:50 PM, Michael Sturm wrote:
> Add a dedicated target descriptor for the feature combination
> avx-avx512 as implemented by certain IA CPU models.
> 
> The corresponding X86_XSTATE_AVX_AVX512_MASK already exists, but shared
> the tdesc with X86_XSTATE_AVX_MPX_AVX512_MASK. This caused MPX registers
> displayed as undefined on CPUs that only implemented
> X86_XSTATE_AVX_AVX512_MASK, which is undesired. This patch solves this issue.
> 
> This patch also corrects the wrong usage of x32-avx-mpx-avx512, which is
> replaced by x32-avx-avx512. The MPX feature is not implemented in x32 mode.
> 
> gdb/Changelog:
> 2016-04-18  Michael Sturm  <michael.sturm@intel.com>
> 
>      * amd64-linux-tdep.c (features/i386/amd64-avx-avx512-linux.c):
>      New include.
>      (features/i386/x32-avx-mpx-avx512-linux.c): Renamed include.
>      (amd64_linux_core_read_description): Add dedicated cases for
>      X86_XSTATE_AVX_AVX512_MASK and return appropriate tdesc.
>      (_initialize_amd64_linux_tdep): Add calls to
>      initialize_tdesc_amd64_avx_avx512_linux () and
>      initialize_tdesc_x32_avx_avx512_linux ().

Don't add " ()" when you mention names of functions.  Here and
throughout (the series).

>      * amd64-linux.tdep.h (tdesc_amd64_avx_avx512_linux): New prototype.
>      (tdesc_x32_avx_mpx_avx512_linux): Renamed prototype.

Is that old or new name?  Renames should say both.  E.g., this
is a frequently used style:

      (foo): Rename to ... 
      (bar): ... this.

>      * features/i386/x32-avx-mpx-avx512-linux.c: Regenerated from renamed XML
>      file.

This is incorrect -- it must be that x32-avx-mpx-avx512-linux.c was deleted,
and features/i386/x32-avx-avx512-linux.c is a new file.

>      * features/i386/x32-avx-mpx-avx512-linux.xml: Renamed to
>      features/i386/x32-avx-avx512-linux.xml

Missing period.


>      * Makefile.in  (clean): Added handling new source files

                    ^^ spurious double space.

"handling of" ?  Or just say "Remove i386-avx-avx512.c, ..."

>      i386-avx-avx512.c, i386-avx-avx512-linux.c, amd64-avx-avx512.c,
>      amd64-avx-avx512-linux.c.

>      (initialize_low_tracepoint): Added init_registers_amd64_avx_avx512_linux.
>      * linux-i386-ipa.c (get_ipa_tdesc): Added dedicated case for
>      X86_TDESC_AVX_AVX512 and returne appropriate tdesc.

typo "returne".

The comments above apply to multiple places, but I just pointed out one
of each.

> diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
> index d70c4b0..7e54a8f 100644
> --- a/gdb/amd64-linux-tdep.c
> +++ b/gdb/amd64-linux-tdep.c
> @@ -44,11 +44,12 @@
>  #include "features/i386/amd64-avx-linux.c"
>  #include "features/i386/amd64-mpx-linux.c"
>  #include "features/i386/amd64-avx-mpx-linux.c"
> +#include "features/i386/amd64-avx-avx512-linux.c"
>  #include "features/i386/amd64-avx-mpx-avx512-linux.c"
>  
>  #include "features/i386/x32-linux.c"
>  #include "features/i386/x32-avx-linux.c"
> -#include "features/i386/x32-avx-mpx-avx512-linux.c"
> +#include "features/i386/x32-avx-avx512-linux.c"
>  
>  /* The syscall's XML filename for i386.  */
>  #define XML_SYSCALL_FILENAME_AMD64 "syscalls/amd64-linux.xml"
> @@ -1581,11 +1582,16 @@ amd64_linux_core_read_description (struct gdbarch *gdbarch,
>    switch (xcr0 & X86_XSTATE_ALL_MASK)
>      {
>      case X86_XSTATE_AVX_MPX_AVX512_MASK:
> -    case X86_XSTATE_AVX_AVX512_MASK:
>        if (gdbarch_ptr_bit (gdbarch) == 32)
> -	return tdesc_x32_avx_mpx_avx512_linux;
> +	/* No x32 MPX falling back to AVX-AVX512.  */

I know this string appears in other places already, but
I'd like to point out that it sounds quite odd to me.

I suggest saying what appears on the gdbserver hunk:

       /* No MPX on x32, fallback to AVX-AVX512.  */

> diff --git a/gdb/features/i386/x32-avx-avx512-linux.xml b/gdb/features/i386/x32-avx-avx512-linux.xml
> new file mode 100644
> index 0000000..36bcbb7
> --- /dev/null
> +++ b/gdb/features/i386/x32-avx-avx512-linux.xml
> @@ -0,0 +1,19 @@
> +<?xml version="1.0"?>
> +<!-- Copyright (C) 2014-2016 Free Software Foundation, Inc.
> +
> +     Copying and distribution of this file, with or without modification,
> +     are permitted in any medium without royalty provided the copyright
> +     notice and this notice are preserved.  -->
> +
> +<!-- X32 with AVX, MPX, AVX512 - Includes Linux-only special "register".  -->

MPX shouldn't be here, I believe.


> diff --git a/gdb/features/i386/x32-avx-avx512.xml b/gdb/features/i386/x32-avx-avx512.xml
> new file mode 100644
> index 0000000..9e17826
> --- /dev/null
> +++ b/gdb/features/i386/x32-avx-avx512.xml
> @@ -0,0 +1,17 @@
> +<?xml version="1.0"?>
> +<!-- Copyright (C) 2014-2016 Free Software Foundation, Inc.
> +
> +     Copying and distribution of this file, with or without modification,
> +     are permitted in any medium without royalty provided the copyright
> +     notice and this notice are preserved.  -->
> +
> +<!-- X32 with AVX, MPX, AVX512 -->

Ditto.

Thanks,
Pedro Alves