This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v2 1/1] Fix PR16193 - gdbserver aborts.
- From: Pedro Alves <palves at redhat dot com>
- To: Walfred Tedeschi <walfred dot tedeschi at intel dot com>
- Cc: mark dot kettenis at xs4all dot nl, gdb-patches at sourceware dot org
- Date: Mon, 25 Nov 2013 16:27:56 +0000
- Subject: Re: [PATCH v2 1/1] Fix PR16193 - gdbserver aborts.
- Authentication-results: sourceware.org; auth=none
- References: <1385394941-26593-1-git-send-email-walfred dot tedeschi at intel dot 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? ;-)