* [PATCH v2 1/1] Fix PR16193 - gdbserver aborts.
@ 2013-11-25 16:28 Walfred Tedeschi
2013-11-25 17:00 ` Pedro Alves
0 siblings, 1 reply; 3+ messages in thread
From: Walfred Tedeschi @ 2013-11-25 16:28 UTC (permalink / raw)
To: palves, mark.kettenis; +Cc: gdb-patches, Walfred Tedeschi
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.
2013-12-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.
---
gdb/common/i386-xstate.h | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/gdb/common/i386-xstate.h b/gdb/common/i386-xstate.h
index e76ecc1..c4b8c8a 100644
--- a/gdb/common/i386-xstate.h
+++ b/gdb/common/i386-xstate.h
@@ -26,14 +26,13 @@
#define I386_XSTATE_AVX (1ULL << 2)
#define I386_XSTATE_BNDREGS (1ULL << 3)
#define I386_XSTATE_BNDCFG (1ULL << 4)
+#define I386_XSTATE_MPX (I386_XSTATE_BNDREGS | I386_XSTATE_BNDCFG)
/* Supported mask and size of the extended state. */
#define I386_XSTATE_X87_MASK I386_XSTATE_X87
#define I386_XSTATE_SSE_MASK (I386_XSTATE_X87 | I386_XSTATE_SSE)
#define I386_XSTATE_AVX_MASK (I386_XSTATE_SSE_MASK | I386_XSTATE_AVX)
-#define I386_XSTATE_MPX_MASK (I386_XSTATE_AVX_MASK \
- | I386_XSTATE_BNDREGS \
- | I386_XSTATE_BNDCFG)
+#define I386_XSTATE_MPX_MASK (I386_XSTATE_AVX_MASK | I386_XSTATE_MPX)
#define I386_XSTATE_ALL_MASK I386_XSTATE_MPX_MASK
@@ -44,11 +43,13 @@
#define I386_XSTATE_MAX_SIZE 1088
+/* In case one of the MPX XCR0 bits is set we consider we have MPX. */
+#define HAS_MPX(XCR0) (((XCR0) & I386_XSTATE_MPX) != 0)
+#define HAS_AVX(XCR0) (((XCR0) & I386_XSTATE_AVX) != 0)
+
/* 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 \
- : I386_XSTATE_SSE_SIZE)))
+ (HAS_MPX (XCR0) ? I386_XSTATE_BNDCFG_SIZE : \
+ (HAS_AVX (XCR0) ? I386_XSTATE_AVX_SIZE : I386_XSTATE_SSE_SIZE))
#endif /* I386_XSTATE_H */
--
1.8.1.2
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2 1/1] Fix PR16193 - gdbserver aborts.
2013-11-25 16:28 [PATCH v2 1/1] Fix PR16193 - gdbserver aborts Walfred Tedeschi
@ 2013-11-25 17:00 ` Pedro Alves
2013-11-25 17:08 ` Tedeschi, Walfred
0 siblings, 1 reply; 3+ messages in thread
From: Pedro Alves @ 2013-11-25 17:00 UTC (permalink / raw)
To: Walfred Tedeschi; +Cc: mark.kettenis, gdb-patches
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? ;-)
^ permalink raw reply [flat|nested] 3+ messages in thread* RE: [PATCH v2 1/1] Fix PR16193 - gdbserver aborts.
2013-11-25 17:00 ` Pedro Alves
@ 2013-11-25 17:08 ` Tedeschi, Walfred
0 siblings, 0 replies; 3+ messages in thread
From: Tedeschi, Walfred @ 2013-11-25 17:08 UTC (permalink / raw)
To: Pedro Alves; +Cc: mark.kettenis, gdb-patches
Hi Pedro,
In fact I didn't like this macro from the beginning on. And liked less when I saw it with AVX512.
Therefore I took the time to rewrite it. It was too awful to read in the way it was. :)
:)
I will fix the rationale.
Thanks and regards,
-Fred
-----Original Message-----
From: Pedro Alves [mailto:palves@redhat.com]
Sent: Monday, November 25, 2013 5:28 PM
To: Tedeschi, Walfred
Cc: mark.kettenis@xs4all.nl; gdb-patches@sourceware.org
Subject: Re: [PATCH v2 1/1] Fix PR16193 - gdbserver aborts.
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? ;-)
-> Oh! Yah! :)
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-11-25 16:50 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-25 16:28 [PATCH v2 1/1] Fix PR16193 - gdbserver aborts Walfred Tedeschi
2013-11-25 17:00 ` Pedro Alves
2013-11-25 17:08 ` Tedeschi, Walfred
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox