gdb/i386: fix tdesc rejection issue for targets without PTRACE_GETREGSETusers/aburgess/try-fix-i386-tdesc-issue

After the x86 target description changes that I committed recently,
the first commit in the series being:

  commit 8a29222b85f28a2201db50a34ac4144f961311db
  Date:   Sat Jan 27 10:40:35 2024 +0000

      gdb/gdbserver: share I386_LINUX_XSAVE_XCR0_OFFSET definition

and the last commit in the series being:

  commit 646d754d14c2fe70a492a893506a74b0463b6ae8
  Author: Andrew Burgess <aburgess@redhat.com>
  Date:   Tue Jan 30 15:37:23 2024 +0000

      gdb/gdbserver: share x86/linux tdesc caching

The sourceware buildbot highlighted a regression on i386.  On the GDB
side we'd see this:

  Remote debugging using :54321
  warning: Architecture rejected target-supplied description
  Remote connection closed
  (gdb)

while on the gdbserver side we'd see this:

  $ ./gdbserver/gdbserver --once :54321 ~/empty
  Process /srv/aburgess/empty created; pid = 31406
  Listening on port 54321
  Remote debugging from host ::1, port 39488
  ../../src/gdbserver/regcache.cc:272: A problem internal to GDBserver has been detected.
  Unknown register st0 requested
  Aborted (core dumped)

When I tried to reproduce this regression on my local i386 VM the
issue would not reproduce.

I eventually tracked the problem down to x86_linux_tdesc_for_tid in
gdb/nat/x86-linux-tdesc.c.  In this function we have this line:

  /* Check if PTRACE_GETREGSET works.  */
  if (ptrace (PTRACE_GETREGSET, tid,
              (unsigned int) NT_X86_XSTATE, &iov) < 0)
    {
      ... handle failure ...
    }
  else
    {
      ... handle success ...
    }

The problem is that on my VM the PTRACE_GETREGSET feature is
supported, while on sourceware's buildbot machine this feature is not
supported.

I did a quick search and it seems like the 'xsave' feature in
/proc/cpuinfo might be the indicator for whether PTRACE_GETREGSET is
supported or not, and indeed my machine has the 'xsave' feature while
the sourceware machine does not.

The point of divergence then is this ptrace call, on my machine the
call succeeds and we extract the xcr0 value from the iov vector, while
on the sourceware machine the ptrace call fails and we use a default
xcr0 value of 0.

This xcr0 value is then passed to i386_linux_read_description at the
end of x86_linux_tdesc_for_tid.

In gdb/arch/i386-linux-tdesc.c we find i386_linux_read_description
which does some caching but calls i386_create_target_description to
actually create the target descriptions when needed.  The xcr0 value
is masked to only the bits that are interesting, but given a value of
0 we'll just pass 0 through to i386_create_target_description.

In gdb/arch/i386.c we find i386_create_target_description which checks
the xcr0 bits and builds the target description.  What we can see is
that if no bits are set in the xcr0 value then no features will be
added to the created target description.  This featureless target
description is then transmitted back to GDB, which is then rejected
due to lack of essential core registers.

So, how did things work prior to the above commit series?  There are
three places of interest, on the GDB side there is
x86_linux_nat_target::read_description and
i386_linux_core_read_description.  Then on the gdbserver side there is
x86_linux_read_description.

All of these locations have a call to i386_linux_read_description
followed by a check if the return value was nullptr.  If we do get
back nullptr then we perform another call to
i386_linux_read_description with a default xcr0 value.

Looking in i386_linux_read_description we see a specific check for
xcr0 being 0 in which case we return nullptr.

And so, prior to the above series, if xcr0 was 0 due to
PTRACE_GETREGSET being unavailable we'd use a default xcr0 value.

After the above series this is no longer the case, the 'xcr0 == 0'
check has been removed from i386_linux_read_description and the
calling code is streamlined to remove the use of default xcr0 values.

The fix I propose here is to setup the default xcr0 value at the point
where we find that PTRACE_GETREGSET is unavailable.  The default value
used is X86_XSTATE_SSE_MASK.  This is the default used in
x86_linux_nat_target::read_description (for GDB) and in
x86_linux_read_description (for gdbserver).  The above commit series
already fixed i386_linux_core_read_description to ensure that the
correct default xcr0 value was used, this case is a little special in
that it uses different defaults depending on which sections are
present in the core file, so that case always needed to be handled
differently.

The choice of X86_XSTATE_SSE_MASK corresponds to the default used for
i386 before the above series was committed.  This mask includes the
X87 and SSE bits only, neither of these bits are checked for on amd64
or x32, so this default doesn't change the behaviour on these targets.

By setting the default xcr0 value at this early stage we ensure that
the cached xcr0 value on the gdbserver side is correct.  This is
critical as this cached xcr0 value is passed through to the in process
agent (IPA).  If we leave the cached xcr0 value as 0 and apply the
defaults later in the series we also have to encode the knowledge of
the default into the IPA, this just means we have the default encoded
in multiple locations, which seems like a bad idea.  The approach used
in this patch means the default is present in just one location.

This commit should fix the i386 regressions seen on the sourceware
buildbot.

2 files changed, 10 insertions, 13 deletions

diff --git a/gdb/nat/x86-linux-tdesc.c b/gdb/nat/x86-linux-tdesc.c
index 05aa75d..c15a600 100644
--- a/gdb/nat/x86-linux-tdesc.c
+++ b/gdb/nat/x86-linux-tdesc.c
@@ -90,8 +90,13 @@ x86_linux_tdesc_for_tid (int tid, uint64_t *xcr0_storage,
       if (ptrace (PTRACE_GETREGSET, tid,
 		  (unsigned int) NT_X86_XSTATE, &iov) < 0)
 	{
+	  /* Can't fetch the xcr0 value so pick a simple default.  This
+	     default has x87 and sse bits set.  These bits are ignored for
+	     amd64 and x32 targets, but are checked for on i386.  Without
+	     these bits being set we generate a completely empty tdesc for
+	     i386 which will be rejected by GDB.  */
 	  have_ptrace_getregset = TRIBOOL_FALSE;
-	  *xcr0_storage = 0;
+	  *xcr0_storage = X86_XSTATE_SSE_MASK;
 	}
       else
 	{
@@ -112,15 +117,8 @@ x86_linux_tdesc_for_tid (int tid, uint64_t *xcr0_storage,
 	}
     }
 
-  /* 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;
+  /* Use cached xcr0 value.  */
+  uint64_t xcr0_features_bits = *xcr0_storage & X86_XSTATE_ALL_MASK;
 
 #ifdef __x86_64__
   if (is_64bit)
diff --git a/gdb/nat/x86-linux-tdesc.h b/gdb/nat/x86-linux-tdesc.h
index b7a4649..1ec190e 100644
--- a/gdb/nat/x86-linux-tdesc.h
+++ b/gdb/nat/x86-linux-tdesc.h
@@ -44,8 +44,7 @@ struct x86_xsave_layout;
    i386_linux_read_description for cases when nullptr might be
    returned.  */
 
-extern const target_desc *
-x86_linux_tdesc_for_tid (int tid, uint64_t *xcr0_storage,
-			 x86_xsave_layout *xsave_layout_storage);
+extern const target_desc * x86_linux_tdesc_for_tid
+  (int tid, uint64_t *xcr0_storage, x86_xsave_layout *xsave_layout_storage);
 
 #endif /* NAT_X86_LINUX_TDESC_H */