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

[Andrew Burgess <aburgess@redhat.com>]

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
> +    }
> +}