[v2] gdbserver: Clear X86_XSTATE_MPX bits in xcr0 on x32

  Since MPX isn't available for x32, we should clear X86_XSTATE_MPX bits
on x32.

	PR server/31511
	* linux-x86-low.cc (x86_linux_read_description): Clear
	X86_XSTATE_MPX bits in xcr0 on x32.
---
 gdbserver/linux-x86-low.cc | 4 ++++
 1 file changed, 4 insertions(+)
  

> -----Original Message-----
> From: H.J. Lu <hjl.tools@gmail.com>
> Sent: Mittwoch, 20. März 2024 12:13
> To: gdb-patches@sourceware.org
> Cc: Willgerodt, Felix <felix.willgerodt@intel.com>; jhb@FreeBSD.org
> Subject: [PATCH v2] gdbserver: Clear X86_XSTATE_MPX bits in xcr0 on x32
> 
> Since MPX isn't available for x32, we should clear X86_XSTATE_MPX bits
> on x32.
> 
> 	PR server/31511
> 	* linux-x86-low.cc (x86_linux_read_description): Clear
> 	X86_XSTATE_MPX bits in xcr0 on x32.
> ---
>  gdbserver/linux-x86-low.cc | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/gdbserver/linux-x86-low.cc b/gdbserver/linux-x86-low.cc
> index 3af0a009052..933d1fb012a 100644
> --- a/gdbserver/linux-x86-low.cc
> +++ b/gdbserver/linux-x86-low.cc
> @@ -938,6 +938,10 @@ x86_linux_read_description (void)
>  	  xcr0 = xstateregs[(I386_LINUX_XSAVE_XCR0_OFFSET
>  			     / sizeof (uint64_t))];
> 
> +	  /* No MPX on x32.  */
> +	  if (machine == EM_X86_64 && !is_elf64)
> +	    xcr0 &= ~X86_XSTATE_MPX;
> +
>  	  xsave_len = x86_xsave_length ();
> 
>  	  /* Use PTRACE_GETREGSET if it is available.  */
> --
> 2.44.0

+1 from me.

Reviewed-by: Felix Willgerodt <felix.willgerodt@intel.com>

Regards,
Felix
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
  

On 3/20/24 7:45 AM, Willgerodt, Felix wrote:
>> -----Original Message-----
>> From: H.J. Lu <hjl.tools@gmail.com>
>> Sent: Mittwoch, 20. März 2024 12:13
>> To: gdb-patches@sourceware.org
>> Cc: Willgerodt, Felix <felix.willgerodt@intel.com>; jhb@FreeBSD.org
>> Subject: [PATCH v2] gdbserver: Clear X86_XSTATE_MPX bits in xcr0 on x32
>>
>> Since MPX isn't available for x32, we should clear X86_XSTATE_MPX bits
>> on x32.
>>
>> 	PR server/31511
>> 	* linux-x86-low.cc (x86_linux_read_description): Clear
>> 	X86_XSTATE_MPX bits in xcr0 on x32.
>> ---
>>   gdbserver/linux-x86-low.cc | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/gdbserver/linux-x86-low.cc b/gdbserver/linux-x86-low.cc
>> index 3af0a009052..933d1fb012a 100644
>> --- a/gdbserver/linux-x86-low.cc
>> +++ b/gdbserver/linux-x86-low.cc
>> @@ -938,6 +938,10 @@ x86_linux_read_description (void)
>>   	  xcr0 = xstateregs[(I386_LINUX_XSAVE_XCR0_OFFSET
>>   			     / sizeof (uint64_t))];
>>
>> +	  /* No MPX on x32.  */
>> +	  if (machine == EM_X86_64 && !is_elf64)
>> +	    xcr0 &= ~X86_XSTATE_MPX;
>> +
>>   	  xsave_len = x86_xsave_length ();
>>
>>   	  /* Use PTRACE_GETREGSET if it is available.  */
>> --
>> 2.44.0
> 
> +1 from me.
> 
> Reviewed-by: Felix Willgerodt <felix.willgerodt@intel.com>

Looks good to me as well.

Approved-By: John Baldwin <jhb@FreeBSD.org>
  

"H.J. Lu" <hjl.tools@gmail.com> writes:

> Since MPX isn't available for x32, we should clear X86_XSTATE_MPX bits
> on x32.
>
> 	PR server/31511
> 	* linux-x86-low.cc (x86_linux_read_description): Clear
> 	X86_XSTATE_MPX bits in xcr0 on x32.
> ---
>  gdbserver/linux-x86-low.cc | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/gdbserver/linux-x86-low.cc b/gdbserver/linux-x86-low.cc
> index 3af0a009052..933d1fb012a 100644
> --- a/gdbserver/linux-x86-low.cc
> +++ b/gdbserver/linux-x86-low.cc
> @@ -938,6 +938,10 @@ x86_linux_read_description (void)
>  	  xcr0 = xstateregs[(I386_LINUX_XSAVE_XCR0_OFFSET
>  			     / sizeof (uint64_t))];
>  
> +	  /* No MPX on x32.  */
> +	  if (machine == EM_X86_64 && !is_elf64)
> +	    xcr0 &= ~X86_XSTATE_MPX;

Hi,

I have a series in flight that conflicts with this change:

  https://inbox.sourceware.org/gdb-patches/cover.1706801009.git.aburgess@redhat.com

And so I'm trying to resolve the conflicts.

Initially I thought I could test this by compiling something with the
gcc -m32 option, but I now believe that is wrong, and I'd actually need
to compile using -mx32, is this correct?

Gcc seems to understand this option, but I appear to be missing the
required development libraries, and I'm currently struggling to figure
out what I'd need to install.  Can you suggest a particular
distro/version that should have the required development libraries
available?  Or maybe I'm just not understanding something here and you
can point out what I'd doing wrong.

The series of mine above came about for one reason: the `machine` type
checks in the gdbserver are not a good idea, they rely on access to an
executable, which isn't always available (e.g. if gdbserver attaches to
a running process for which the executable is not available).  When I
started looking at this code I then decided to merge the whole target
description logic between GDB and gdbserver for x86/linux.

Which leads onto my second question; you changed the gdbserver code, but
not the GDB code.  Was this just an oversight?  Or is there some reason
why this fix doesn't need making for GDB too?

Now worryingly, on the GDB side, in
x86_linux_nat_target::read_description (gdb/x86-linux-nat.c) we do have
a variable `is_x32` which I sounds like it should be true when we are
debugging an x32 process.... but.... this variable is true for a binary
compiled with the -m32 flag, which I don't think is right.  If my
understanding is correct then a -m32 binary is one compiled on an x86-64
machine, but which is uses 32-bit int/pointer types, and which only uses
i386 instructions, and can use MPX, right?

To summarise:

  1. How might I go about compiling an x32 binary?

  2. How can we detect an x32 process without checking the machine type?

  3. Should your fix above be applied on the GDB side too?

I've included below my initial attempt at a test for this change,
currently it compiles with -m32 as (like I said) -mx32 doesn't work on
my machine.  I've been testing this:

  make check-gdb TESTS="gdb.arch/amd64-x32-no-mpx-in-tdesc.exp" \
      RUNTESTFLAGS="--target_board=unix"
  make check-gdb TESTS="gdb.arch/amd64-x32-no-mpx-in-tdesc.exp" \
      RUNTESTFLAGS="--target_board=native-gdbserver"
  make check-gdb TESTS="gdb.arch/amd64-x32-no-mpx-in-tdesc.exp" \
      RUNTESTFLAGS="--target_board=native-extended-gdbserver"

Currently it will fail for all as -m32 != -mx32 and the .mpx feature
shows up (as expected).

I guess you have a target which supports -mx32, so on that target you
should be able to change the '-m32' in the test to '-mx32' and I would
expect you to see the test pass for the gdbserver boards, but fail on
the unix board.

Thanks,
Andrew

---

commit 22167bc5c2f2770e1b1a3f0eb0569df3140921e8
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Sat Mar 23 10:45:22 2024 +0000

    WIP: New test

diff --git a/gdb/testsuite/gdb.arch/amd64-x32-no-mpx-in-tdesc.c b/gdb/testsuite/gdb.arch/amd64-x32-no-mpx-in-tdesc.c
new file mode 100644
index 00000000000..e2119ba7d92
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/amd64-x32-no-mpx-in-tdesc.c
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2024 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int
+main (void)
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.arch/amd64-x32-no-mpx-in-tdesc.exp b/gdb/testsuite/gdb.arch/amd64-x32-no-mpx-in-tdesc.exp
new file mode 100644
index 00000000000..78e0d948348
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/amd64-x32-no-mpx-in-tdesc.exp
@@ -0,0 +1,52 @@
+# Copyright 2024 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# This file is part of the gdb testsuite.
+
+# Check that the mpx feature is not present in the target description
+# when debugging x32 binaries on an x86-64 target.
+
+require {istarget x86_64-*-*}
+
+standard_testfile
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
+	  {debug additional_flags=-m32}] } {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+set found_mpx false
+gdb_test_multiple "maint print xml-tdesc" "" {
+    -re "^maint print xml-tdesc\r\n" {
+	exp_continue
+    }
+
+    -re "^\\s+<feature name=\"org.gnu.gdb.i386.mpx\">\r\n" {
+	set found_mpx true
+	exp_continue
+    }
+
+    -re "^$gdb_prompt $" {
+	gdb_assert { !$found_mpx } \
+	    "mpx feature not in target description"
+    }
+
+    -re "^\[^\r\n\]+\r\n" {
+	exp_continue
+    }
+}
  

Andrew Burgess <aburgess@redhat.com> writes:

> "H.J. Lu" <hjl.tools@gmail.com> writes:
>
>> Since MPX isn't available for x32, we should clear X86_XSTATE_MPX bits
>> on x32.
>>
>> 	PR server/31511
>> 	* linux-x86-low.cc (x86_linux_read_description): Clear
>> 	X86_XSTATE_MPX bits in xcr0 on x32.
>> ---
>>  gdbserver/linux-x86-low.cc | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/gdbserver/linux-x86-low.cc b/gdbserver/linux-x86-low.cc
>> index 3af0a009052..933d1fb012a 100644
>> --- a/gdbserver/linux-x86-low.cc
>> +++ b/gdbserver/linux-x86-low.cc
>> @@ -938,6 +938,10 @@ x86_linux_read_description (void)
>>  	  xcr0 = xstateregs[(I386_LINUX_XSAVE_XCR0_OFFSET
>>  			     / sizeof (uint64_t))];
>>  
>> +	  /* No MPX on x32.  */
>> +	  if (machine == EM_X86_64 && !is_elf64)
>> +	    xcr0 &= ~X86_XSTATE_MPX;
>
> Hi,
>
> I have a series in flight that conflicts with this change:
>
>   https://inbox.sourceware.org/gdb-patches/cover.1706801009.git.aburgess@redhat.com
>
> And so I'm trying to resolve the conflicts.

OK, I think I may have resolved the conflict.  I'm still unable to test
x32 ABI, would you mind applying this series:

  https://inbox.sourceware.org/gdb-patches/cover.1711211528.git.aburgess@redhat.com

and checking that your x32 use case still behaves as you expect.  The
merge of our work occurs in this patch:

  https://inbox.sourceware.org/gdb-patches/a76168beacd9bb79b72ca1a0d26995abd770104c.1711211528.git.aburgess@redhat.com

Also, I invite you to take a look at this additional patch which adds an
extra assert that relates to your work:

  https://inbox.sourceware.org/gdb-patches/159cadba5ba824579cbc6426cb26f45228eda0a7.1711211528.git.aburgess@redhat.com

Any feedback would be great.

Thanks,
Andrew



>
> Initially I thought I could test this by compiling something with the
> gcc -m32 option, but I now believe that is wrong, and I'd actually need
> to compile using -mx32, is this correct?
>
> Gcc seems to understand this option, but I appear to be missing the
> required development libraries, and I'm currently struggling to figure
> out what I'd need to install.  Can you suggest a particular
> distro/version that should have the required development libraries
> available?  Or maybe I'm just not understanding something here and you
> can point out what I'd doing wrong.
>
> The series of mine above came about for one reason: the `machine` type
> checks in the gdbserver are not a good idea, they rely on access to an
> executable, which isn't always available (e.g. if gdbserver attaches to
> a running process for which the executable is not available).  When I
> started looking at this code I then decided to merge the whole target
> description logic between GDB and gdbserver for x86/linux.
>
> Which leads onto my second question; you changed the gdbserver code, but
> not the GDB code.  Was this just an oversight?  Or is there some reason
> why this fix doesn't need making for GDB too?
>
> Now worryingly, on the GDB side, in
> x86_linux_nat_target::read_description (gdb/x86-linux-nat.c) we do have
> a variable `is_x32` which I sounds like it should be true when we are
> debugging an x32 process.... but.... this variable is true for a binary
> compiled with the -m32 flag, which I don't think is right.  If my
> understanding is correct then a -m32 binary is one compiled on an x86-64
> machine, but which is uses 32-bit int/pointer types, and which only uses
> i386 instructions, and can use MPX, right?
>
> To summarise:
>
>   1. How might I go about compiling an x32 binary?
>
>   2. How can we detect an x32 process without checking the machine type?
>
>   3. Should your fix above be applied on the GDB side too?
>
> I've included below my initial attempt at a test for this change,
> currently it compiles with -m32 as (like I said) -mx32 doesn't work on
> my machine.  I've been testing this:
>
>   make check-gdb TESTS="gdb.arch/amd64-x32-no-mpx-in-tdesc.exp" \
>       RUNTESTFLAGS="--target_board=unix"
>   make check-gdb TESTS="gdb.arch/amd64-x32-no-mpx-in-tdesc.exp" \
>       RUNTESTFLAGS="--target_board=native-gdbserver"
>   make check-gdb TESTS="gdb.arch/amd64-x32-no-mpx-in-tdesc.exp" \
>       RUNTESTFLAGS="--target_board=native-extended-gdbserver"
>
> Currently it will fail for all as -m32 != -mx32 and the .mpx feature
> shows up (as expected).
>
> I guess you have a target which supports -mx32, so on that target you
> should be able to change the '-m32' in the test to '-mx32' and I would
> expect you to see the test pass for the gdbserver boards, but fail on
> the unix board.
>
> Thanks,
> Andrew
>
> ---
>
> commit 22167bc5c2f2770e1b1a3f0eb0569df3140921e8
> Author: Andrew Burgess <aburgess@redhat.com>
> Date:   Sat Mar 23 10:45:22 2024 +0000
>
>     WIP: New test
>
> diff --git a/gdb/testsuite/gdb.arch/amd64-x32-no-mpx-in-tdesc.c b/gdb/testsuite/gdb.arch/amd64-x32-no-mpx-in-tdesc.c
> new file mode 100644
> index 00000000000..e2119ba7d92
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/amd64-x32-no-mpx-in-tdesc.c
> @@ -0,0 +1,22 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2024 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +int
> +main (void)
> +{
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.arch/amd64-x32-no-mpx-in-tdesc.exp b/gdb/testsuite/gdb.arch/amd64-x32-no-mpx-in-tdesc.exp
> new file mode 100644
> index 00000000000..78e0d948348
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/amd64-x32-no-mpx-in-tdesc.exp
> @@ -0,0 +1,52 @@
> +# Copyright 2024 Free Software Foundation, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +# This file is part of the gdb testsuite.
> +
> +# Check that the mpx feature is not present in the target description
> +# when debugging x32 binaries on an x86-64 target.
> +
> +require {istarget x86_64-*-*}
> +
> +standard_testfile
> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
> +	  {debug additional_flags=-m32}] } {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +    return -1
> +}
> +
> +set found_mpx false
> +gdb_test_multiple "maint print xml-tdesc" "" {
> +    -re "^maint print xml-tdesc\r\n" {
> +	exp_continue
> +    }
> +
> +    -re "^\\s+<feature name=\"org.gnu.gdb.i386.mpx\">\r\n" {
> +	set found_mpx true
> +	exp_continue
> +    }
> +
> +    -re "^$gdb_prompt $" {
> +	gdb_assert { !$found_mpx } \
> +	    "mpx feature not in target description"
> +    }
> +
> +    -re "^\[^\r\n\]+\r\n" {
> +	exp_continue
> +    }
> +}
  

On Sat, Mar 23, 2024 at 9:39 AM Andrew Burgess <aburgess@redhat.com> wrote:
>
> Andrew Burgess <aburgess@redhat.com> writes:
>
> > "H.J. Lu" <hjl.tools@gmail.com> writes:
> >
> >> Since MPX isn't available for x32, we should clear X86_XSTATE_MPX bits
> >> on x32.
> >>
> >>      PR server/31511
> >>      * linux-x86-low.cc (x86_linux_read_description): Clear
> >>      X86_XSTATE_MPX bits in xcr0 on x32.
> >> ---
> >>  gdbserver/linux-x86-low.cc | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/gdbserver/linux-x86-low.cc b/gdbserver/linux-x86-low.cc
> >> index 3af0a009052..933d1fb012a 100644
> >> --- a/gdbserver/linux-x86-low.cc
> >> +++ b/gdbserver/linux-x86-low.cc
> >> @@ -938,6 +938,10 @@ x86_linux_read_description (void)
> >>        xcr0 = xstateregs[(I386_LINUX_XSAVE_XCR0_OFFSET
> >>                           / sizeof (uint64_t))];
> >>
> >> +      /* No MPX on x32.  */
> >> +      if (machine == EM_X86_64 && !is_elf64)
> >> +        xcr0 &= ~X86_XSTATE_MPX;
> >
> > Hi,
> >
> > I have a series in flight that conflicts with this change:
> >
> >   https://inbox.sourceware.org/gdb-patches/cover.1706801009.git.aburgess@redhat.com
> >
> > And so I'm trying to resolve the conflicts.
>
> OK, I think I may have resolved the conflict.  I'm still unable to test
> x32 ABI, would you mind applying this series:
>
>   https://inbox.sourceware.org/gdb-patches/cover.1711211528.git.aburgess@redhat.com
>
> and checking that your x32 use case still behaves as you expect.  The
> merge of our work occurs in this patch:
>
>   https://inbox.sourceware.org/gdb-patches/a76168beacd9bb79b72ca1a0d26995abd770104c.1711211528.git.aburgess@redhat.com
>
> Also, I invite you to take a look at this additional patch which adds an
> extra assert that relates to your work:
>
>   https://inbox.sourceware.org/gdb-patches/159cadba5ba824579cbc6426cb26f45228eda0a7.1711211528.git.aburgess@redhat.com
>
> Any feedback would be great.
>

I applied the whole patch series from

https://patchwork.sourceware.org/project/gdb/list/?series=32189

Both gdb and gdbserver work on x32.

Thanks.