RE: [PATCHv5 07/11] gdb/gdbserver: share some code relating to target description creation

["Willgerodt, Felix" <felix.willgerodt@intel.com>]

> -----Original Message-----
> From: Andrew Burgess <aburgess@redhat.com>
> Sent: Freitag, 26. April 2024 17:02
> To: gdb-patches@sourceware.org
> Cc: Andrew Burgess <aburgess@redhat.com>; Willgerodt, Felix
> <felix.willgerodt@intel.com>; John Baldwin <jhb@FreeBSD.org>
> Subject: [PATCHv5 07/11] gdb/gdbserver: share some code relating to target
> description creation
> 
> 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 the GDB way, the only changes are to
> allow for the sharing; we now have a callback function to call the
> first time that the xcr0 state is read, this allows for GDB and
> gdbserver to perform their own initialisation as needed, and
> additionally, the new function takes a pointer for where to cache the
> xcr0 value, this isn't needed for this commit, but will be useful in a
> later commit where gdbserver will want to read this cached xcr0
> value.
> 
> 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.
> ---
>  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            |   4 +-
>  gdb/i386-linux-tdep.c        |   1 +
>  gdb/i386-linux-tdep.h        |   3 -
>  gdb/nat/x86-linux-tdesc.c    | 124 ++++++++++++++++++++++++++++
>  gdb/nat/x86-linux-tdesc.h    |  60 ++++++++++++++
>  gdb/x86-linux-nat.c          |  91 ++++-----------------
>  gdbserver/configure.srv      |   2 +
>  gdbserver/linux-amd64-ipa.cc |   1 +
>  gdbserver/linux-i386-ipa.cc  |   1 +
>  gdbserver/linux-x86-low.cc   | 151 +++++++++++------------------------
>  gdbserver/linux-x86-tdesc.cc |   2 +
>  gdbserver/linux-x86-tdesc.h  |   7 --
>  17 files changed, 317 insertions(+), 199 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
> 
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 353e45b3cec..a24b2232daa 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -1549,8 +1549,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 \
> @@ -1606,6 +1608,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 9d560ac4fbf..bcb9868e79e 100644
> --- a/gdb/amd64-linux-tdep.c
> +++ b/gdb/amd64-linux-tdep.c
> @@ -41,6 +41,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 8b98511cef7..4bcc0696027 100644
> --- a/gdb/configure.nat
> +++ b/gdb/configure.nat
> @@ -256,7 +256,7 @@ 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/x86-linux.o nat/x86-linux-dregs.o nat/x86-linux-tdesc.o"
>  		;;
>  	    ia64)
>  		# Host: Intel IA-64 running GNU/Linux
> @@ -322,7 +322,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 44730f204db..78ebc99d3df 100644
> --- a/gdb/i386-linux-tdep.c
> +++ b/gdb/i386-linux-tdep.c
> @@ -39,6 +39,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..c2301e6a873
> --- /dev/null
> +++ b/gdb/nat/x86-linux-tdesc.c
> @@ -0,0 +1,124 @@
> +/* 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"
> +
> +#ifndef __x86_64__
> +#include <sys/procfs.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, enum tribool *have_ptrace_getregset,
> +			 gdb::function_view<void (uint64_t)> xcr0_init_cb,
> +			 const char *error_msg, uint64_t *xcr0_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 == -1)
> +    {
> +      elf_fpxregset_t fpxregs;
> +
> +      if (ptrace (PTRACE_GETFPXREGS, tid, 0, (int) &fpxregs) < 0)
> +	{
> +	  have_ptrace_getfpxregs = 0;
> +	  *have_ptrace_getregset = TRIBOOL_FALSE;

We tried PTRACE_GETFPXREGS, does that really indicate
PTRACE_GETREGSET is not supported? I realize you just moved this code,
but it feels weird to me. Then again this is Linux specific and it might actually
be the case.


> +	  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__ */
> +
> +	  xcr0_init_cb (*xcr0_storage);


Needing a callback made me scratch my head. What could be the reason
that GDB and gdbserver really want to do different things here?

Digging into the code made me see that GDB saves just the xsave_layout.
Gdbserver in addition saves xcr0, though I guess that might not even
be needed if you have a tdesc already (or the xsave_layout?).
And they are saving things in different locations (global in gdbserver,
member in GDB), but that could be done without a callback I think.

I realize that changing these things is a bigger tasks, that you might
not want to do at all or mix with this series. But can we avoid
a callback? E.g. by adding another return value, that only gdbserver
uses for now? With a TODO comment explaining this?



> +	}
> +    }
> +
> +  /* 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.  */
> +  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);
> +    }

Nit: I think we could just drop the brackets here.

> +  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..8c481609876
> --- /dev/null
> +++ b/gdb/nat/x86-linux-tdesc.h
> @@ -0,0 +1,60 @@
> +/* 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;
> +
> +/* Return the target description for Linux thread TID.
> +
> +   When *HAVE_PTRACE_GETREGSET is TRIBOOL_UNKNOWN then the current
> value of
> +   xcr0 is read using ptrace calls and stored into *XCR0_STORAGE.  Then
> +   XCR0_INIT_CB is called with the value of *XCR0_STORAGE and
> +   *HAVE_PTRACE_GETREGSET is set to TRIBOOL_TRUE.
> +
> +   If the attempt to read xcr0 using ptrace fails then *XCR0_STORAGE is set
> +   to zero and *HAVE_PTRACE_GETREGSET is set to TRIBOOL_FALSE.
> +
> +   The storage pointed to by XCR0_STORAGE must exist until the program
> +   terminates, this storage is used to cache the xcr0 value.  As such
> +   XCR0_INIT_CB will only be called once if xcr0 is successfully read using
> +   ptrace, or not at all if the ptrace call fails.
> +
> +   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, enum tribool *have_ptrace_getregset,
> +			 gdb::function_view<void (uint64_t)> xcr0_init_cb,
> +			 const char *error_msg, uint64_t *xcr0_storage);
> +
> +#endif /* NAT_X86_LINUX_TDESC_H */
> diff --git a/gdb/x86-linux-nat.c b/gdb/x86-linux-nat.c
> index 12965c7a21b..9fbccb28d21 100644
> --- a/gdb/x86-linux-nat.c
> +++ b/gdb/x86-linux-nat.c
> @@ -38,6 +38,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.  */
> 
> @@ -92,90 +93,26 @@ 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 ();
> -
> -#elif HAVE_PTRACE_GETFPXREGS
> -  if (have_ptrace_getfpxregs == -1)
> -    {
> -      elf_fpxregset_t fpxregs;
> -
> -      if (ptrace (PTRACE_GETFPXREGS, tid, 0, (int) &fpxregs) < 0)
> -	{
> -	  have_ptrace_getfpxregs = 0;
> -	  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.  */

Should we s/switches/code?

> -  if (have_ptrace_getregset == TRIBOOL_TRUE)
> -    xcr0_features_bits = xcr0 & X86_XSTATE_ALL_MASK;
> -  else
> -    xcr0_features_bits = 0;
> -
> -  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);
> +  int tid = inferior_ptid.pid ();
> 
> -      if (tdesc == NULL)
> -	tdesc = i386_linux_read_description (X86_XSTATE_SSE_MASK);
> +  const char *error_msg
> +    = _("Can't debug 64-bit process with 32-bit GDB");
> 
> -      return tdesc;
> -    }
> +  /* Callback that is triggered the first time x86_linux_tdesc_for_tid
> +     reads the xcr0 register.  Setup other bits of state.  */
> +  auto cb = [&] (uint64_t xcr0)
> +  {
> +    this->m_xsave_layout
> +      = x86_fetch_xsave_layout (xcr0, x86_xsave_length ());
> +  };
> 
> -  gdb_assert_not_reached ("failed to return tdesc");
> +  return x86_linux_tdesc_for_tid (tid, &have_ptrace_getregset, cb,
> +				  error_msg, &xcr0_storage);
>  }
> 
> 
> 
> 
> diff --git a/gdbserver/configure.srv b/gdbserver/configure.srv
> index 9e861a75088..7a2702d78bf 100644
> --- a/gdbserver/configure.srv
> +++ b/gdbserver/configure.srv
> @@ -109,6 +109,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_linux_usrregs=yes
>  			srv_linux_regsets=yes
>  			srv_linux_thread_db=yes
> @@ -371,6 +372,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/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 62612773a87..68d2f13537c 100644
> --- a/gdbserver/linux-x86-low.cc
> +++ b/gdbserver/linux-x86-low.cc
> @@ -29,8 +29,11 @@
> 
>  #ifdef __x86_64__
>  #include "nat/amd64-linux-siginfo.h"
> +#include "arch/amd64-linux-tdesc.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.  */
> @@ -46,6 +49,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;
> @@ -831,32 +835,20 @@ x86_target::low_siginfo_fixup (siginfo_t *ptrace,
> gdb_byte *inf, int direction)
> 
> 
> 
>  static int use_xml;
> 
> +/* Cached xcr0 value.  This is initialised the first time
> +   x86_linux_read_description is called.  */
> +
> +static uint64_t xcr0_storage;

We already cache it in fp-i387.cc. It get's set by the callback
you added. I think we should avoid having it twice.
 

>  /* Get Linux/x86 target description from running target.  */
> 
>  static const struct target_desc *
>  x86_linux_read_description (void)

Not strictly necessary, but we could remove the void while we are touching this.
This is a .cc file.

Thanks,
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