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)
>
> > 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

Pedro and all,

Thanks again for your review!

V3 will come soon, however we have a finding that needs to be addressed!

The reading of registers and the definition for the target descriptions 
can trigger an assertion. I will address it on an additional patch.

Here we selected the target description according to the XCR0 bits. But 
we might select one target description that has the XCR0 bits for a 
feature but registers are not named in there. E.G. XCR0  = 
X86_XSTATE_AVX_MPX_MASK returning a tdesc_x32_avx_linux.


switch (xcr0_features_bits)
    {
    case X86_XSTATE_MPX_AVX512_MASK:
    case X86_XSTATE_AVX512_MASK:
      if (is_x32)
        return tdesc_x32_avx512_linux;
      else
        return tdesc_amd64_avx512_linux;
    case X86_XSTATE_MPX_MASK:
      if (is_x32)
        return tdesc_x32_avx_linux; /* No MPX on x32 using AVX.  */
      else
        return tdesc_amd64_mpx_linux;
    case X86_XSTATE_AVX_MPX_MASK:
      if (is_x32)
        return tdesc_x32_avx_linux; /* No MPX on x32 using AVX.  */
      else
        return tdesc_amd64_avx_mpx_linux;
    case X86_XSTATE_AVX_MASK:
      if (is_x32)
        return tdesc_x32_avx_linux;
      else
        return tdesc_amd64_avx_linux;
    default:
      if (is_x32)
        return tdesc_x32_linux;
      else
        return tdesc_amd64_linux;
    }



Here based on XCR0 we look for the register, but it is not in the target 
description.
  if ((x86_xcr0 & X86_XSTATE_MPX))
    {
      int ymm0h_regnum = find_regno (regcache->tdesc, "bnd0raw");


Here we assert.
int
find_regno (const struct target_desc *tdesc, const char *name)
{
  int i;

  for (i = 0; i < tdesc->num_registers; i++)
    if (strcmp (name, tdesc->reg_defs[i].name) == 0)
      return i;
  internal_error (__FILE__, __LINE__, "Unknown register %s requested",
          name);
}

My proposal would be to create another variable to keep the actual 
feature bits when detecting the target description to be used.

Thoughts?


Thanks and regards,
-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