[PATCHv7,7/9] gdb/gdbserver: share some code relating to target description creation

Message ID 3f2e66bab7bb82afcc7cd5297786807c6626f2d6.1715421687.git.aburgess@redhat.com
State New
Headers
Series x86/Linux Target Description Changes |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Testing passed

Commit Message

Andrew Burgess May 11, 2024, 10:08 a.m. UTC
  This commit is part of a series to share more of the x86 target
description creation code between GDB and gdbserver.

Unlike previous commits which were mostly refactoring, this commit is
the first that makes a real change, though that change should mostly
be for gdbserver; I've largely adopted the "GDB" way of doing things
for gdbserver, and this fixes a real gdbserver bug.

On a x86-64 Linux target, running the test:

  gdb.server/connect-with-no-symbol-file.exp

results in two core files being created.  Both of these core files are
from the inferior process, created after gdbserver has detached.

In this test a gdbserver process is started and then, after gdbserver
has started, but before GDB attaches, we either delete the inferior
executable, or change its permissions so it can't be read.  Only after
doing this do we attempt to connect with GDB.

As GDB connects to gdbserver, gdbserver attempts to figure out the
target description so that it can send the description to GDB, this
involves a call to x86_linux_read_description.

In x86_linux_read_description one of the first things we do is try to
figure out if the process is 32-bit or 64-bit.  To do this we look up
the executable via the thread-id, and then attempt to read the
architecture size from the executable.  This isn't going to work if
the executable has been deleted, or is no longer readable.

And so, as we can't read the executable, we default to an i386 target
and use an i386 target description.

A consequence of using an i386 target description is that addresses
are assumed to be 32-bits.  Here's an example session that shows the
problems this causes.  This is run on an x86-64 machine, and the test
binary (xx.x) is a standard 64-bit x86-64 binary:

  shell_1$ gdbserver --once localhost :54321 /tmp/xx.x

  shell_2$ gdb -q
  (gdb) set sysroot
  (gdb) shell chmod 000 /tmp/xx.x
  (gdb) target remote :54321
  Remote debugging using :54321
  warning: /tmp/xx.x: Permission denied.
  0xf7fd3110 in ?? ()
  (gdb) show architecture
  The target architecture is set to "auto" (currently "i386").
  (gdb) p/x $pc
  $1 = 0xf7fd3110
  (gdb) info proc mappings
  process 2412639
  Mapped address spaces:

  	Start Addr   End Addr       Size     Offset  Perms   objfile
  	  0x400000   0x401000     0x1000        0x0  r--p   /tmp/xx.x
  	  0x401000   0x402000     0x1000     0x1000  r-xp   /tmp/xx.x
  	  0x402000   0x403000     0x1000     0x2000  r--p   /tmp/xx.x
  	  0x403000   0x405000     0x2000     0x2000  rw-p   /tmp/xx.x
  	0xf7fcb000 0xf7fcf000     0x4000        0x0  r--p   [vvar]
  	0xf7fcf000 0xf7fd1000     0x2000        0x0  r-xp   [vdso]
  	0xf7fd1000 0xf7fd3000     0x2000        0x0  r--p   /usr/lib64/ld-2.30.so
  	0xf7fd3000 0xf7ff3000    0x20000     0x2000  r-xp   /usr/lib64/ld-2.30.so
  	0xf7ff3000 0xf7ffb000     0x8000    0x22000  r--p   /usr/lib64/ld-2.30.so
  	0xf7ffc000 0xf7ffe000     0x2000    0x2a000  rw-p   /usr/lib64/ld-2.30.so
  	0xf7ffe000 0xf7fff000     0x1000        0x0  rw-p
  	0xfffda000 0xfffff000    0x25000        0x0  rw-p   [stack]
  	0xff600000 0xff601000     0x1000        0x0  r-xp   [vsyscall]
  (gdb) info inferiors
    Num  Description       Connection           Executable
  * 1    process 2412639   1 (remote :54321)
  (gdb) shell cat /proc/2412639/maps
  00400000-00401000 r--p 00000000 fd:03 45907133           /tmp/xx.x
  00401000-00402000 r-xp 00001000 fd:03 45907133           /tmp/xx.x
  00402000-00403000 r--p 00002000 fd:03 45907133           /tmp/xx.x
  00403000-00405000 rw-p 00002000 fd:03 45907133           /tmp/xx.x
  7ffff7fcb000-7ffff7fcf000 r--p 00000000 00:00 0          [vvar]
  7ffff7fcf000-7ffff7fd1000 r-xp 00000000 00:00 0          [vdso]
  7ffff7fd1000-7ffff7fd3000 r--p 00000000 fd:00 143904     /usr/lib64/ld-2.30.so
  7ffff7fd3000-7ffff7ff3000 r-xp 00002000 fd:00 143904     /usr/lib64/ld-2.30.so
  7ffff7ff3000-7ffff7ffb000 r--p 00022000 fd:00 143904     /usr/lib64/ld-2.30.so
  7ffff7ffc000-7ffff7ffe000 rw-p 0002a000 fd:00 143904     /usr/lib64/ld-2.30.so
  7ffff7ffe000-7ffff7fff000 rw-p 00000000 00:00 0
  7ffffffda000-7ffffffff000 rw-p 00000000 00:00 0          [stack]
  ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0  [vsyscall]
  (gdb)

Notice the difference between the mappings reported via GDB and those
reported directly from the kernel via /proc/PID/maps, the addresses of
every mapping is clamped to 32-bits for GDB, while the kernel reports
real 64-bit addresses.

Notice also that the $pc value is a 32-bit value.  It appears to be
within one of the mappings reported by GDB, but is outside any of the
mappings reported from the kernel.

And this is where the problem arises.  When gdbserver detaches from
the inferior we pass the inferior the address from which it should
resume.  Due to the 32/64 bit confusion we tell the inferior to resume
from the 32-bit $pc value, which is not within any valid mapping, and
so, as soon as the inferior resumes, it segfaults.

If we look at how GDB (not gdbserver) figures out its target
description then we see an interesting difference.  GDB doesn't try to
read the executable.  Instead GDB uses ptrace to query the thread's
state, and uses this to figure out the if the thread is 32 or 64 bit.

If we update gdbserver to do it the "GDB" way then the above problem
is resolved, gdbserver now sees the process as 64-bit, and when we
detach from the inferior we give it the correct 64-bit address, and
the inferior no longer segfaults.

Now, I could just update the gdbserver code, but better, I think, to
share one copy of the code between GDB and gdbserver in gdb/nat/.
That is what this commit does.

The cores of x86_linux_read_description from gdbserver and
x86_linux_nat_target::read_description from GDB are moved into a new
file gdb/nat/x86-linux-tdesc.c and combined into a single function
x86_linux_tdesc_for_tid which is called from each location.

This new function does things mostly the GDB way, some changes are
needed to allow for the sharing; we now take some pointers for where
the shared code can cache the xcr0 and xsave layout values.

Another thing to note about this commit is how the functions
i386_linux_read_description and amd64_linux_read_description are
handled.  For now I've left these function as implemented separately
in GDB and gdbserver.  I've moved the declarations of these functions
into gdb/arch/{i386,amd64}-linux-tdesc.h, but the implementations are
left where they are.

A later commit in this series will make these functions shared too,
but doing this is not trivial, so I've left that for a separate
commit.  Merging the declarations as I've done here ensures that
everyone implements the function to the same API, and once these
functions are shared (in a later commit) we'll want a shared
declaration anyway.

Reviewed-By: Felix Willgerodt <felix.willgerodt@intel.com>
Acked-By: John Baldwin <jhb@FreeBSD.org>
---
 gdb/Makefile.in              |   3 +
 gdb/amd64-linux-tdep.c       |   1 +
 gdb/amd64-linux-tdep.h       |   6 --
 gdb/arch/amd64-linux-tdesc.h |  30 +++++++
 gdb/arch/i386-linux-tdesc.h  |  29 +++++++
 gdb/configure.nat            |   5 +-
 gdb/i386-linux-tdep.c        |   1 +
 gdb/i386-linux-tdep.h        |   3 -
 gdb/nat/x86-linux-tdesc.c    | 129 ++++++++++++++++++++++++++++++
 gdb/nat/x86-linux-tdesc.h    |  54 +++++++++++++
 gdb/x86-linux-nat.c          |  88 ++-------------------
 gdbserver/configure.srv      |   2 +
 gdbserver/i387-fp.cc         |   9 +--
 gdbserver/i387-fp.h          |   4 +-
 gdbserver/linux-amd64-ipa.cc |   1 +
 gdbserver/linux-i386-ipa.cc  |   1 +
 gdbserver/linux-x86-low.cc   | 147 ++++++++++-------------------------
 gdbserver/linux-x86-tdesc.cc |   2 +
 gdbserver/linux-x86-tdesc.h  |   7 --
 19 files changed, 312 insertions(+), 210 deletions(-)
 create mode 100644 gdb/arch/amd64-linux-tdesc.h
 create mode 100644 gdb/arch/i386-linux-tdesc.h
 create mode 100644 gdb/nat/x86-linux-tdesc.c
 create mode 100644 gdb/nat/x86-linux-tdesc.h
  

Comments

Willgerodt, Felix May 17, 2024, 11:59 a.m. UTC | #1
Hi Andrew,

I mainly have some nits and one actual remark (the x32 check).
Thanks again for doing this and sorry for taking a bit to review this.

Felix
 
> diff --git a/gdb/nat/x86-linux-tdesc.c b/gdb/nat/x86-linux-tdesc.c
> new file mode 100644
> index 00000000000..b065928f8ba
> --- /dev/null
> +++ b/gdb/nat/x86-linux-tdesc.c
> @@ -0,0 +1,129 @@
> +/* Target description related code for GNU/Linux x86 (i386 and x86-64).
> +
> +   Copyright (C) 2024 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   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/>.  */
> +
> +#include "nat/x86-linux-tdesc.h"
> +#ifdef __x86_64__
> +#include "arch/amd64.h"
> +#include "arch/amd64-linux-tdesc.h"
> +#endif
> +#include "arch/i386.h"
> +#include "arch/i386-linux-tdesc.h"
> +
> +#include "nat/x86-linux.h"
> +#include "nat/gdb_ptrace.h"
> +#include "nat/x86-xstate.h"
> +#include "gdbsupport/x86-xstate.h"
> +
> +#ifndef __x86_64__
> +#include <sys/procfs.h>
> +#include "nat/i386-linux.h"
> +#endif
> +
> +#include <sys/uio.h>
> +#include <elf.h>
> +
> +#ifndef IN_PROCESS_AGENT
> +
> +/* See nat/x86-linux-tdesc.h.  */
> +
> +const target_desc *
> +x86_linux_tdesc_for_tid (int tid, const char *error_msg,
> +			 uint64_t *xcr0_storage,
> +			 x86_xsave_layout *xsave_layout_storage)
> +{
> +#ifdef __x86_64__
> +

It looks weird to me that there is an empty line here but not for the others.

> +  x86_linux_arch_size arch_size = x86_linux_ptrace_get_arch_size (tid);
> +  bool is_64bit = arch_size.is_64bit ();
> +  bool is_x32 = arch_size.is_x32 ();
> +
> +  if (sizeof (void *) == 4 && is_64bit && !is_x32)
> +    error ("%s", error_msg);
> +
> +#elif HAVE_PTRACE_GETFPXREGS
> +  if (have_ptrace_getfpxregs == TRIBOOL_UNKNOWN)
> +    {
> +      elf_fpxregset_t fpxregs;
> +
> +      if (ptrace (PTRACE_GETFPXREGS, tid, 0, (int) &fpxregs) < 0)
> +	{
> +	  have_ptrace_getfpxregs = TRIBOOL_FALSE;
> +	  have_ptrace_getregset = TRIBOOL_FALSE;
> +	}
> +      else
> +	have_ptrace_getfpxregs = TRIBOOL_TRUE;
> +    }
> +
> +  if (have_ptrace_getfpxregs == TRIBOOL_FALSE)
> +    return i386_linux_read_description (X86_XSTATE_X87_MASK);
> +#endif
> +
> +  if (have_ptrace_getregset == TRIBOOL_UNKNOWN)
> +    {
> +      uint64_t xstateregs[(X86_XSTATE_SSE_SIZE / sizeof (uint64_t))];
> +      struct iovec iov;
> +
> +      iov.iov_base = xstateregs;
> +      iov.iov_len = sizeof (xstateregs);
> +
> +      /* Check if PTRACE_GETREGSET works.  */
> +      if (ptrace (PTRACE_GETREGSET, tid,
> +		  (unsigned int) NT_X86_XSTATE, &iov) < 0)
> +	{
> +	  have_ptrace_getregset = TRIBOOL_FALSE;
> +	  *xcr0_storage = 0;
> +	}
> +      else
> +	{
> +	  have_ptrace_getregset = TRIBOOL_TRUE;
> +
> +	  /* Get XCR0 from XSAVE extended state.  */
> +	  *xcr0_storage = xstateregs[(I386_LINUX_XSAVE_XCR0_OFFSET
> +				      / sizeof (uint64_t))];
> +
> +#ifdef __x86_64__
> +	  /* No MPX on x32.  */
> +	  if (is_64bit && is_x32)
> +	    *xcr0_storage &= ~X86_XSTATE_MPX;
> +#endif /* __x86_64__ */
> +
> +	  *xsave_layout_storage
> +	    = x86_fetch_xsave_layout (*xcr0_storage, x86_xsave_length ());
> +	}
> +    }
> +
> +  /* Check the native XCR0 only if PTRACE_GETREGSET is available.  If
> +     PTRACE_GETREGSET is not available then set xcr0_features_bits to
> +     zero so that the "no-features" descriptions are returned by the
> +     code below.  */
> +  uint64_t xcr0_features_bits;
> +  if (have_ptrace_getregset == TRIBOOL_TRUE)
> +    xcr0_features_bits = *xcr0_storage & X86_XSTATE_ALL_MASK;
> +  else
> +    xcr0_features_bits = 0;
> +
> +#ifdef __x86_64__
> +  if (is_64bit)
> +    return amd64_linux_read_description (xcr0_features_bits, is_x32);
> +  else
> +#endif
> +    return i386_linux_read_description (xcr0_features_bits);
> +}
> +
> +#endif /* !IN_PROCESS_AGENT */
> diff --git a/gdb/nat/x86-linux-tdesc.h b/gdb/nat/x86-linux-tdesc.h
> new file mode 100644
> index 00000000000..4035c49fc06
> --- /dev/null
> +++ b/gdb/nat/x86-linux-tdesc.h
> @@ -0,0 +1,54 @@
> +/* Target description related code for GNU/Linux x86 (i386 and x86-64).
> +
> +   Copyright (C) 2024 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   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/>.  */
> +
> +#ifndef NAT_X86_LINUX_TDESC_H
> +#define NAT_X86_LINUX_TDESC_H
> +
> +#include "gdbsupport/function-view.h"
> +
> +struct target_desc;
> +struct x86_xsave_layout;
> +
> +/* Return the target description for Linux thread TID.
> +
> +   The storage pointed to by XCR0_STORAGE AND XSAVE_LAYOUT_STORAGE
> must

s/AND/and

> +   exist until the program (GDB or gdbserver) terminates, this storage is
> +   used to cache the xcr0 and xsave layout values.  The values pointed to
> +   by these arguments are only updated at most once, the first time this
> +   function is called.
> +
> +   This function returns a target description based on the extracted xcr0
> +   value along with other characteristics of the thread identified by TID.
> +
> +   This function can return nullptr if we encounter a machine configuration
> +   for which a target_desc cannot be created.  Ideally this would not be
> +   the case, we should be able to create a target description for every
> +   possible machine configuration.  See amd64_linux_read_description and
> +   i386_linux_read_description for cases when nullptr might be
> +   returned.
> +
> +   ERROR_MSG is using in an error() call if we try to create a target
> +   description for a 64-bit process but this is a 32-bit build of GDB.  */
> +
> +extern const target_desc *
> +x86_linux_tdesc_for_tid (int tid, const char *error_msg,
> +			 uint64_t *xcr0_storage,
> +			 x86_xsave_layout *xsave_layout_storage);
> +
> +#endif /* NAT_X86_LINUX_TDESC_H */
> diff --git a/gdb/x86-linux-nat.c b/gdb/x86-linux-nat.c
> index ed76c1760bc..7c97f84ef56 100644
> --- a/gdb/x86-linux-nat.c
> +++ b/gdb/x86-linux-nat.c
> @@ -41,6 +41,7 @@
>  #include "nat/x86-linux.h"
>  #include "nat/x86-linux-dregs.h"
>  #include "nat/linux-ptrace.h"
> +#include "nat/x86-linux-tdesc.h"
> 
>  /* linux_nat_target::low_new_fork implementation.  */
> 
> @@ -95,93 +96,18 @@ x86_linux_nat_target::post_startup_inferior (ptid_t ptid)
>  const struct target_desc *
>  x86_linux_nat_target::read_description ()
>  {
> -  int tid;
> -  int is_64bit = 0;
> -#ifdef __x86_64__
> -  int is_x32;
> -#endif
> -  static uint64_t xcr0;
> -  uint64_t xcr0_features_bits;
> +  static uint64_t xcr0_storage;
> 
>    if (inferior_ptid == null_ptid)
>      return this->beneath ()->read_description ();
> 
> -  tid = inferior_ptid.pid ();
> -
> -#ifdef __x86_64__
> -
> -  x86_linux_arch_size arch_size = x86_linux_ptrace_get_arch_size (tid);
> -  is_64bit = arch_size.is_64bit ();
> -  is_x32 = arch_size.is_x32 ();
> -
> -  if (sizeof (void *) == 4 && is_64bit && !is_x32)
> -    error (_("Can't debug 64-bit process with 32-bit GDB"));
> -
> -#elif HAVE_PTRACE_GETFPXREGS
> -  if (have_ptrace_getfpxregs == TRIBOOL_UNKNOWN)
> -    {
> -      elf_fpxregset_t fpxregs;
> -
> -      if (ptrace (PTRACE_GETFPXREGS, tid, 0, (int) &fpxregs) < 0)
> -	{
> -	  have_ptrace_getfpxregs = TRIBOOL_FALSE;
> -	  have_ptrace_getregset = TRIBOOL_FALSE;
> -	  return i386_linux_read_description (X86_XSTATE_X87_MASK);
> -	}
> -    }
> -#endif
> -
> -  if (have_ptrace_getregset == TRIBOOL_UNKNOWN)
> -    {
> -      uint64_t xstateregs[(X86_XSTATE_SSE_SIZE / sizeof (uint64_t))];
> -      struct iovec iov;
> -
> -      iov.iov_base = xstateregs;
> -      iov.iov_len = sizeof (xstateregs);
> -
> -      /* Check if PTRACE_GETREGSET works.  */
> -      if (ptrace (PTRACE_GETREGSET, tid,
> -		  (unsigned int) NT_X86_XSTATE, &iov) < 0)
> -	have_ptrace_getregset = TRIBOOL_FALSE;
> -      else
> -	{
> -	  have_ptrace_getregset = TRIBOOL_TRUE;
> -
> -	  /* Get XCR0 from XSAVE extended state.  */
> -	  xcr0 = xstateregs[(I386_LINUX_XSAVE_XCR0_OFFSET
> -			     / sizeof (uint64_t))];
> -
> -	  m_xsave_layout = x86_fetch_xsave_layout (xcr0, x86_xsave_length ());
> -	}
> -    }
> -
> -  /* Check the native XCR0 only if PTRACE_GETREGSET is available.  If
> -     PTRACE_GETREGSET is not available then set xcr0_features_bits to
> -     zero so that the "no-features" descriptions are returned by the
> -     switches below.  */
> -  if (have_ptrace_getregset == TRIBOOL_TRUE)
> -    xcr0_features_bits = xcr0 & X86_XSTATE_ALL_MASK;
> -  else
> -    xcr0_features_bits = 0;
> +  int tid = inferior_ptid.pid ();
> 
> -  if (is_64bit)
> -    {
> -#ifdef __x86_64__
> -      return amd64_linux_read_description (xcr0_features_bits, is_x32);
> -#endif
> -    }
> -  else
> -    {
> -      const struct target_desc * tdesc
> -	= i386_linux_read_description (xcr0_features_bits);
> -
> -      if (tdesc == NULL)
> -	tdesc = i386_linux_read_description (X86_XSTATE_SSE_MASK);
> -
> -      return tdesc;
> -    }
> +  const char *error_msg
> +    = _("Can't debug 64-bit process with 32-bit GDB");

In gdbserver we pass "Can't debug 64-bit process with 32-bit GDBserver".
One could argue that something common like
"Can't debug 64-bit process with 32-bit GDB/gdbserver" is good enough.
Then we could avoid the function argument. Though being more explicit
can also be viewed as helpful for the remote scenario.

(Maybe this is solved by another comment below).

> 
> -  gdb_assert_not_reached ("failed to return tdesc");
> +  return x86_linux_tdesc_for_tid (tid, error_msg, &xcr0_storage,
> +				  &this->m_xsave_layout);
>  }
> 
> 
> 
> 
> diff --git a/gdbserver/configure.srv b/gdbserver/configure.srv
> index 8e882d2159b..538845b96d5 100644
> --- a/gdbserver/configure.srv
> +++ b/gdbserver/configure.srv
> @@ -110,6 +110,7 @@ case "${gdbserver_host}" in
>  			srv_tgtobj="${srv_tgtobj} nat/x86-linux.o"
>  			srv_tgtobj="${srv_tgtobj} nat/x86-linux-dregs.o"
>  			srv_tgtobj="${srv_tgtobj} nat/i386-linux.o"
> +			srv_tgtobj="${srv_tgtobj} nat/x86-linux-tdesc.o"
>  			srv_linux_usrregs=yes
>  			srv_linux_regsets=yes
>  			srv_linux_thread_db=yes
> @@ -372,6 +373,7 @@ case "${gdbserver_host}" in
>  			srv_tgtobj="${srv_tgtobj} nat/linux-btrace.o"
>  			srv_tgtobj="${srv_tgtobj} nat/x86-linux.o"
>  			srv_tgtobj="${srv_tgtobj} nat/x86-linux-dregs.o"
> +			srv_tgtobj="${srv_tgtobj} nat/x86-linux-tdesc.o"
>  			srv_tgtobj="${srv_tgtobj} nat/amd64-linux-siginfo.o"
>  			srv_linux_usrregs=yes # This is for i386 progs.
>  			srv_linux_regsets=yes
> diff --git a/gdbserver/i387-fp.cc b/gdbserver/i387-fp.cc
> index 62cafd87204..4d8bcb5edfa 100644
> --- a/gdbserver/i387-fp.cc
> +++ b/gdbserver/i387-fp.cc
> @@ -21,7 +21,7 @@
>  #include "nat/x86-xstate.h"
> 
>  /* Default to SSE.  */
> -static unsigned long long x86_xcr0 = X86_XSTATE_SSE_MASK;
> +static uint64_t x86_xcr0 = X86_XSTATE_SSE_MASK;
> 
>  static const int num_mpx_bnd_registers = 4;
>  static const int num_mpx_cfg_registers = 2;
> @@ -944,9 +944,8 @@ i387_xsave_to_cache (struct regcache *regcache, const
> void *buf)
> 
>  /* See i387-fp.h.  */
> 
> -void
> -i387_set_xsave_mask (uint64_t xcr0, int len)
> +std::pair<uint64_t *, x86_xsave_layout *>
> +i387_get_xsave_storage ()
>  {
> -  x86_xcr0 = xcr0;
> -  xsave_layout = x86_fetch_xsave_layout (xcr0, len);
> +  return { &x86_xcr0, &xsave_layout };
>  }
> diff --git a/gdbserver/i387-fp.h b/gdbserver/i387-fp.h
> index 450466efb75..4ee21da8461 100644
> --- a/gdbserver/i387-fp.h
> +++ b/gdbserver/i387-fp.h
> @@ -19,6 +19,8 @@
>  #ifndef GDBSERVER_I387_FP_H
>  #define GDBSERVER_I387_FP_H
> 
> +struct x86_xsave_layout;

Do we really save much from not including the header and doing
a forward declaration? But I guess GDB is already super inconsistent
on that front.

> +
>  void i387_cache_to_fsave (struct regcache *regcache, void *buf);
>  void i387_fsave_to_cache (struct regcache *regcache, const void *buf);
> 
> @@ -30,6 +32,6 @@ void i387_xsave_to_cache (struct regcache *regcache,
> const void *buf);
> 
>  /* Set the XSAVE mask and fetch the XSAVE layout via CPUID.  */
> 
> -void i387_set_xsave_mask (uint64_t xcr0, int len);
> +std::pair<uint64_t *, x86_xsave_layout *> i387_get_xsave_storage ();
> 
>  #endif /* GDBSERVER_I387_FP_H */
> diff --git a/gdbserver/linux-amd64-ipa.cc b/gdbserver/linux-amd64-ipa.cc
> index a6346750f49..df5e6aca081 100644
> --- a/gdbserver/linux-amd64-ipa.cc
> +++ b/gdbserver/linux-amd64-ipa.cc
> @@ -22,6 +22,7 @@
>  #include "tracepoint.h"
>  #include "linux-x86-tdesc.h"
>  #include "gdbsupport/x86-xstate.h"
> +#include "arch/amd64-linux-tdesc.h"
> 
>  /* fast tracepoints collect registers.  */
> 
> diff --git a/gdbserver/linux-i386-ipa.cc b/gdbserver/linux-i386-ipa.cc
> index 8f14e0937d4..aa346fc9bc3 100644
> --- a/gdbserver/linux-i386-ipa.cc
> +++ b/gdbserver/linux-i386-ipa.cc
> @@ -22,6 +22,7 @@
>  #include "tracepoint.h"
>  #include "linux-x86-tdesc.h"
>  #include "gdbsupport/x86-xstate.h"
> +#include "arch/i386-linux-tdesc.h"
> 
>  /* GDB register numbers.  */
> 
> diff --git a/gdbserver/linux-x86-low.cc b/gdbserver/linux-x86-low.cc
> index 2d99a82f566..1c3d55f17fc 100644
> --- a/gdbserver/linux-x86-low.cc
> +++ b/gdbserver/linux-x86-low.cc
> @@ -29,10 +29,13 @@
> 
>  #ifdef __x86_64__
>  #include "nat/amd64-linux-siginfo.h"
> +#include "arch/amd64-linux-tdesc.h"
>  #else
>  #include "nat/i386-linux.h"
>  #endif
> 
> +#include "arch/i386-linux-tdesc.h"
> +
>  #include "gdb_proc_service.h"
>  /* Don't include elf/common.h if linux/elf.h got included by
>     gdb_proc_service.h.  */
> @@ -48,6 +51,7 @@
>  #include "nat/x86-linux.h"
>  #include "nat/x86-linux-dregs.h"
>  #include "linux-x86-tdesc.h"
> +#include "nat/x86-linux-tdesc.h"
> 
>  #ifdef __x86_64__
>  static target_desc_up tdesc_amd64_linux_no_xml;
> @@ -836,29 +840,12 @@ static int use_xml;
>  /* Get Linux/x86 target description from running target.  */
> 
>  static const struct target_desc *
> -x86_linux_read_description (void)
> +x86_linux_read_description ()
>  {
> -  unsigned int machine;
> -  int is_elf64;
> -  int xcr0_features;
> -  int tid;
> -  static uint64_t xcr0;
> -  static int xsave_len;
> -  struct regset_info *regset;
> -
> -  tid = lwpid_of (current_thread);
> +  int tid = lwpid_of (current_thread);
> 
> -  is_elf64 = linux_pid_exe_is_elf_64_file (tid, &machine);
> -
> -  if (sizeof (void *) == 4)
> -    {
> -      if (is_elf64 > 0)
> -       error (_("Can't debug 64-bit process with 32-bit GDBserver"));
> -#ifndef __x86_64__
> -      else if (machine == EM_X86_64)
> -       error (_("Can't debug x86-64 process with 32-bit GDBserver"));
> -#endif
> -    }
> +  const char *error_msg
> +    = _("Can't debug 64-bit process with 32-bit GDBserver");
> 
>    /* If we are not allowed to send an XML target description then we need
>       to use the hard-wired target descriptions.  This corresponds to GDB's
> @@ -868,103 +855,53 @@ x86_linux_read_description (void)
>       generate some alternative target descriptions.  */
>    if (!use_xml)
>      {
> +      x86_linux_arch_size arch_size = x86_linux_ptrace_get_arch_size (tid);
> +      bool is_64bit = arch_size.is_64bit ();
> +      bool is_x32 = arch_size.is_x32 ();
> +
> +      if (sizeof (void *) == 4 && is_64bit && !is_x32)
> +	error ("%s", error_msg);

I am stumbling a bit on these x32 checks. So, we do this check twice in code
now anyway? If we did this unconditionally here (and once in GDB code)
we could get rid of this check and the error_msg argument in
x86_linux_tdesc_for_tid(). And still have this code just twice.

Factoring it out into a separate function would also be something that comes
to mind, though it might be less easy to do, I didn't really check.

Or maybe I am missing something.

> +
>  #ifdef __x86_64__
> -      if (machine == EM_X86_64)
> +      if (is_64bit && !is_x32)
>  	return tdesc_amd64_linux_no_xml.get ();
>        else
>  #endif
>  	return tdesc_i386_linux_no_xml.get ();
>      }
> 
> -#if !defined __x86_64__ && defined HAVE_PTRACE_GETFPXREGS
> -  if (machine == EM_386 && have_ptrace_getfpxregs == TRIBOOL_UNKNOWN)
> -    {
> -      elf_fpxregset_t fpxregs;
> -
> -      if (ptrace (PTRACE_GETFPXREGS, tid, 0, (long) &fpxregs) < 0)
> -	{
> -	  have_ptrace_getfpxregs = TRIBOOL_FALSE;
> -	  have_ptrace_getregset = TRIBOOL_FALSE;
> -	  return i386_linux_read_description (X86_XSTATE_X87);
> -	}
> -      else
> -	have_ptrace_getfpxregs = TRIBOOL_TRUE;
> -    }
> -#endif
> -
> -  if (have_ptrace_getregset == TRIBOOL_UNKNOWN)
> -    {
> -      uint64_t xstateregs[(X86_XSTATE_SSE_SIZE / sizeof (uint64_t))];
> -      struct iovec iov;
> -
> -      iov.iov_base = xstateregs;
> -      iov.iov_len = sizeof (xstateregs);
> -
> -      /* Check if PTRACE_GETREGSET works.  */
> -      if (ptrace (PTRACE_GETREGSET, tid,
> -		  (unsigned int) NT_X86_XSTATE, (long) &iov) < 0)
> -	have_ptrace_getregset = TRIBOOL_FALSE;
> -      else
> -	{
> -	  have_ptrace_getregset = TRIBOOL_TRUE;
> -
> -	  /* Get XCR0 from XSAVE extended state.  */
> -	  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.  */
> -	  for (regset = x86_regsets;
> -	       regset->fill_function != NULL; regset++)
> -	    if (regset->get_request == PTRACE_GETREGSET)
> -	      regset->size = xsave_len;
> -	    else if (regset->type != GENERAL_REGS)
> -	      regset->size = 0;
> -	}
> -    }
> -
> -  /* Check the native XCR0 only if PTRACE_GETREGSET is available.  */
> -  xcr0_features = (have_ptrace_getregset == TRIBOOL_TRUE
> -		   && (xcr0 & X86_XSTATE_ALL_MASK));
> +  /* If have_ptrace_getregset is changed to true by calling
> +     x86_linux_tdesc_for_tid then we will perform some additional
> +     initialisation.  */
> +  bool have_ptrace_getregset_is_unknown
> +    = have_ptrace_getregset == TRIBOOL_UNKNOWN;
> 
> -  if (xcr0_features)
> -    i387_set_xsave_mask (xcr0, xsave_len);
> +  /* Get pointers to where we should store the xcr0 and xsave_layout
> +     values.  These will be filled in by x86_linux_tdesc_for_tid the first
> +     time that the function is called.  Subsequent calls will not modify
> +     the stored values.  */
> +  std::pair<uint64_t *, x86_xsave_layout *> storage
> +    = i387_get_xsave_storage ();
> 
> -  if (machine == EM_X86_64)
> -    {
> -#ifdef __x86_64__
> -      const target_desc *tdesc = NULL;
> +  const target_desc *tdesc
> +    = x86_linux_tdesc_for_tid (tid, error_msg, storage.first, storage.second);
> 
> -      if (xcr0_features)
> -	{
> -	  tdesc = amd64_linux_read_description (xcr0 & X86_XSTATE_ALL_MASK,
> -						!is_elf64);
> -	}
> -
> -      if (tdesc == NULL)
> -	tdesc = amd64_linux_read_description (X86_XSTATE_SSE_MASK,
> !is_elf64);
> -      return tdesc;
> -#endif
> -    }
> -  else
> +  if (have_ptrace_getregset_is_unknown
> +      && have_ptrace_getregset == TRIBOOL_TRUE)
>      {
> -      const target_desc *tdesc = NULL;
> -
> -      if (xcr0_features)
> -	  tdesc = i386_linux_read_description (xcr0 & X86_XSTATE_ALL_MASK);
> -
> -      if (tdesc == NULL)
> -	tdesc = i386_linux_read_description (X86_XSTATE_SSE);
> -
> -      return tdesc;
> +      int xsave_len = x86_xsave_length ();
> +
> +      /* Use PTRACE_GETREGSET if it is available.  */
> +      for (regset_info *regset = x86_regsets;
> +	   regset->fill_function != nullptr;
> +	   regset++)
> +	if (regset->get_request == PTRACE_GETREGSET)
> +	  regset->size = xsave_len;
> +	else if (regset->type != GENERAL_REGS)
> +	  regset->size = 0;

I know you copied this, but doesn't this somehow go against our coding
standards? Not having braces for loops that have more then one line?
Though we don't have this particular case written down it seems, only for
if statements.
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
  
Andrew Burgess June 5, 2024, 5:04 p.m. UTC | #2
"Willgerodt, Felix" <felix.willgerodt@intel.com> writes:

> Hi Andrew,
>
> I mainly have some nits and one actual remark (the x32 check).
> Thanks again for doing this and sorry for taking a bit to review this.

No problem.  I've not been pushing this series as I didn't want to land
this before the release branched off, I think this should be tested in
master for a while before making it into a release.

I have some follow up, mostly on the x32 checks...

>> +
>> +const target_desc *
>> +x86_linux_tdesc_for_tid (int tid, const char *error_msg,
>> +			 uint64_t *xcr0_storage,
>> +			 x86_xsave_layout *xsave_layout_storage)
>> +{
>> +#ifdef __x86_64__
>> +
>
> It looks weird to me that there is an empty line here but not for the others.

Fixed.

>> +#include "gdbsupport/function-view.h"
>> +
>> +struct target_desc;
>> +struct x86_xsave_layout;
>> +
>> +/* Return the target description for Linux thread TID.
>> +
>> +   The storage pointed to by XCR0_STORAGE AND XSAVE_LAYOUT_STORAGE
>> must
>
> s/AND/and

Fixed.

>> diff --git a/gdbserver/i387-fp.h b/gdbserver/i387-fp.h
>> index 450466efb75..4ee21da8461 100644
>> --- a/gdbserver/i387-fp.h
>> +++ b/gdbserver/i387-fp.h
>> @@ -19,6 +19,8 @@
>>  #ifndef GDBSERVER_I387_FP_H
>>  #define GDBSERVER_I387_FP_H
>> 
>> +struct x86_xsave_layout;
>
> Do we really save much from not including the header and doing
> a forward declaration? But I guess GDB is already super inconsistent
> on that front.

I don't know.  There's plenty of places where we forward declare like
this.  I thought we only added the header if we needed the contents of
the struct.  I've left this as is for now, but if it really bothers you
I can change it, I'm not that attached to this style.

>> @@ -836,29 +840,12 @@ static int use_xml;
>>  /* Get Linux/x86 target description from running target.  */
>> 
>>  static const struct target_desc *
>> -x86_linux_read_description (void)
>> +x86_linux_read_description ()
>>  {
>> -  unsigned int machine;
>> -  int is_elf64;
>> -  int xcr0_features;
>> -  int tid;
>> -  static uint64_t xcr0;
>> -  static int xsave_len;
>> -  struct regset_info *regset;
>> -
>> -  tid = lwpid_of (current_thread);
>> +  int tid = lwpid_of (current_thread);
>> 
>> -  is_elf64 = linux_pid_exe_is_elf_64_file (tid, &machine);
>> -
>> -  if (sizeof (void *) == 4)
>> -    {
>> -      if (is_elf64 > 0)
>> -       error (_("Can't debug 64-bit process with 32-bit GDBserver"));
>> -#ifndef __x86_64__
>> -      else if (machine == EM_X86_64)
>> -       error (_("Can't debug x86-64 process with 32-bit GDBserver"));
>> -#endif
>> -    }
>> +  const char *error_msg
>> +    = _("Can't debug 64-bit process with 32-bit GDBserver");
>> 
>>    /* If we are not allowed to send an XML target description then we need
>>       to use the hard-wired target descriptions.  This corresponds to GDB's
>> @@ -868,103 +855,53 @@ x86_linux_read_description (void)
>>       generate some alternative target descriptions.  */
>>    if (!use_xml)
>>      {
>> +      x86_linux_arch_size arch_size = x86_linux_ptrace_get_arch_size (tid);
>> +      bool is_64bit = arch_size.is_64bit ();
>> +      bool is_x32 = arch_size.is_x32 ();
>> +
>> +      if (sizeof (void *) == 4 && is_64bit && !is_x32)
>> +	error ("%s", error_msg);
>
> I am stumbling a bit on these x32 checks. So, we do this check twice in code
> now anyway? If we did this unconditionally here (and once in GDB code)
> we could get rid of this check and the error_msg argument in
> x86_linux_tdesc_for_tid(). And still have this code just twice.
>
> Factoring it out into a separate function would also be something that comes
> to mind, though it might be less easy to do, I didn't really check.
>
> Or maybe I am missing something.

No you've not missed anything, this is not ideal right now.

So my argument for duplicating this code is that the duplication is
inside the use_xml block.

One thing I'm planning to propose as a follow up to this series is to
removed the use_xml flag and all the associated code, including this
duplicate error check.  The use_xml flag is only set to 0 if a GDB
connects to gdbserver and doesn't offer the 'xmlRegisters' feature in
its qSupported packet, this means that GDB was compiled without XML
support.

My argument for removing this code (spoiler alert!) is going to be:

  1. Having two fixed tdesc to x86, with all its possible variants,
     doesn't make a lot of sense, the fixed tdesc are unlikely to be
     correct.

  2. GDB has had xml support for 10+ years now, so I don't think there's
     really any reason why users should be using a version of GDB
     without xml register support to connect to gdbserver.  One argument
     that I've heard is: "What about compiling GDB on small targets
     where the XML libraries are not available?"  My thinking is that
     you'd only compile on such a target in order to run GDB natively on
     that target.  You wouldn't then connect from that target to some
     remote.  If you want remote debug then compile on a full sized
     machine where the xml libraries are available and connect to the
     remote from there.

  3. I'd then remove the use_xml flag and all associated code, and add
     an error check: if a GDB connects to an x86 gdbserver, and GDB
     doesn't offer the 'xmlRegisters' feature, gdbserver would give an
     error and drop the connection.

With that in mind I've tried throughout this series to keep the use_xml
code separate from the reset of the code.

Then on a purely engineering level, it's not entirely clear what would
be a good way too share the code.  Moving the error check out of the
function isn't great, we then need to document that checking for this
error is a prerequisite, plus we'd still need to call
x86_linux_ptrace_get_arch_size within x86_linux_read_description as we
need the result for other things.

We could move the error check to a helper function, but either the
helper needs to call x86_linux_ptrace_get_arch_size itself, which means
we repeat that work, or the caller of the helper has to call
x86_linux_ptrace_get_arch_size, in which case the helper function is
really just a wrapper around:

      if (sizeof (void *) == 4 && is_64bit && !is_x32)
	error (_("Can't debug 64-bit process with 32-bit GDBserver"));

which maybe isn't terrible, but isn't a huge win.

I haven't changed anything here for now, but if you really would like to
see some sharing happen then I can add a smaller helper function, it's
not much effort.

>> +
>> +      /* Use PTRACE_GETREGSET if it is available.  */
>> +      for (regset_info *regset = x86_regsets;
>> +	   regset->fill_function != nullptr;
>> +	   regset++)
>> +	if (regset->get_request == PTRACE_GETREGSET)
>> +	  regset->size = xsave_len;
>> +	else if (regset->type != GENERAL_REGS)
>> +	  regset->size = 0;
>
> I know you copied this, but doesn't this somehow go against our coding
> standards? Not having braces for loops that have more then one line?
> Though we don't have this particular case written down it seems, only for
> if statements.

Fixed.

I'll post another iteration of this series once I've finished updating
and testing this new version.

Thanks,
Andrew
  

Patch

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index bdb8a3683b5..13ca417706b 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -1550,8 +1550,10 @@  HFILES_NO_SRCDIR = \
 	arch/aarch64-insn.h \
 	arch/aarch64-mte-linux.h \
 	arch/aarch64-scalable-linux.h \
+	arch/amd64-linux-tdesc.h \
 	arch/arc.h \
 	arch/arm.h \
+	arch/i386-linux-tdesc.h \
 	arch/i386.h \
 	arch/loongarch.h \
 	arch/ppc-linux-common.h \
@@ -1608,6 +1610,7 @@  HFILES_NO_SRCDIR = \
 	nat/x86-gcc-cpuid.h \
 	nat/x86-linux.h \
 	nat/x86-linux-dregs.h \
+	nat/x86-linux-tdesc.h \
 	python/py-event.h \
 	python/py-events.h \
 	python/py-stopevent.h \
diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
index c52b0436872..f9647dce1e6 100644
--- a/gdb/amd64-linux-tdep.c
+++ b/gdb/amd64-linux-tdep.c
@@ -42,6 +42,7 @@ 
 #include "arch/amd64.h"
 #include "target-descriptions.h"
 #include "expop.h"
+#include "arch/amd64-linux-tdesc.h"
 
 /* The syscall's XML filename for i386.  */
 #define XML_SYSCALL_FILENAME_AMD64 "syscalls/amd64-linux.xml"
diff --git a/gdb/amd64-linux-tdep.h b/gdb/amd64-linux-tdep.h
index 2003dcda78f..0ec49e7fe03 100644
--- a/gdb/amd64-linux-tdep.h
+++ b/gdb/amd64-linux-tdep.h
@@ -43,12 +43,6 @@  extern struct target_desc *tdesc_x32_linux;
 extern struct target_desc *tdesc_x32_avx_linux;
 extern struct target_desc *tdesc_x32_avx_avx512_linux;
 
-/* Return the right amd64-linux target descriptions according to
-   XCR0_FEATURES_BIT and IS_X32.  */
-
-const target_desc *amd64_linux_read_description (uint64_t xcr0_features_bit,
-						 bool is_x32);
-
 /* Enum that defines the syscall identifiers for amd64 linux.
    Used for process record/replay, these will be translated into
    a gdb-canonical set of syscall ids in linux-record.c.  */
diff --git a/gdb/arch/amd64-linux-tdesc.h b/gdb/arch/amd64-linux-tdesc.h
new file mode 100644
index 00000000000..db425b60df6
--- /dev/null
+++ b/gdb/arch/amd64-linux-tdesc.h
@@ -0,0 +1,30 @@ 
+/*  Target description related code for GNU/Linux x86-64.
+
+   Copyright (C) 2024 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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/>.  */
+
+#ifndef ARCH_AMD64_LINUX_TDESC_H
+#define ARCH_AMD64_LINUX_TDESC_H
+
+struct target_desc;
+
+/* Return the AMD64 target descriptions corresponding to XCR0 and IS_X32.  */
+
+extern const target_desc *amd64_linux_read_description (uint64_t xcr0,
+							bool is_x32);
+
+#endif /* ARCH_AMD64_LINUX_TDESC_H */
diff --git a/gdb/arch/i386-linux-tdesc.h b/gdb/arch/i386-linux-tdesc.h
new file mode 100644
index 00000000000..0b736337a75
--- /dev/null
+++ b/gdb/arch/i386-linux-tdesc.h
@@ -0,0 +1,29 @@ 
+/* Target description related code for GNU/Linux i386.
+
+   Copyright (C) 2024 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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/>.  */
+
+#ifndef ARCH_I386_LINUX_TDESC_H
+#define ARCH_I386_LINUX_TDESC_H
+
+struct target_desc;
+
+/* Return the i386 target description corresponding to XCR0.  */
+
+extern const struct target_desc *i386_linux_read_description (uint64_t xcr0);
+
+#endif /* ARCH_I386_LINUX_TDESC_H */
diff --git a/gdb/configure.nat b/gdb/configure.nat
index 25268bb268b..a30b07ef8c5 100644
--- a/gdb/configure.nat
+++ b/gdb/configure.nat
@@ -256,7 +256,8 @@  case ${gdb_host} in
 		NATDEPFILES="${NATDEPFILES} x86-nat.o nat/x86-dregs.o \
 		nat/x86-xstate.o \
 		i386-linux-nat.o x86-linux-nat.o nat/linux-btrace.o \
-		nat/x86-linux.o nat/x86-linux-dregs.o nat/i386-linux.o"
+		nat/x86-linux.o nat/x86-linux-dregs.o nat/i386-linux.o \
+		nat/x86-linux-tdesc.o"
 		;;
 	    ia64)
 		# Host: Intel IA-64 running GNU/Linux
@@ -322,7 +323,7 @@  case ${gdb_host} in
 		NATDEPFILES="${NATDEPFILES} x86-nat.o nat/x86-dregs.o \
 		nat/x86-xstate.o amd64-nat.o amd64-linux-nat.o x86-linux-nat.o \
 		nat/linux-btrace.o \
-		nat/x86-linux.o nat/x86-linux-dregs.o \
+		nat/x86-linux.o nat/x86-linux-dregs.o nat/x86-linux-tdesc.o \
 		nat/amd64-linux-siginfo.o"
 		;;
 	    sparc)
diff --git a/gdb/i386-linux-tdep.c b/gdb/i386-linux-tdep.c
index 511e43f3b6f..6a7490680c1 100644
--- a/gdb/i386-linux-tdep.c
+++ b/gdb/i386-linux-tdep.c
@@ -40,6 +40,7 @@ 
 
 #include "i387-tdep.h"
 #include "gdbsupport/x86-xstate.h"
+#include "arch/i386-linux-tdesc.h"
 
 /* The syscall's XML filename for i386.  */
 #define XML_SYSCALL_FILENAME_I386 "syscalls/i386-linux.xml"
diff --git a/gdb/i386-linux-tdep.h b/gdb/i386-linux-tdep.h
index 07593c6a8ec..e8691cd778e 100644
--- a/gdb/i386-linux-tdep.h
+++ b/gdb/i386-linux-tdep.h
@@ -55,9 +55,6 @@  extern void i386_linux_report_signal_info (struct gdbarch *gdbarch,
 					   struct ui_out *uiout,
 					   enum gdb_signal siggnal);
 
-/* Return the target description according to XCR0.  */
-extern const struct target_desc *i386_linux_read_description (uint64_t xcr0);
-
 extern int i386_linux_gregset_reg_offset[];
 
 /* Return x86 siginfo type.  */
diff --git a/gdb/nat/x86-linux-tdesc.c b/gdb/nat/x86-linux-tdesc.c
new file mode 100644
index 00000000000..b065928f8ba
--- /dev/null
+++ b/gdb/nat/x86-linux-tdesc.c
@@ -0,0 +1,129 @@ 
+/* Target description related code for GNU/Linux x86 (i386 and x86-64).
+
+   Copyright (C) 2024 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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/>.  */
+
+#include "nat/x86-linux-tdesc.h"
+#ifdef __x86_64__
+#include "arch/amd64.h"
+#include "arch/amd64-linux-tdesc.h"
+#endif
+#include "arch/i386.h"
+#include "arch/i386-linux-tdesc.h"
+
+#include "nat/x86-linux.h"
+#include "nat/gdb_ptrace.h"
+#include "nat/x86-xstate.h"
+#include "gdbsupport/x86-xstate.h"
+
+#ifndef __x86_64__
+#include <sys/procfs.h>
+#include "nat/i386-linux.h"
+#endif
+
+#include <sys/uio.h>
+#include <elf.h>
+
+#ifndef IN_PROCESS_AGENT
+
+/* See nat/x86-linux-tdesc.h.  */
+
+const target_desc *
+x86_linux_tdesc_for_tid (int tid, const char *error_msg,
+			 uint64_t *xcr0_storage,
+			 x86_xsave_layout *xsave_layout_storage)
+{
+#ifdef __x86_64__
+
+  x86_linux_arch_size arch_size = x86_linux_ptrace_get_arch_size (tid);
+  bool is_64bit = arch_size.is_64bit ();
+  bool is_x32 = arch_size.is_x32 ();
+
+  if (sizeof (void *) == 4 && is_64bit && !is_x32)
+    error ("%s", error_msg);
+
+#elif HAVE_PTRACE_GETFPXREGS
+  if (have_ptrace_getfpxregs == TRIBOOL_UNKNOWN)
+    {
+      elf_fpxregset_t fpxregs;
+
+      if (ptrace (PTRACE_GETFPXREGS, tid, 0, (int) &fpxregs) < 0)
+	{
+	  have_ptrace_getfpxregs = TRIBOOL_FALSE;
+	  have_ptrace_getregset = TRIBOOL_FALSE;
+	}
+      else
+	have_ptrace_getfpxregs = TRIBOOL_TRUE;
+    }
+
+  if (have_ptrace_getfpxregs == TRIBOOL_FALSE)
+    return i386_linux_read_description (X86_XSTATE_X87_MASK);
+#endif
+
+  if (have_ptrace_getregset == TRIBOOL_UNKNOWN)
+    {
+      uint64_t xstateregs[(X86_XSTATE_SSE_SIZE / sizeof (uint64_t))];
+      struct iovec iov;
+
+      iov.iov_base = xstateregs;
+      iov.iov_len = sizeof (xstateregs);
+
+      /* Check if PTRACE_GETREGSET works.  */
+      if (ptrace (PTRACE_GETREGSET, tid,
+		  (unsigned int) NT_X86_XSTATE, &iov) < 0)
+	{
+	  have_ptrace_getregset = TRIBOOL_FALSE;
+	  *xcr0_storage = 0;
+	}
+      else
+	{
+	  have_ptrace_getregset = TRIBOOL_TRUE;
+
+	  /* Get XCR0 from XSAVE extended state.  */
+	  *xcr0_storage = xstateregs[(I386_LINUX_XSAVE_XCR0_OFFSET
+				      / sizeof (uint64_t))];
+
+#ifdef __x86_64__
+	  /* No MPX on x32.  */
+	  if (is_64bit && is_x32)
+	    *xcr0_storage &= ~X86_XSTATE_MPX;
+#endif /* __x86_64__ */
+
+	  *xsave_layout_storage
+	    = x86_fetch_xsave_layout (*xcr0_storage, x86_xsave_length ());
+	}
+    }
+
+  /* Check the native XCR0 only if PTRACE_GETREGSET is available.  If
+     PTRACE_GETREGSET is not available then set xcr0_features_bits to
+     zero so that the "no-features" descriptions are returned by the
+     code below.  */
+  uint64_t xcr0_features_bits;
+  if (have_ptrace_getregset == TRIBOOL_TRUE)
+    xcr0_features_bits = *xcr0_storage & X86_XSTATE_ALL_MASK;
+  else
+    xcr0_features_bits = 0;
+
+#ifdef __x86_64__
+  if (is_64bit)
+    return amd64_linux_read_description (xcr0_features_bits, is_x32);
+  else
+#endif
+    return i386_linux_read_description (xcr0_features_bits);
+}
+
+#endif /* !IN_PROCESS_AGENT */
diff --git a/gdb/nat/x86-linux-tdesc.h b/gdb/nat/x86-linux-tdesc.h
new file mode 100644
index 00000000000..4035c49fc06
--- /dev/null
+++ b/gdb/nat/x86-linux-tdesc.h
@@ -0,0 +1,54 @@ 
+/* Target description related code for GNU/Linux x86 (i386 and x86-64).
+
+   Copyright (C) 2024 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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/>.  */
+
+#ifndef NAT_X86_LINUX_TDESC_H
+#define NAT_X86_LINUX_TDESC_H
+
+#include "gdbsupport/function-view.h"
+
+struct target_desc;
+struct x86_xsave_layout;
+
+/* Return the target description for Linux thread TID.
+
+   The storage pointed to by XCR0_STORAGE AND XSAVE_LAYOUT_STORAGE must
+   exist until the program (GDB or gdbserver) terminates, this storage is
+   used to cache the xcr0 and xsave layout values.  The values pointed to
+   by these arguments are only updated at most once, the first time this
+   function is called.
+
+   This function returns a target description based on the extracted xcr0
+   value along with other characteristics of the thread identified by TID.
+
+   This function can return nullptr if we encounter a machine configuration
+   for which a target_desc cannot be created.  Ideally this would not be
+   the case, we should be able to create a target description for every
+   possible machine configuration.  See amd64_linux_read_description and
+   i386_linux_read_description for cases when nullptr might be
+   returned.
+
+   ERROR_MSG is using in an error() call if we try to create a target
+   description for a 64-bit process but this is a 32-bit build of GDB.  */
+
+extern const target_desc *
+x86_linux_tdesc_for_tid (int tid, const char *error_msg,
+			 uint64_t *xcr0_storage,
+			 x86_xsave_layout *xsave_layout_storage);
+
+#endif /* NAT_X86_LINUX_TDESC_H */
diff --git a/gdb/x86-linux-nat.c b/gdb/x86-linux-nat.c
index ed76c1760bc..7c97f84ef56 100644
--- a/gdb/x86-linux-nat.c
+++ b/gdb/x86-linux-nat.c
@@ -41,6 +41,7 @@ 
 #include "nat/x86-linux.h"
 #include "nat/x86-linux-dregs.h"
 #include "nat/linux-ptrace.h"
+#include "nat/x86-linux-tdesc.h"
 
 /* linux_nat_target::low_new_fork implementation.  */
 
@@ -95,93 +96,18 @@  x86_linux_nat_target::post_startup_inferior (ptid_t ptid)
 const struct target_desc *
 x86_linux_nat_target::read_description ()
 {
-  int tid;
-  int is_64bit = 0;
-#ifdef __x86_64__
-  int is_x32;
-#endif
-  static uint64_t xcr0;
-  uint64_t xcr0_features_bits;
+  static uint64_t xcr0_storage;
 
   if (inferior_ptid == null_ptid)
     return this->beneath ()->read_description ();
 
-  tid = inferior_ptid.pid ();
-
-#ifdef __x86_64__
-
-  x86_linux_arch_size arch_size = x86_linux_ptrace_get_arch_size (tid);
-  is_64bit = arch_size.is_64bit ();
-  is_x32 = arch_size.is_x32 ();
-
-  if (sizeof (void *) == 4 && is_64bit && !is_x32)
-    error (_("Can't debug 64-bit process with 32-bit GDB"));
-
-#elif HAVE_PTRACE_GETFPXREGS
-  if (have_ptrace_getfpxregs == TRIBOOL_UNKNOWN)
-    {
-      elf_fpxregset_t fpxregs;
-
-      if (ptrace (PTRACE_GETFPXREGS, tid, 0, (int) &fpxregs) < 0)
-	{
-	  have_ptrace_getfpxregs = TRIBOOL_FALSE;
-	  have_ptrace_getregset = TRIBOOL_FALSE;
-	  return i386_linux_read_description (X86_XSTATE_X87_MASK);
-	}
-    }
-#endif
-
-  if (have_ptrace_getregset == TRIBOOL_UNKNOWN)
-    {
-      uint64_t xstateregs[(X86_XSTATE_SSE_SIZE / sizeof (uint64_t))];
-      struct iovec iov;
-
-      iov.iov_base = xstateregs;
-      iov.iov_len = sizeof (xstateregs);
-
-      /* Check if PTRACE_GETREGSET works.  */
-      if (ptrace (PTRACE_GETREGSET, tid,
-		  (unsigned int) NT_X86_XSTATE, &iov) < 0)
-	have_ptrace_getregset = TRIBOOL_FALSE;
-      else
-	{
-	  have_ptrace_getregset = TRIBOOL_TRUE;
-
-	  /* Get XCR0 from XSAVE extended state.  */
-	  xcr0 = xstateregs[(I386_LINUX_XSAVE_XCR0_OFFSET
-			     / sizeof (uint64_t))];
-
-	  m_xsave_layout = x86_fetch_xsave_layout (xcr0, x86_xsave_length ());
-	}
-    }
-
-  /* Check the native XCR0 only if PTRACE_GETREGSET is available.  If
-     PTRACE_GETREGSET is not available then set xcr0_features_bits to
-     zero so that the "no-features" descriptions are returned by the
-     switches below.  */
-  if (have_ptrace_getregset == TRIBOOL_TRUE)
-    xcr0_features_bits = xcr0 & X86_XSTATE_ALL_MASK;
-  else
-    xcr0_features_bits = 0;
+  int tid = inferior_ptid.pid ();
 
-  if (is_64bit)
-    {
-#ifdef __x86_64__
-      return amd64_linux_read_description (xcr0_features_bits, is_x32);
-#endif
-    }
-  else
-    {
-      const struct target_desc * tdesc
-	= i386_linux_read_description (xcr0_features_bits);
-
-      if (tdesc == NULL)
-	tdesc = i386_linux_read_description (X86_XSTATE_SSE_MASK);
-
-      return tdesc;
-    }
+  const char *error_msg
+    = _("Can't debug 64-bit process with 32-bit GDB");
 
-  gdb_assert_not_reached ("failed to return tdesc");
+  return x86_linux_tdesc_for_tid (tid, error_msg, &xcr0_storage,
+				  &this->m_xsave_layout);
 }
 
 
diff --git a/gdbserver/configure.srv b/gdbserver/configure.srv
index 8e882d2159b..538845b96d5 100644
--- a/gdbserver/configure.srv
+++ b/gdbserver/configure.srv
@@ -110,6 +110,7 @@  case "${gdbserver_host}" in
 			srv_tgtobj="${srv_tgtobj} nat/x86-linux.o"
 			srv_tgtobj="${srv_tgtobj} nat/x86-linux-dregs.o"
 			srv_tgtobj="${srv_tgtobj} nat/i386-linux.o"
+			srv_tgtobj="${srv_tgtobj} nat/x86-linux-tdesc.o"
 			srv_linux_usrregs=yes
 			srv_linux_regsets=yes
 			srv_linux_thread_db=yes
@@ -372,6 +373,7 @@  case "${gdbserver_host}" in
 			srv_tgtobj="${srv_tgtobj} nat/linux-btrace.o"
 			srv_tgtobj="${srv_tgtobj} nat/x86-linux.o"
 			srv_tgtobj="${srv_tgtobj} nat/x86-linux-dregs.o"
+			srv_tgtobj="${srv_tgtobj} nat/x86-linux-tdesc.o"
 			srv_tgtobj="${srv_tgtobj} nat/amd64-linux-siginfo.o"
 			srv_linux_usrregs=yes # This is for i386 progs.
 			srv_linux_regsets=yes
diff --git a/gdbserver/i387-fp.cc b/gdbserver/i387-fp.cc
index 62cafd87204..4d8bcb5edfa 100644
--- a/gdbserver/i387-fp.cc
+++ b/gdbserver/i387-fp.cc
@@ -21,7 +21,7 @@ 
 #include "nat/x86-xstate.h"
 
 /* Default to SSE.  */
-static unsigned long long x86_xcr0 = X86_XSTATE_SSE_MASK;
+static uint64_t x86_xcr0 = X86_XSTATE_SSE_MASK;
 
 static const int num_mpx_bnd_registers = 4;
 static const int num_mpx_cfg_registers = 2;
@@ -944,9 +944,8 @@  i387_xsave_to_cache (struct regcache *regcache, const void *buf)
 
 /* See i387-fp.h.  */
 
-void
-i387_set_xsave_mask (uint64_t xcr0, int len)
+std::pair<uint64_t *, x86_xsave_layout *>
+i387_get_xsave_storage ()
 {
-  x86_xcr0 = xcr0;
-  xsave_layout = x86_fetch_xsave_layout (xcr0, len);
+  return { &x86_xcr0, &xsave_layout };
 }
diff --git a/gdbserver/i387-fp.h b/gdbserver/i387-fp.h
index 450466efb75..4ee21da8461 100644
--- a/gdbserver/i387-fp.h
+++ b/gdbserver/i387-fp.h
@@ -19,6 +19,8 @@ 
 #ifndef GDBSERVER_I387_FP_H
 #define GDBSERVER_I387_FP_H
 
+struct x86_xsave_layout;
+
 void i387_cache_to_fsave (struct regcache *regcache, void *buf);
 void i387_fsave_to_cache (struct regcache *regcache, const void *buf);
 
@@ -30,6 +32,6 @@  void i387_xsave_to_cache (struct regcache *regcache, const void *buf);
 
 /* Set the XSAVE mask and fetch the XSAVE layout via CPUID.  */
 
-void i387_set_xsave_mask (uint64_t xcr0, int len);
+std::pair<uint64_t *, x86_xsave_layout *> i387_get_xsave_storage ();
 
 #endif /* GDBSERVER_I387_FP_H */
diff --git a/gdbserver/linux-amd64-ipa.cc b/gdbserver/linux-amd64-ipa.cc
index a6346750f49..df5e6aca081 100644
--- a/gdbserver/linux-amd64-ipa.cc
+++ b/gdbserver/linux-amd64-ipa.cc
@@ -22,6 +22,7 @@ 
 #include "tracepoint.h"
 #include "linux-x86-tdesc.h"
 #include "gdbsupport/x86-xstate.h"
+#include "arch/amd64-linux-tdesc.h"
 
 /* fast tracepoints collect registers.  */
 
diff --git a/gdbserver/linux-i386-ipa.cc b/gdbserver/linux-i386-ipa.cc
index 8f14e0937d4..aa346fc9bc3 100644
--- a/gdbserver/linux-i386-ipa.cc
+++ b/gdbserver/linux-i386-ipa.cc
@@ -22,6 +22,7 @@ 
 #include "tracepoint.h"
 #include "linux-x86-tdesc.h"
 #include "gdbsupport/x86-xstate.h"
+#include "arch/i386-linux-tdesc.h"
 
 /* GDB register numbers.  */
 
diff --git a/gdbserver/linux-x86-low.cc b/gdbserver/linux-x86-low.cc
index 2d99a82f566..1c3d55f17fc 100644
--- a/gdbserver/linux-x86-low.cc
+++ b/gdbserver/linux-x86-low.cc
@@ -29,10 +29,13 @@ 
 
 #ifdef __x86_64__
 #include "nat/amd64-linux-siginfo.h"
+#include "arch/amd64-linux-tdesc.h"
 #else
 #include "nat/i386-linux.h"
 #endif
 
+#include "arch/i386-linux-tdesc.h"
+
 #include "gdb_proc_service.h"
 /* Don't include elf/common.h if linux/elf.h got included by
    gdb_proc_service.h.  */
@@ -48,6 +51,7 @@ 
 #include "nat/x86-linux.h"
 #include "nat/x86-linux-dregs.h"
 #include "linux-x86-tdesc.h"
+#include "nat/x86-linux-tdesc.h"
 
 #ifdef __x86_64__
 static target_desc_up tdesc_amd64_linux_no_xml;
@@ -836,29 +840,12 @@  static int use_xml;
 /* Get Linux/x86 target description from running target.  */
 
 static const struct target_desc *
-x86_linux_read_description (void)
+x86_linux_read_description ()
 {
-  unsigned int machine;
-  int is_elf64;
-  int xcr0_features;
-  int tid;
-  static uint64_t xcr0;
-  static int xsave_len;
-  struct regset_info *regset;
-
-  tid = lwpid_of (current_thread);
+  int tid = lwpid_of (current_thread);
 
-  is_elf64 = linux_pid_exe_is_elf_64_file (tid, &machine);
-
-  if (sizeof (void *) == 4)
-    {
-      if (is_elf64 > 0)
-       error (_("Can't debug 64-bit process with 32-bit GDBserver"));
-#ifndef __x86_64__
-      else if (machine == EM_X86_64)
-       error (_("Can't debug x86-64 process with 32-bit GDBserver"));
-#endif
-    }
+  const char *error_msg
+    = _("Can't debug 64-bit process with 32-bit GDBserver");
 
   /* If we are not allowed to send an XML target description then we need
      to use the hard-wired target descriptions.  This corresponds to GDB's
@@ -868,103 +855,53 @@  x86_linux_read_description (void)
      generate some alternative target descriptions.  */
   if (!use_xml)
     {
+      x86_linux_arch_size arch_size = x86_linux_ptrace_get_arch_size (tid);
+      bool is_64bit = arch_size.is_64bit ();
+      bool is_x32 = arch_size.is_x32 ();
+
+      if (sizeof (void *) == 4 && is_64bit && !is_x32)
+	error ("%s", error_msg);
+
 #ifdef __x86_64__
-      if (machine == EM_X86_64)
+      if (is_64bit && !is_x32)
 	return tdesc_amd64_linux_no_xml.get ();
       else
 #endif
 	return tdesc_i386_linux_no_xml.get ();
     }
 
-#if !defined __x86_64__ && defined HAVE_PTRACE_GETFPXREGS
-  if (machine == EM_386 && have_ptrace_getfpxregs == TRIBOOL_UNKNOWN)
-    {
-      elf_fpxregset_t fpxregs;
-
-      if (ptrace (PTRACE_GETFPXREGS, tid, 0, (long) &fpxregs) < 0)
-	{
-	  have_ptrace_getfpxregs = TRIBOOL_FALSE;
-	  have_ptrace_getregset = TRIBOOL_FALSE;
-	  return i386_linux_read_description (X86_XSTATE_X87);
-	}
-      else
-	have_ptrace_getfpxregs = TRIBOOL_TRUE;
-    }
-#endif
-
-  if (have_ptrace_getregset == TRIBOOL_UNKNOWN)
-    {
-      uint64_t xstateregs[(X86_XSTATE_SSE_SIZE / sizeof (uint64_t))];
-      struct iovec iov;
-
-      iov.iov_base = xstateregs;
-      iov.iov_len = sizeof (xstateregs);
-
-      /* Check if PTRACE_GETREGSET works.  */
-      if (ptrace (PTRACE_GETREGSET, tid,
-		  (unsigned int) NT_X86_XSTATE, (long) &iov) < 0)
-	have_ptrace_getregset = TRIBOOL_FALSE;
-      else
-	{
-	  have_ptrace_getregset = TRIBOOL_TRUE;
-
-	  /* Get XCR0 from XSAVE extended state.  */
-	  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.  */
-	  for (regset = x86_regsets;
-	       regset->fill_function != NULL; regset++)
-	    if (regset->get_request == PTRACE_GETREGSET)
-	      regset->size = xsave_len;
-	    else if (regset->type != GENERAL_REGS)
-	      regset->size = 0;
-	}
-    }
-
-  /* Check the native XCR0 only if PTRACE_GETREGSET is available.  */
-  xcr0_features = (have_ptrace_getregset == TRIBOOL_TRUE
-		   && (xcr0 & X86_XSTATE_ALL_MASK));
+  /* If have_ptrace_getregset is changed to true by calling
+     x86_linux_tdesc_for_tid then we will perform some additional
+     initialisation.  */
+  bool have_ptrace_getregset_is_unknown
+    = have_ptrace_getregset == TRIBOOL_UNKNOWN;
 
-  if (xcr0_features)
-    i387_set_xsave_mask (xcr0, xsave_len);
+  /* Get pointers to where we should store the xcr0 and xsave_layout
+     values.  These will be filled in by x86_linux_tdesc_for_tid the first
+     time that the function is called.  Subsequent calls will not modify
+     the stored values.  */
+  std::pair<uint64_t *, x86_xsave_layout *> storage
+    = i387_get_xsave_storage ();
 
-  if (machine == EM_X86_64)
-    {
-#ifdef __x86_64__
-      const target_desc *tdesc = NULL;
+  const target_desc *tdesc
+    = x86_linux_tdesc_for_tid (tid, error_msg, storage.first, storage.second);
 
-      if (xcr0_features)
-	{
-	  tdesc = amd64_linux_read_description (xcr0 & X86_XSTATE_ALL_MASK,
-						!is_elf64);
-	}
-
-      if (tdesc == NULL)
-	tdesc = amd64_linux_read_description (X86_XSTATE_SSE_MASK, !is_elf64);
-      return tdesc;
-#endif
-    }
-  else
+  if (have_ptrace_getregset_is_unknown
+      && have_ptrace_getregset == TRIBOOL_TRUE)
     {
-      const target_desc *tdesc = NULL;
-
-      if (xcr0_features)
-	  tdesc = i386_linux_read_description (xcr0 & X86_XSTATE_ALL_MASK);
-
-      if (tdesc == NULL)
-	tdesc = i386_linux_read_description (X86_XSTATE_SSE);
-
-      return tdesc;
+      int xsave_len = x86_xsave_length ();
+
+      /* Use PTRACE_GETREGSET if it is available.  */
+      for (regset_info *regset = x86_regsets;
+	   regset->fill_function != nullptr;
+	   regset++)
+	if (regset->get_request == PTRACE_GETREGSET)
+	  regset->size = xsave_len;
+	else if (regset->type != GENERAL_REGS)
+	  regset->size = 0;
     }
 
-  gdb_assert_not_reached ("failed to return tdesc");
+  return tdesc;
 }
 
 /* Update all the target description of all processes; a new GDB
diff --git a/gdbserver/linux-x86-tdesc.cc b/gdbserver/linux-x86-tdesc.cc
index cd3b5d80e37..af3735aa895 100644
--- a/gdbserver/linux-x86-tdesc.cc
+++ b/gdbserver/linux-x86-tdesc.cc
@@ -23,8 +23,10 @@ 
 #include "gdbsupport/x86-xstate.h"
 #ifdef __x86_64__
 #include "arch/amd64.h"
+#include "arch/amd64-linux-tdesc.h"
 #endif
 #include "x86-tdesc.h"
+#include "arch/i386-linux-tdesc.h"
 
 /* Return the right x86_linux_tdesc index for a given XCR0.  Return
    X86_TDESC_LAST if can't find a match.  */
diff --git a/gdbserver/linux-x86-tdesc.h b/gdbserver/linux-x86-tdesc.h
index f9561b129ae..576aaf5e165 100644
--- a/gdbserver/linux-x86-tdesc.h
+++ b/gdbserver/linux-x86-tdesc.h
@@ -46,11 +46,4 @@  int amd64_get_ipa_tdesc_idx (const struct target_desc *tdesc);
 
 const struct target_desc *i386_get_ipa_tdesc (int idx);
 
-#ifdef __x86_64__
-const struct target_desc *amd64_linux_read_description (uint64_t xcr0,
-							bool is_x32);
-#endif
-
-const struct target_desc *i386_linux_read_description (uint64_t xcr0);
-
 #endif /* GDBSERVER_LINUX_X86_TDESC_H */