[PATCH v1 1/1] Fix PR16193 - gdbserver aborts.

[PATCH v1 1/1] Fix PR16193 - gdbserver aborts.

[Walfred Tedeschi]

Macro returning the size of the xsave buffer got broken with the MPX
patch.  Fix improves the macro to make it more readable.

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): Refactored macro using HAS_MPX and
	HAS_AVX.
---
 gdb/common/i386-xstate.h | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/gdb/common/i386-xstate.h b/gdb/common/i386-xstate.h
index e76ecc1..f8b85e3 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,14 @@
 
 #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) == I386_XSTATE_BNDREGS) \
+        || (((XCR0) & I386_XSTATE_MPX) == I386_XSTATE_BNDCFG)
+#define HAS_AVX(XCR0) ((XCR0) & I386_XSTATE_AVX) == I386_XSTATE_AVX
+
 /* 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

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

[Pedro Alves]

Thanks.

On 11/25/2013 01:40 PM, Walfred Tedeschi wrote:
> Macro returning the size of the xsave buffer got broken with the MPX
> patch.  Fix improves the macro to make it more readable.

To be clear, this patch doesn't do any fix yet -- it's just
a preparatory cleanup, right?

-- 
Pedro Alves

RE: [PATCH v1 1/1] Fix PR16193 - gdbserver aborts.

[Tedeschi, Walfred]

Pedro,

This should be the fix. The issue was that the buffer size was not matching the size actually read.
Macro was really broken, for any XCR0 value it was returning 576, as pointed by Joel:
https://sourceware.org/ml/gdb-patches/2013-11/msg00761.html

I have tested the macros on an ad-hoc application to verify its functionality.

Do you still see the issue when applying this?

Thanks for your review!
Regards,
-Fred

-----Original Message-----
From: Pedro Alves [mailto:palves@redhat.com] 
Sent: Monday, November 25, 2013 3:24 PM
To: Tedeschi, Walfred
Cc: yao@codesourcery.com; gdb-patches@sourceware.org
Subject: Re: [PATCH v1 1/1] Fix PR16193 - gdbserver aborts.

Thanks.

On 11/25/2013 01:40 PM, Walfred Tedeschi wrote:
> Macro returning the size of the xsave buffer got broken with the MPX 
> patch.  Fix improves the macro to make it more readable.

To be clear, this patch doesn't do any fix yet -- it's just a preparatory cleanup, right?

--
Pedro Alves

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

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

[Pedro Alves]

On 11/25/2013 02:32 PM, Tedeschi, Walfred wrote:
> This should be the fix. The issue was that the buffer size was not matching the size actually read.

I'm staring at the patch, and not seeing what the actual functional
change was.  Can you point it out?  That's the sort of thing that helps
review, and that is good to have in the email/commit log -- a rationale
for the fix, an explanation of what was wrong, how the fix actually fixes it, etc.

> Macro was really broken, for any XCR0 value it was returning 576, as pointed by Joel:
> https://sourceware.org/ml/gdb-patches/2013-11/msg00761.html
> 
> I have tested the macros on an ad-hoc application to verify its functionality.
> 
> Do you still see the issue when applying this?

I haven't tried it yet.  I'll do that now.

Thanks,
-- 
Pedro Alves

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

[Pedro Alves]

On 11/25/2013 02:39 PM, Pedro Alves wrote:
>> > Do you still see the issue when applying this?
> I haven't tried it yet.  I'll do that now.

The issue is gone with the patch.  It works.  I don't
know how, but it works.  :-)

-- 
Pedro Alves

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

[Mark Kettenis]

> From: Walfred Tedeschi <walfred.tedeschi@intel.com>
> Date: Mon, 25 Nov 2013 14:40:02 +0100
> 
> Macro returning the size of the xsave buffer got broken with the MPX
> patch.  Fix improves the macro to make it more readable.
> 
> 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): Refactored macro using HAS_MPX and
> 	HAS_AVX.
> ---
>  gdb/common/i386-xstate.h | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> +/* In case one of the MPX XCR0 bits is set we consider we have MPX.  */
> +#define HAS_MPX(XCR0) (((XCR0) & I386_XSTATE_MPX) == I386_XSTATE_BNDREGS) \
> +        || (((XCR0) & I386_XSTATE_MPX) == I386_XSTATE_BNDCFG)

That doesn't seem to do what it says on the tin!

RE: [PATCH v1 1/1] Fix PR16193 - gdbserver aborts.

[Tedeschi, Walfred]

Hello Mark,

Ok, I Will let it symmetrical to the AVX, i.e:

/* In case one of the MPX XCR0 bits is set we consider we have MPX.  
 #define HAS_MPX(XCR0) ((XCR0) & I386_XSTATE_MPX) == I386_XSTATE_MPX


Thanks a lot for your review,
Regards,
-Fred
-----Original Message-----
From: Mark Kettenis [mailto:mark.kettenis@xs4all.nl] 
Sent: Monday, November 25, 2013 3:49 PM
To: Tedeschi, Walfred
Cc: palves@redhat.com; yao@codesourcery.com; gdb-patches@sourceware.org; Tedeschi, Walfred
Subject: Re: [PATCH v1 1/1] Fix PR16193 - gdbserver aborts.

> From: Walfred Tedeschi <walfred.tedeschi@intel.com>
> Date: Mon, 25 Nov 2013 14:40:02 +0100
> 
> Macro returning the size of the xsave buffer got broken with the MPX 
> patch.  Fix improves the macro to make it more readable.
> 
> 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): Refactored macro using HAS_MPX and
> 	HAS_AVX.
> ---
>  gdb/common/i386-xstate.h | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> +/* In case one of the MPX XCR0 bits is set we consider we have MPX.  
> +*/ #define HAS_MPX(XCR0) (((XCR0) & I386_XSTATE_MPX) == I386_XSTATE_BNDREGS) \
> +        || (((XCR0) & I386_XSTATE_MPX) == I386_XSTATE_BNDCFG)

That doesn't seem to do what it says on the tin!
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

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

[Pedro Alves]

On 11/25/2013 02:56 PM, Tedeschi, Walfred wrote:
> 
> /* In case one of the MPX XCR0 bits is set we consider we have MPX.  
>  #define HAS_MPX(XCR0) ((XCR0) & I386_XSTATE_MPX) == I386_XSTATE_MPX

Please wrap macros in ()'s:

#define HAS_MPX(XCR0) (((XCR0) & I386_XSTATE_MPX) == I386_XSTATE_MPX)

-- 
Pedro Alves

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

[Mark Kettenis]

> From: "Tedeschi, Walfred" <walfred.tedeschi@intel.com>
> Date: Mon, 25 Nov 2013 14:56:59 +0000
> 
> Hello Mark,
> 
> Ok, I Will let it symmetrical to the AVX, i.e:
> 
> /* In case one of the MPX XCR0 bits is set we consider we have MPX.  
>  #define HAS_MPX(XCR0) ((XCR0) & I386_XSTATE_MPX) == I386_XSTATE_MPX

Still doesn't do what it says on the tin.  The macro is checking if
*both* bits are set.  If you want to have the code match the comment
it needs to be changed to something like:

#define HAS_MPX(XRC0) (((XRC0) & I386_XSTATE_MPX) != 0)

or you need to change the comment to state that all bits need to be set.

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

[Walfred Tedeschi]

Am 11/25/2013 4:18 PM, schrieb Mark Kettenis:
>> From: "Tedeschi, Walfred" <walfred.tedeschi@intel.com>
>> Date: Mon, 25 Nov 2013 14:56:59 +0000
>>
>> Hello Mark,
>>
>> Ok, I Will let it symmetrical to the AVX, i.e:
>>
>> /* In case one of the MPX XCR0 bits is set we consider we have MPX.
>>   #define HAS_MPX(XCR0) ((XCR0) & I386_XSTATE_MPX) == I386_XSTATE_MPX
> Still doesn't do what it says on the tin.  The macro is checking if
> *both* bits are set.  If you want to have the code match the comment
> it needs to be changed to something like:
>
> #define HAS_MPX(XRC0) (((XRC0) & I386_XSTATE_MPX) != 0)
>
> or you need to change the comment to state that all bits need to be set.
Hello Mark,

What you proposed is better.
I will incorporate your feedback and Pedro's feedback and resend it.

Thanks your review and feedback!
Regards,
-Fred
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

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

[Walfred Tedeschi]

Hello All,

For clarification the macro I386_XSTATE_SIZE(XCR0) was returning for any 
XCR0 the value 576.
However when reading the registers we used more than this buffer size 
(when on AVX case).
Interestingly this did not happened for MPX.

Patch bellow changes/fixes the I386_XSTATE_SIZE(XCR0). To fix acctually 
the problem we decided to improve
macro readability.

Regards,
-Fred



Am 11/25/2013 2:40 PM, schrieb Walfred Tedeschi:
> Macro returning the size of the xsave buffer got broken with the MPX
> patch.  Fix improves the macro to make it more readable.
>
> 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): Refactored macro using HAS_MPX and
> 	HAS_AVX.
> ---
>   gdb/common/i386-xstate.h | 16 +++++++++-------
>   1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/gdb/common/i386-xstate.h b/gdb/common/i386-xstate.h
> index e76ecc1..f8b85e3 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,14 @@
>   
>   #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) == I386_XSTATE_BNDREGS) \
> +        || (((XCR0) & I386_XSTATE_MPX) == I386_XSTATE_BNDCFG)
> +#define HAS_AVX(XCR0) ((XCR0) & I386_XSTATE_AVX) == I386_XSTATE_AVX
> +
>   /* 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 */

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

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

[Pedro Alves]

On 11/25/2013 02:51 PM, Walfred Tedeschi wrote:
> Hello All,
> 
> For clarification the macro I386_XSTATE_SIZE(XCR0) was returning for any 
> XCR0 the value 576.

I must be very dense today, sorry.  :-(  Can you point out why that
was so before the patch, and no longer is so after the patch?

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

I'm unrolling the new macros in my head, and I can figure out why
does this now return a different value.  :-/

-- 
Pedro Alves