Re: [PATCH V2 1/2] Add redundant target descriptor for tdesc(amd64|i386)_avx_mpx_*

[Pedro Alves <palves@redhat.com>]

On 03/03/2016 05:25 PM, Walfred Tedeschi wrote:
> Add a redundant target description for the MPX and AVX case using a
> combined feature name to reflect that, i.e. avx-mpx.

It's better when commit logs are self-contained and don't
depend on info in the cover letter.  The cover letter doesn't
make it to git.

E.g., 

~~
Subject: Add target descriptions for AVX + MPX

The current MPX target descriptions assume that MPX is always
combined with AVX, however that's not correct.  We can have
machines with MPX and without AVX; or machines with AVX
and without MPX.

This patch adds new target descriptions for machines that
support both MPX and AVX, as duplicates of the existing
MPX descriptions.  The following commit will remove AVX from
the MPX-only descriptions.
~~

(Note s/descriptors/descriptions in subject)

> 
> Usage of both series of target descriptor (avx-mpx and mpx) were also fixed.

I don't understand what this means.

> +++ b/gdb/features/i386/i386-avx-mpx-linux.xml
> @@ -0,0 +1,19 @@
> +<?xml version="1.0"?>
> +<!-- Copyright (C) 2013-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.  -->
> +
> +<!-- I386 with MPX- Includes Linux-only special "register".  -->

Missing space before "-".  (Please audit the other added files.)

> +++ b/gdb/features/i386/i386-avx-mpx.xml
> @@ -0,0 +1,17 @@
> +<?xml version="1.0"?>
> +<!-- Copyright (C) 2013-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.  -->
> +
> +<!-- I386 with MPX -->

Shouldn't this say "and AVX" as well?


> +
> +<!DOCTYPE target SYSTEM "gdb-target.dtd">
> +<target>
> +  <architecture>i386</architecture>
> +  <xi:include href="32bit-core.xml"/>
> +  <xi:include href="32bit-sse.xml"/>
> +  <xi:include href="32bit-avx.xml"/>
> +  <xi:include href="32bit-mpx.xml"/>
> +</target>
> diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in

> +	case X86_XSTATE_AVX_MPX_MASK:
> +	  if (is_x32)
> +	    return tdesc_x32_linux; /* No AVX and MPX is not available in x32.  */


The comment sounds odd to me.  Does this intend to say:

  /* Neither AVX nor MPX are available on x32.  */

?

Is it really true that x32 does not support AVX?

Thanks,
Pedro Alves