Re: [PATCH v2 1/1] Fix PR16193 - gdbserver aborts.

[Pedro Alves <palves@redhat.com>]

On 11/25/2013 03:55 PM, Walfred Tedeschi wrote:
> The MPX patch has broken the I386_XSTATE_SIZE macro. For any xcr0 value
> entered return is 576. This patch fixes that and improves readability
> of the macros.

Since you still didn't explain it, I took another closer look.
I can't believe how long it took me to spot it.  I kept staring
at the BNDREGS and BNDCFG bits, but the issue is really in the AVX
check.

>  /* Get I386 XSAVE extended state size.  */
>  #define I386_XSTATE_SIZE(XCR0) \
> -    (((XCR0) & I386_XSTATE_BNDCFG) != 0 ? I386_XSTATE_BNDCFG_SIZE \
> -       : (((XCR0) & I386_XSTATE_BNDREGS) != 0 ? I386_XSTATE_BNDCFG_SIZE \
> -       : (((XCR0) & I386_XSTATE_AVX_SIZE) != 0 ? I386_XSTATE_AVX_SIZE \

The issue is here:

     : (((XCR0) & I386_XSTATE_AVX_SIZE) != 0 ? I386_XSTATE_AVX_SIZE \

That should have been:

     : (((XCR0) & I386_XSTATE_AVX) != 0 ? I386_XSTATE_AVX_SIZE \


Please fix the rationale in the commit log.  It's not true that
I386_XSTATE_SIZE _always_ returns 576.  For MPX machines, the
existing code returns the right value.  It's only non-MPX AVX
machines that get the wrong value.  IOW, this one lines would
be the equivalent smallest fix:

/* Get I386 XSAVE extended state size.  */
#define I386_XSTATE_SIZE(XCR0) \
    (((XCR0) & I386_XSTATE_BNDCFG) != 0 ? I386_XSTATE_BNDCFG_SIZE \
       : (((XCR0) & I386_XSTATE_BNDREGS) != 0 ? I386_XSTATE_BNDCFG_SIZE \
-       : (((XCR0) & I386_XSTATE_AVX_SIZE) != 0 ? I386_XSTATE_AVX_SIZE \
+       : (((XCR0) & I386_XSTATE_AVX) != 0 ? I386_XSTATE_AVX_SIZE \
       : I386_XSTATE_SSE_SIZE)))


Something like this:

"
The MPX patch has broken the I386_XSTATE_SIZE macro.  For AVX machines,
it ends up returning I386_XSTATE_SSE_SIZE.  Where it first reads
I386_XSTATE_AVX_SIZE, it should have read I386_XSTATE_AVX:

 #define I386_XSTATE_SIZE(XCR0) \
     (((XCR0) & I386_XSTATE_BNDCFG) != 0 ? I386_XSTATE_BNDCFG_SIZE \
        : (((XCR0) & I386_XSTATE_BNDREGS) != 0 ? I386_XSTATE_BNDCFG_SIZE \
 -       : (((XCR0) & I386_XSTATE_AVX_SIZE) != 0 ? I386_XSTATE_AVX_SIZE \
 +       : (((XCR0) & I386_XSTATE_AVX) != 0 ? I386_XSTATE_AVX_SIZE \
        : I386_XSTATE_SSE_SIZE)))

The patch goes a step further and improves readability of the
macro, by adding a couple other auxiliary macros.

2013-11-25  Walfred Tedeschi  <walfred.tedeschi@intel.com>

	* i386-xstate.h (I386_XSTATE_MPX): New Macro.
	(I386_XSTATE_MPX_MASK): Makes use of I386_XSTATE_MPX.
	(HAS_MPX): New macro.
	(HAS_AVX): New macro.
	(I386_XSTATE_SIZE): Uses HAS_MPX and HAS_AVX.
"

> 2013-12-25  Walfred Tedeschi  <walfred.tedeschi@intel.com>

Longing for Christmas? ;-)