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

[Walfred Tedeschi <walfred dot tedeschi at intel dot com>]

Am 4/13/2016 um 2:05 PM schrieb Pedro Alves:
> 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)

Ok, you are right. Intention was to present the full picture. But then 
this is lost in the commit itself.

> > Usage of both series of target descriptor (avx-mpx and mpx) were also fixed.
>
> I don't understand what this means.
It is that the switch and the flags used to select the right descriptor 
was also adapted.


> > +++ 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.)
Will address that.

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

Yes, i have to improve the comment for the XML files. Thanks!

> > +
> > +<!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.  */
>
> ?

It is odd. I will take a look in fact it's MPX will not be available for 
x32.

> Is it really true that x32 does not support AVX?
>
> Thanks,
> Pedro Alves

Thanks again!

-Fred
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928