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

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

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