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

[Walfred Tedeschi <walfred.tedeschi@intel.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