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

[Andrew Burgess <aburgess@redhat.com>]

"Willgerodt, Felix" <felix.willgerodt@intel.com> writes:

>> -----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.

Thanks for all your feedback on this series.  I'm working through the
comments, but wanted to reply to this point separately:

I also wondered about this.  I initially left if because, as you say,
I'm just moving the code, and didn't want to change too much in one go.

Given your feedback though I did trace this code back to this commit:
3a13a53b432d732be258d677b6afca969ce0d65f from April 2010.  What's
interesting is this part of the ChangeLog:

  (i386_linux_read_description): Set have_ptrace_getfpxregs and
  have_ptrace_getregset to 0 if ptrace PTRACE_GETFPXREGS failed.

The function name has changed since then, but notice that the author
specifically mentions their intention to set BOTH flags when
PTRACE_GETFPXREGS fails.  Based on that I'm inclined to leave this alone
unless it can be shown that it's wrong.

Thanks,
Andrew

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