From d51096abeeb22418e3407d6892536a02f82333bb Mon Sep 17 00:00:00 2001 From: Andrew Burgess Date: Fri, 21 Jun 2024 11:43:58 +0100 Subject: gdb/i386: fix tdesc rejection issue for targets without PTRACE_GETREGSET 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 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. --- gdb/nat/x86-linux-tdesc.c | 18 ++++++++---------- gdb/nat/x86-linux-tdesc.h | 5 ++--- 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 */ -- cgit v1.1