[v6,00/15] Handle variable XSAVE layouts

Message ID 20230714155151.21723-1-jhb@FreeBSD.org
Headers
Series Handle variable XSAVE layouts |

Message

John Baldwin July 14, 2023, 3:51 p.m. UTC
  Changes since V5:

- A few fixes not tied to the new layout handling have been merged to
  master.

- Reworded the comment describing i386_*_core_read_xsave_info in patches
  6 and 8.

Aleksandar Paunovic (2):
  gdbserver: Refactor the legacy region within the xsave struct
  gdbserver: Use x86_xstate_layout to parse the XSAVE extended state
    area.

John Baldwin (13):
  x86: Add an x86_xsave_layout structure to handle variable XSAVE
    layouts.
  gdb: Store an x86_xsave_layout in i386_gdbarch_tdep.
  core: Support fetching x86 XSAVE layout from architectures.
  nat/x86-cpuid.h: Add x86_cpuid_count wrapper around __get_cpuid_count.
  x86 nat: Add helper functions to save the XSAVE layout for the host.
  gdb: Update x86 FreeBSD architectures to support XSAVE layouts.
  gdb: Support XSAVE layouts for the current host in the FreeBSD x86
    targets.
  gdb: Update x86 Linux architectures to support XSAVE layouts.
  gdb: Support XSAVE layouts for the current host in the Linux x86
    targets.
  gdb: Use x86_xstate_layout to parse the XSAVE extended state area.
  gdbserver: Add a function to set the XSAVE mask and size.
  x86: Remove X86_XSTATE_SIZE and related constants.
  gdbserver: Simplify handling of ZMM registers.

 gdb/amd64-fbsd-nat.c       |  40 +--
 gdb/amd64-fbsd-tdep.c      |  12 +-
 gdb/amd64-linux-nat.c      |   6 +-
 gdb/amd64-linux-tdep.c     |  11 +-
 gdb/configure.nat          |   8 +-
 gdb/corelow.c              |  21 ++
 gdb/gdbarch-gen.h          |   9 +
 gdb/gdbarch.c              |  32 +++
 gdb/gdbarch.h              |   1 +
 gdb/gdbarch_components.py  |  11 +
 gdb/i386-fbsd-nat.c        |  39 +--
 gdb/i386-fbsd-tdep.c       |  72 ++---
 gdb/i386-fbsd-tdep.h       |  14 +-
 gdb/i386-linux-nat.c       |   8 +-
 gdb/i386-linux-tdep.c      |  67 ++---
 gdb/i386-linux-tdep.h      |  25 +-
 gdb/i386-tdep.c            |  18 +-
 gdb/i386-tdep.h            |   4 +
 gdb/i387-tdep.c            | 522 +++++++++++++++++++++++--------------
 gdb/i387-tdep.h            |   9 +
 gdb/nat/x86-cpuid.h        |  32 +++
 gdb/nat/x86-xstate.c       |  67 +++++
 gdb/nat/x86-xstate.h       |  35 +++
 gdb/target-debug.h         |  20 ++
 gdb/target-delegates.c     |  27 ++
 gdb/target.c               |   6 +
 gdb/target.h               |   7 +
 gdb/x86-fbsd-nat.c         |  21 ++
 gdb/x86-fbsd-nat.h         |  19 ++
 gdb/x86-linux-nat.c        |   3 +
 gdb/x86-linux-nat.h        |   7 +
 gdbserver/configure.srv    |  12 +-
 gdbserver/i387-fp.cc       | 268 +++++++++----------
 gdbserver/i387-fp.h        |   4 +-
 gdbserver/linux-x86-low.cc |  10 +-
 gdbsupport/x86-xstate.h    |  77 ++++--
 36 files changed, 1027 insertions(+), 517 deletions(-)
 create mode 100644 gdb/nat/x86-xstate.c
 create mode 100644 gdb/nat/x86-xstate.h
  

Comments

John Baldwin July 14, 2023, 3:58 p.m. UTC | #1
On 7/14/23 8:51 AM, John Baldwin wrote:
> Changes since V5:
> 
> - A few fixes not tied to the new layout handling have been merged to
>    master.
> 
> - Reworded the comment describing i386_*_core_read_xsave_info in patches
>    6 and 8.
> 
> Aleksandar Paunovic (2):
>    gdbserver: Refactor the legacy region within the xsave struct
>    gdbserver: Use x86_xstate_layout to parse the XSAVE extended state
>      area.
> 
> John Baldwin (13):
>    x86: Add an x86_xsave_layout structure to handle variable XSAVE
>      layouts.
>    gdb: Store an x86_xsave_layout in i386_gdbarch_tdep.
>    core: Support fetching x86 XSAVE layout from architectures.
>    nat/x86-cpuid.h: Add x86_cpuid_count wrapper around __get_cpuid_count.
>    x86 nat: Add helper functions to save the XSAVE layout for the host.
>    gdb: Update x86 FreeBSD architectures to support XSAVE layouts.
>    gdb: Support XSAVE layouts for the current host in the FreeBSD x86
>      targets.
>    gdb: Update x86 Linux architectures to support XSAVE layouts.
>    gdb: Support XSAVE layouts for the current host in the Linux x86
>      targets.
>    gdb: Use x86_xstate_layout to parse the XSAVE extended state area.
>    gdbserver: Add a function to set the XSAVE mask and size.
>    x86: Remove X86_XSTATE_SIZE and related constants.
>    gdbserver: Simplify handling of ZMM registers.

I would like to get this series into GDB 14 if possible.  Without it one
cannot examine certain XSAVE registers (e.g. YMM for AVX) in core dumps
generated on modern AMD CPUs on both Linux and FreeBSD.
  
Keith Seitz July 25, 2023, 5:17 p.m. UTC | #2
Hi,

On 7/14/23 08:51, John Baldwin wrote:
> Changes since V5:
> 
> - A few fixes not tied to the new layout handling have been merged to
>    master.
> 
> - Reworded the comment describing i386_*_core_read_xsave_info in patches
>    6 and 8.
> 

I am sorry I am late to the review here, but I've been testing Sapphire
Rapids this past week (and its expanded register save area), and thought
I would dig into this a bit, testing it on random x86 systems in our (internal)
test farm.
  
The (unsurprising) good news is that on RHEL9, this series does not adversely
affect regression testing results on ppc64le, aarch64, or s390x. It also
greatly improves results on Sapphire Rapids CPUs (at least on the native
unix target).

However, I've run into some pretty consistent problems which I have not yet begun to
investigate (likely all the same bug).

With either a Raptor Lake CPU ("13th Gen Intel(R) Core(TM) i7-13700") or Sapphire
Rapids ("Intel(R) Xeon(R) Gold 5418Y"), I can get gdb to consistently segfault in
memcpy in several tests using gdbserver w/-m32:

$ make check RUNTESTFLAGS="--target_board native-gdbserver/-m32" TESTS=gdb.base/auxv.exp
[snip]
		=== gdb Summary ===

# of expected passes		6
# of unexpected failures	1
# of unresolved testcases	5
# of unsupported tests		3

 From gdb.log:

(gdb) PASS: gdb.base/auxv.exp: info auxv on live process
gcore /root/test-fsf-master/gdb/build-x86_64-redhat-linux-gnu/gdb/testsuite/outputs/gdb.base/auxv/auxv.gcore


Fatal signal: Segmentation fault
----- Backtrace -----
0x55ef54b9acda gdb_internal_backtrace_1
         ../../gdb/bt-utils.c:122
0x55ef54b9acda _Z22gdb_internal_backtracev
         ../../gdb/bt-utils.c:168
0x55ef54b9acda _Z22gdb_internal_backtracev
         ../../gdb/bt-utils.c:154
0x55ef54cc086e handle_fatal_signal
         ../../gdb/event-top.c:889
0x55ef54cc0a78 handle_sigsegv
         ../../gdb/event-top.c:962
0x7fd237654dcf ???
0x55ef54d4e9bf memcpy
         /usr/include/bits/string_fortified.h:29
0x55ef54d4e9bf _Z18i387_collect_xsavePK8regcacheiPvi
         ../../gdb/i387-tdep.c:1543
0x55ef54d062c8 gcore_elf_collect_regset_section_cb
         ../../gdb/gcore-elf.c:85
0x55ef54d057ac gcore_elf_collect_thread_registers
         ../../gdb/gcore-elf.c:121
0x55ef54d057ac _Z37gcore_elf_build_thread_register_notesP7gdbarchP11thread_info10gdb_signalP3bfdPSt10unique_ptrIcN3gdb13xfree_deleterIcEEEPi
         ../../gdb/gcore-elf.c:135
0x55ef54db378f linux_corefile_thread
         ../../gdb/linux-tdep.c:1846
0x55ef54db3aec linux_make_corefile_notes
         ../../gdb/linux-tdep.c:2102
0x55ef54d06777 _Z27gdbarch_make_corefile_notesP7gdbarchP3bfdPi
         ../../gdb/gdbarch.c:3743
0x55ef54d06777 write_gcore_file_1
         ../../gdb/gcore.c:82
0x55ef54d070f7 _Z16write_gcore_fileP3bfd
         ../../gdb/gcore.c:119
0x55ef54d070f7 gcore_command
         ../../gdb/gcore.c:157
0x55ef54be0fe4 _Z8cmd_funcP16cmd_list_elementPKci
         ../../gdb/cli/cli-decode.c:2735
0x55ef54fca4b0 _Z15execute_commandPKci
         ../../gdb/top.c:574
0x55ef54cc91b7 _Z15command_handlerPKc
         ../../gdb/event-top.c:552
0x55ef54cc9263 _Z20command_line_handlerOSt10unique_ptrIcN3gdb13xfree_deleterIcEEE
         ../../gdb/event-top.c:788
0x55ef54cc048c gdb_rl_callback_handler
         ../../gdb/event-top.c:259
0x7fd2384c65bd ???
0x55ef54cc6a1a gdb_rl_callback_read_char_wrapper_noexcept
         ../../gdb/event-top.c:195
0x55ef54cc6c03 gdb_rl_callback_read_char_wrapper
         ../../gdb/event-top.c:234
0x55ef54fedc6f stdin_event_handler
         ../../gdb/ui.c:155
0x55ef551c5f2d gdb_wait_for_event
         ../gdbsupport/../../gdbsupport/event-loop.cc:694
0x55ef551c5f2d gdb_wait_for_event
         ../gdbsupport/../../gdbsupport/event-loop.cc:585
0x55ef5522c227 _Z16gdb_do_one_eventi.constprop.0
         ../gdbsupport/../../gdbsupport/event-loop.cc:264
0x55ef54dc8634 start_event_loop
         ../../gdb/main.c:412
0x55ef54dc8634 captured_command_loop
         ../../gdb/main.c:476
0x55ef54aaee2c captured_main
         ../../gdb/main.c:1320
0x55ef54aaee2c _Z8gdb_mainP18captured_main_args
         ../../gdb/main.c:1339
0x55ef54aaee2c main
         ../../gdb/gdb.c:32
---------------------
A fatal error internal to GDB has been detected, further
debugging is not possible.  GDB will now terminate.

This is a bug, please report it.  For instructions, see:
<https://www.gnu.org/software/gdb/bugs/>.

ERROR: GDB process no longer exists

A complete list of affected tests:

gdb.base/auxv.exp
gdb.base/coredump-filter.exp
gdb.base/corefile2.exp
gdb.base/gcore-tls-pie.exp
gdb.base/gcore.exp
gdb.base/info-proc.exp
gdb.base/patch.exp
gdb.base/print-symbol-loading.exp
gdb.base/siginfo-obj.exp
gdb.base/siginfo-thread.exp
gdb.base/solib-search.exp
gdb.base/vdso-warning.exp
gdb.btrace/gcore.exp
gdb.python/py-strfns.exp
gdb.reverse/break-precsave.exp
gdb.reverse/consecutive-precsave.exp
gdb.reverse/finish-precsave.exp
gdb.reverse/i386-precsave.exp
gdb.reverse/machinestate-precsave.exp
gdb.reverse/sigall-precsave.exp
gdb.reverse/solib-precsave.exp
gdb.reverse/step-precsave.exp
gdb.reverse/until-precsave.exp
gdb.threads/tls-core.exp same

The unix/-m32 target does not exhibit these problems. Just the
gdbserver targets (native-gdbserver, native-extended-gdbserver).

On my personal machine, a Comet Lake, all of these tests work/pass.

Keith
  
John Baldwin July 25, 2023, 6:15 p.m. UTC | #3
On 7/25/23 10:17 AM, Keith Seitz wrote:
> Hi,
> 
> On 7/14/23 08:51, John Baldwin wrote:
>> Changes since V5:
>>
>> - A few fixes not tied to the new layout handling have been merged to
>>     master.
>>
>> - Reworded the comment describing i386_*_core_read_xsave_info in patches
>>     6 and 8.
>>
> 
> I am sorry I am late to the review here, but I've been testing Sapphire
> Rapids this past week (and its expanded register save area), and thought
> I would dig into this a bit, testing it on random x86 systems in our (internal)
> test farm.
>    
> The (unsurprising) good news is that on RHEL9, this series does not adversely
> affect regression testing results on ppc64le, aarch64, or s390x. It also
> greatly improves results on Sapphire Rapids CPUs (at least on the native
> unix target).
> 
> However, I've run into some pretty consistent problems which I have not yet begun to
> investigate (likely all the same bug).
> 
> With either a Raptor Lake CPU ("13th Gen Intel(R) Core(TM) i7-13700") or Sapphire
> Rapids ("Intel(R) Xeon(R) Gold 5418Y"), I can get gdb to consistently segfault in
> memcpy in several tests using gdbserver w/-m32:
> 
> $ make check RUNTESTFLAGS="--target_board native-gdbserver/-m32" TESTS=gdb.base/auxv.exp
> [snip]
> 		=== gdb Summary ===
> 
> # of expected passes		6
> # of unexpected failures	1
> # of unresolved testcases	5
> # of unsupported tests		3
> 
>   From gdb.log:
> 
> (gdb) PASS: gdb.base/auxv.exp: info auxv on live process
> gcore /root/test-fsf-master/gdb/build-x86_64-redhat-linux-gnu/gdb/testsuite/outputs/gdb.base/auxv/auxv.gcore
> 
> 
> Fatal signal: Segmentation fault
> ----- Backtrace -----
> 0x55ef54b9acda gdb_internal_backtrace_1
>           ../../gdb/bt-utils.c:122
> 0x55ef54b9acda _Z22gdb_internal_backtracev
>           ../../gdb/bt-utils.c:168
> 0x55ef54b9acda _Z22gdb_internal_backtracev
>           ../../gdb/bt-utils.c:154
> 0x55ef54cc086e handle_fatal_signal
>           ../../gdb/event-top.c:889
> 0x55ef54cc0a78 handle_sigsegv
>           ../../gdb/event-top.c:962
> 0x7fd237654dcf ???
> 0x55ef54d4e9bf memcpy
>           /usr/include/bits/string_fortified.h:29
> 0x55ef54d4e9bf _Z18i387_collect_xsavePK8regcacheiPvi
>           ../../gdb/i387-tdep.c:1543

Can you confirm where this is in your patched copy?  For me this line is here:

   if (gcore)
     {
       /* Clear XSAVE extended state.  */
       memset (regs, 0, tdep->xsave_layout.sizeof_xsave);

       /* Update XCR0 and `xstate_bv' with XCR0 for gcore.  */
       if (tdep->xsave_xcr0_offset != -1)
>>>	memcpy (regs + tdep->xsave_xcr0_offset, &tdep->xcr0, 8);
       memcpy (XSAVE_XSTATE_BV_ADDR (regs), &tdep->xcr0, 8);
     }

If you have a core handy, could you provide the output of 'p tdep->xsave_layout'
and 'p tdep->xsave_xcr0_offset'?
  
Keith Seitz July 25, 2023, 6:43 p.m. UTC | #4
On 7/25/23 11:15, John Baldwin wrote:
> On 7/25/23 10:17 AM, Keith Seitz wrote:
>> On 7/14/23 08:51, John Baldwin wrote:
>> 0x55ef54d4e9bf memcpy
>>           /usr/include/bits/string_fortified.h:29
>> 0x55ef54d4e9bf _Z18i387_collect_xsavePK8regcacheiPvi
>>           ../../gdb/i387-tdep.c:1543
> 
> Can you confirm where this is in your patched copy?  For me this line is here:
> 
>    if (gcore)
>      {
>        /* Clear XSAVE extended state.  */
>        memset (regs, 0, tdep->xsave_layout.sizeof_xsave);
> 
>        /* Update XCR0 and `xstate_bv' with XCR0 for gcore.  */
>        if (tdep->xsave_xcr0_offset != -1)
>>>>     memcpy (regs + tdep->xsave_xcr0_offset, &tdep->xcr0, 8);
>        memcpy (XSAVE_XSTATE_BV_ADDR (regs), &tdep->xcr0, 8);
>      }

Yes, that's the problem spot.

> If you have a core handy, could you provide the output of 'p tdep->xsave_layout'
> and 'p tdep->xsave_xcr0_offset'?
> 

I do have a corefile:

(gdb) up
#6  i387_collect_xsave (regcache=0x5568a96b1ab0, regnum=-1, xsave=0x0, gcore=1)
     at ../../gdb/i387-tdep.c:1543
1543		memcpy (regs + tdep->xsave_xcr0_offset, &tdep->xcr0, 8);
(gdb) p tdep->xsave_layout
$1 = {sizeof_xsave = 0, avx_offset = 0, bndregs_offset = 0, bndcfg_offset = 0,
   k_offset = 0, zmm_h_offset = 0, zmm_offset = 0, pkru_offset = 0}
(gdb) p tdep->xsave_xcr0_offset
$2 = 464

If there's anything I can do to help, please do not hesitate to ask!

Keith
  
John Baldwin July 25, 2023, 6:59 p.m. UTC | #5
On 7/25/23 11:43 AM, Keith Seitz wrote:
> On 7/25/23 11:15, John Baldwin wrote:
>> On 7/25/23 10:17 AM, Keith Seitz wrote:
>>> On 7/14/23 08:51, John Baldwin wrote:
>>> 0x55ef54d4e9bf memcpy
>>>            /usr/include/bits/string_fortified.h:29
>>> 0x55ef54d4e9bf _Z18i387_collect_xsavePK8regcacheiPvi
>>>            ../../gdb/i387-tdep.c:1543
>>
>> Can you confirm where this is in your patched copy?  For me this line is here:
>>
>>     if (gcore)
>>       {
>>         /* Clear XSAVE extended state.  */
>>         memset (regs, 0, tdep->xsave_layout.sizeof_xsave);
>>
>>         /* Update XCR0 and `xstate_bv' with XCR0 for gcore.  */
>>         if (tdep->xsave_xcr0_offset != -1)
>>>>>      memcpy (regs + tdep->xsave_xcr0_offset, &tdep->xcr0, 8);
>>         memcpy (XSAVE_XSTATE_BV_ADDR (regs), &tdep->xcr0, 8);
>>       }
> 
> Yes, that's the problem spot.
> 
>> If you have a core handy, could you provide the output of 'p tdep->xsave_layout'
>> and 'p tdep->xsave_xcr0_offset'?
>>
> 
> I do have a corefile:
> 
> (gdb) up
> #6  i387_collect_xsave (regcache=0x5568a96b1ab0, regnum=-1, xsave=0x0, gcore=1)
>       at ../../gdb/i387-tdep.c:1543
> 1543		memcpy (regs + tdep->xsave_xcr0_offset, &tdep->xcr0, 8);
> (gdb) p tdep->xsave_layout
> $1 = {sizeof_xsave = 0, avx_offset = 0, bndregs_offset = 0, bndcfg_offset = 0,
>     k_offset = 0, zmm_h_offset = 0, zmm_offset = 0, pkru_offset = 0}
> (gdb) p tdep->xsave_xcr0_offset
> $2 = 464
> 
> If there's anything I can do to help, please do not hesitate to ask!

Hmm, I would not expect to have an empty layout (xsave_layout.sizeof_xsave == 0)
and then calling collect_xsave.  We only call collect_xsave in i386_linux_tdep.c
if tdep->xcr0 indicates AVX is present.  Possibly we should only call it if
tdep->xsave_layout.sizeof_xsave != 0 instead (that is what I think I did on the
FreeBSD arches).  That said, the inconsistency in tdep->xcr0 vs xsave_layout
could be problematic elsewhere so I'd rather root it out.  Can you please run
a failing test with this local patch:

diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index d5423681802..dab3428c8e6 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -8383,6 +8383,8 @@ i386_validate_tdesc_p (i386_gdbarch_tdep *tdep,
        if (!feature_sse)
  	return 0;
  
+      gdb_assert(tdep->xsave_layout.sizeof_xsave != 0);
+
        if (!feature_avx512)
  	tdep->xcr0 = X86_XSTATE_AVX_MASK;
  
@@ -8768,12 +8770,12 @@ i386_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
    info.tdesc_data = tdesc_data.get ();
    gdbarch_init_osabi (info, gdbarch);
  
+  tdep->xsave_layout = xsave_layout;
    if (!i386_validate_tdesc_p (tdep, tdesc_data.get ()))
      {
        gdbarch_free (gdbarch);
        return NULL;
      }
-  tdep->xsave_layout = xsave_layout;
  
    num_bnd_cooked = (tdep->bnd0r_regnum > 0 ? I387_NUM_BND_REGS : 0);

Hopefully it triggers the assertion, and then what will be useful is to see what
features the tdesc contains ('info locals' would cover that if they aren't all
optimized out).
  
Keith Seitz July 25, 2023, 8:42 p.m. UTC | #6
On 7/25/23 11:59, John Baldwin wrote:
> 
> Hopefully it triggers the assertion, and then what will be useful is to see what
> features the tdesc contains ('info locals' would cover that if they aren't all
> optimized out).

Yes, that assertion does trigger (on all -m32 targets).

 From the native-gdbserver/-m32 case at the assertion (I have a debug build):

(top-gdb) info locals
tdesc = 0x197b410
feature_core = 0x197d5a0
feature_sse = 0x1987200
feature_avx = 0x1989db0
feature_mpx = 0x0
feature_avx512 = 0x198a310
feature_pkeys = 0x198aec0
feature_segments = 0x0
i = 0
num_regs = 32767
valid_p = 1
__func__ = "i386_validate_tdesc_p"
(top-gdb) p *tdep
$2 = {<gdbarch_tdep_base> = {
     _vptr.gdbarch_tdep_base = 0xf5df38 <vtable for i386_gdbarch_tdep+16>},
   gregset_reg_offset = 0x159fb20 <i386_linux_gregset_reg_offset>,
   gregset_num_regs = 73, sizeof_gregset = 68, sizeof_fpregset = 108,
   st0_regnum = 16, num_mmx_regs = 8, mm0_regnum = 0, num_ymm_regs = 0,
   ymm0_regnum = 0, num_k_regs = 0, k0_regnum = 55, num_zmm_regs = 8,
   zmm0_regnum = 0, num_byte_regs = 8, al_regnum = 0, num_word_regs = 8,
   ax_regnum = 0, num_dword_regs = 0, eax_regnum = 0, num_core_regs = 32,
   num_xmm_regs = 8, num_xmm_avx512_regs = 0, xmm16_regnum = -1,
   num_ymm_avx512_regs = 0, ymm16_regnum = 0, xcr0 = 231,
   xsave_xcr0_offset = 464, xsave_layout = {sizeof_xsave = 0, avx_offset = 0,
     bndregs_offset = 0, bndcfg_offset = 0, k_offset = 0, zmm_h_offset = 0,
     zmm_offset = 0, pkru_offset = 0},
   register_names = 0xf5a6a0 <i386_register_names>, ymm0h_regnum = -1,
   ymmh_register_names = 0x0, ymm16h_regnum = -1, ymm16h_register_names = 0x0,
   bnd0r_regnum = -1, bnd0_regnum = 0, bndcfgu_regnum = -1,
   mpx_register_names = 0x0, zmm0h_regnum = 63,
   k_register_names = 0xf5a900 <i386_k_names>,
   zmmh_register_names = 0xf5a8a0 <i386_zmmh_names>,
   xmm_avx512_register_names = 0x0, ymm_avx512_register_names = 0x0,
   num_pkeys_regs = 0, pkru_regnum = -1, pkeys_register_names = 0x0,
   fsbase_regnum = -1, tdesc = 0x197b410, register_reggroup_p = 0x7a9578
      <i386_linux_register_reggroup_p(gdbarch*, int, reggroup const*)>,
   jb_pc_offset = 20, struct_return = pcc_struct_return, sigtramp_start = 0x0,
   sigtramp_end = 0x0,
   sigtramp_p = 0x7a99ae <i386_linux_sigtramp_p(frame_info_ptr)>,
   sigcontext_addr = 0x7a9c52 <i386_linux_sigcontext_addr(frame_info_ptr)>,
   sc_reg_offset = 0x159fc60 <i386_linux_sc_reg_offset>, sc_num_regs = 16,
   sc_pc_offset = -1, sc_sp_offset = -1, i386_mmx_type = 0x0,
   i386_ymm_type = 0x0, i386_zmm_type = 0x0, i387_ext_type = 0x0,
   i386_bnd_type = 0x0, record_regmap = 0xf5d5a0 <i386_record_regmap>,
   i386_intx80_record = 0x7aa2fc <i386_linux_intx80_sysenter_syscall_record(regcache*)>,
   i386_sysenter_record = 0x7aa2fc <i386_linux_intx80_sysenter_syscall_record(regcache*)>,
   i386_syscall_record = 0x7aa2fc <i386_linux_intx80_sysenter_syscall_record(regcache*)>, fpregset = 0xf5b5c0 <i386_fpregset>}

Keith
  
John Baldwin July 25, 2023, 10:05 p.m. UTC | #7
On 7/25/23 1:42 PM, Keith Seitz wrote:
> On 7/25/23 11:59, John Baldwin wrote:
>>
>> Hopefully it triggers the assertion, and then what will be useful is to see what
>> features the tdesc contains ('info locals' would cover that if they aren't all
>> optimized out).
> 
> Yes, that assertion does trigger (on all -m32 targets).
> 
>   From the native-gdbserver/-m32 case at the assertion (I have a debug build):
> 
> (top-gdb) info locals
> tdesc = 0x197b410
> feature_core = 0x197d5a0
> feature_sse = 0x1987200
> feature_avx = 0x1989db0
> feature_mpx = 0x0
> feature_avx512 = 0x198a310
> feature_pkeys = 0x198aec0
> feature_segments = 0x0
> i = 0
> num_regs = 32767
> valid_p = 1
> __func__ = "i386_validate_tdesc_p"
> (top-gdb) p *tdep
> $2 = {<gdbarch_tdep_base> = {
>       _vptr.gdbarch_tdep_base = 0xf5df38 <vtable for i386_gdbarch_tdep+16>},
>     gregset_reg_offset = 0x159fb20 <i386_linux_gregset_reg_offset>,
>     gregset_num_regs = 73, sizeof_gregset = 68, sizeof_fpregset = 108,
>     st0_regnum = 16, num_mmx_regs = 8, mm0_regnum = 0, num_ymm_regs = 0,
>     ymm0_regnum = 0, num_k_regs = 0, k0_regnum = 55, num_zmm_regs = 8,
>     zmm0_regnum = 0, num_byte_regs = 8, al_regnum = 0, num_word_regs = 8,
>     ax_regnum = 0, num_dword_regs = 0, eax_regnum = 0, num_core_regs = 32,
>     num_xmm_regs = 8, num_xmm_avx512_regs = 0, xmm16_regnum = -1,
>     num_ymm_avx512_regs = 0, ymm16_regnum = 0, xcr0 = 231,
>     xsave_xcr0_offset = 464, xsave_layout = {sizeof_xsave = 0, avx_offset = 0,
>       bndregs_offset = 0, bndcfg_offset = 0, k_offset = 0, zmm_h_offset = 0,
>       zmm_offset = 0, pkru_offset = 0},
>     register_names = 0xf5a6a0 <i386_register_names>, ymm0h_regnum = -1,
>     ymmh_register_names = 0x0, ymm16h_regnum = -1, ymm16h_register_names = 0x0,
>     bnd0r_regnum = -1, bnd0_regnum = 0, bndcfgu_regnum = -1,
>     mpx_register_names = 0x0, zmm0h_regnum = 63,
>     k_register_names = 0xf5a900 <i386_k_names>,
>     zmmh_register_names = 0xf5a8a0 <i386_zmmh_names>,
>     xmm_avx512_register_names = 0x0, ymm_avx512_register_names = 0x0,
>     num_pkeys_regs = 0, pkru_regnum = -1, pkeys_register_names = 0x0,
>     fsbase_regnum = -1, tdesc = 0x197b410, register_reggroup_p = 0x7a9578
>        <i386_linux_register_reggroup_p(gdbarch*, int, reggroup const*)>,
>     jb_pc_offset = 20, struct_return = pcc_struct_return, sigtramp_start = 0x0,
>     sigtramp_end = 0x0,
>     sigtramp_p = 0x7a99ae <i386_linux_sigtramp_p(frame_info_ptr)>,
>     sigcontext_addr = 0x7a9c52 <i386_linux_sigcontext_addr(frame_info_ptr)>,
>     sc_reg_offset = 0x159fc60 <i386_linux_sc_reg_offset>, sc_num_regs = 16,
>     sc_pc_offset = -1, sc_sp_offset = -1, i386_mmx_type = 0x0,
>     i386_ymm_type = 0x0, i386_zmm_type = 0x0, i387_ext_type = 0x0,
>     i386_bnd_type = 0x0, record_regmap = 0xf5d5a0 <i386_record_regmap>,
>     i386_intx80_record = 0x7aa2fc <i386_linux_intx80_sysenter_syscall_record(regcache*)>,
>     i386_sysenter_record = 0x7aa2fc <i386_linux_intx80_sysenter_syscall_record(regcache*)>,
>     i386_syscall_record = 0x7aa2fc <i386_linux_intx80_sysenter_syscall_record(regcache*)>, fpregset = 0xf5b5c0 <i386_fpregset>}

Hmm, so what I have been assuming is that you only get a tdesc with those register
sets if target::read_description (either x86_linux_nat_target::read_description
in x86-linux-nat.c for a native process or the gdbarch_core_read_description
handler in (i386|amd64)-linux-tdep.c) returns a tdesc with it, and the ones I just
mentioned all set a xsave_layout at the same time.  However, I think I had overlooked
the remote target.  That is, if you are connected to a remote target but want to
use gcore to write out a local core dump, that I think is the case that is breaking.

This case is a bit weird.  I had assumed I could avoid having to send the
layout across the remote protocol because the individual registers are
sent across the protocol and gdbserver is required to deal with the layout
and reinterpret the XSAVE "block" as individual registers.  However, to write
out a core dump note, we have to use some sort of XSAVE layout.  I could either
go back to making it a new TARGET_OBJECT and it could be fetched across the
wire from gdbserver (but only used for gcore, not for anything else), but
you still have a compatiblity problem in that old gdbserver's won't know about
it so you need a fallback based on the XCR0 mask.  Alternatively, we could
just always use a fallback in this case of picking a layout based on the XCR0
mask.  Previously this was always using the hardcoded Intel layout which is
why this worked before.

It's a bit of a shame to have to fetch it over the wire, but probably that is
the cleaner long term solution, even though it will require a fallback for now
to pick an arbitrary layout based on the XCR0 mask.
  
Terekhov, Mikhail via Gdb-patches July 26, 2023, 8:31 a.m. UTC | #8
Hi John,

Just fyi, this week Intel announced APX:
https://www.intel.com/content/www/us/en/developer/articles/technical/advanced-performance-extensions-apx.html

It adds new GPRs. To cite from there:

"The new GPRs are XSAVE-enabled, which means that they can be automatically saved and restored by XSAVE/XRSTOR sequences during context switches.
They do not change the size and layout of the XSAVE area as they take up the space left behind by the deprecated Intel® MPX registers."

Not sure if that has any influence on your patch series, I thought it might be interesting for you anyway.

Regards,
Felix


> -----Original Message-----
> From: Gdb-patches <gdb-patches-
> bounces+felix.willgerodt=intel.com@sourceware.org> On Behalf Of John Baldwin
> Sent: Freitag, 14. Juli 2023 17:59
> To: gdb-patches@sourceware.org
> Cc: Joel Brobecker <brobecker@adacore.com>; George, Jini Susan
> <JiniSusan.George@amd.com>
> Subject: Re: [PATCH v6 00/15] Handle variable XSAVE layouts
> 
> On 7/14/23 8:51 AM, John Baldwin wrote:
> > Changes since V5:
> >
> > - A few fixes not tied to the new layout handling have been merged to
> >    master.
> >
> > - Reworded the comment describing i386_*_core_read_xsave_info in patches
> >    6 and 8.
> >
> > Aleksandar Paunovic (2):
> >    gdbserver: Refactor the legacy region within the xsave struct
> >    gdbserver: Use x86_xstate_layout to parse the XSAVE extended state
> >      area.
> >
> > John Baldwin (13):
> >    x86: Add an x86_xsave_layout structure to handle variable XSAVE
> >      layouts.
> >    gdb: Store an x86_xsave_layout in i386_gdbarch_tdep.
> >    core: Support fetching x86 XSAVE layout from architectures.
> >    nat/x86-cpuid.h: Add x86_cpuid_count wrapper around __get_cpuid_count.
> >    x86 nat: Add helper functions to save the XSAVE layout for the host.
> >    gdb: Update x86 FreeBSD architectures to support XSAVE layouts.
> >    gdb: Support XSAVE layouts for the current host in the FreeBSD x86
> >      targets.
> >    gdb: Update x86 Linux architectures to support XSAVE layouts.
> >    gdb: Support XSAVE layouts for the current host in the Linux x86
> >      targets.
> >    gdb: Use x86_xstate_layout to parse the XSAVE extended state area.
> >    gdbserver: Add a function to set the XSAVE mask and size.
> >    x86: Remove X86_XSTATE_SIZE and related constants.
> >    gdbserver: Simplify handling of ZMM registers.
> 
> I would like to get this series into GDB 14 if possible.  Without it one
> cannot examine certain XSAVE registers (e.g. YMM for AVX) in core dumps
> generated on modern AMD CPUs on both Linux and FreeBSD.
> 
> --
> John Baldwin

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
  
John Baldwin July 26, 2023, 10:31 p.m. UTC | #9
On 7/25/23 3:05 PM, John Baldwin wrote:
> On 7/25/23 1:42 PM, Keith Seitz wrote:
>> On 7/25/23 11:59, John Baldwin wrote:
>>>
>>> Hopefully it triggers the assertion, and then what will be useful is to see what
>>> features the tdesc contains ('info locals' would cover that if they aren't all
>>> optimized out).
>>
>> Yes, that assertion does trigger (on all -m32 targets).
>>
>>    From the native-gdbserver/-m32 case at the assertion (I have a debug build):
>>
>> (top-gdb) info locals
>> tdesc = 0x197b410
>> feature_core = 0x197d5a0
>> feature_sse = 0x1987200
>> feature_avx = 0x1989db0
>> feature_mpx = 0x0
>> feature_avx512 = 0x198a310
>> feature_pkeys = 0x198aec0
>> feature_segments = 0x0
>> i = 0
>> num_regs = 32767
>> valid_p = 1
>> __func__ = "i386_validate_tdesc_p"
>> (top-gdb) p *tdep
>> $2 = {<gdbarch_tdep_base> = {
>>        _vptr.gdbarch_tdep_base = 0xf5df38 <vtable for i386_gdbarch_tdep+16>},
>>      gregset_reg_offset = 0x159fb20 <i386_linux_gregset_reg_offset>,
>>      gregset_num_regs = 73, sizeof_gregset = 68, sizeof_fpregset = 108,
>>      st0_regnum = 16, num_mmx_regs = 8, mm0_regnum = 0, num_ymm_regs = 0,
>>      ymm0_regnum = 0, num_k_regs = 0, k0_regnum = 55, num_zmm_regs = 8,
>>      zmm0_regnum = 0, num_byte_regs = 8, al_regnum = 0, num_word_regs = 8,
>>      ax_regnum = 0, num_dword_regs = 0, eax_regnum = 0, num_core_regs = 32,
>>      num_xmm_regs = 8, num_xmm_avx512_regs = 0, xmm16_regnum = -1,
>>      num_ymm_avx512_regs = 0, ymm16_regnum = 0, xcr0 = 231,
>>      xsave_xcr0_offset = 464, xsave_layout = {sizeof_xsave = 0, avx_offset = 0,
>>        bndregs_offset = 0, bndcfg_offset = 0, k_offset = 0, zmm_h_offset = 0,
>>        zmm_offset = 0, pkru_offset = 0},
>>      register_names = 0xf5a6a0 <i386_register_names>, ymm0h_regnum = -1,
>>      ymmh_register_names = 0x0, ymm16h_regnum = -1, ymm16h_register_names = 0x0,
>>      bnd0r_regnum = -1, bnd0_regnum = 0, bndcfgu_regnum = -1,
>>      mpx_register_names = 0x0, zmm0h_regnum = 63,
>>      k_register_names = 0xf5a900 <i386_k_names>,
>>      zmmh_register_names = 0xf5a8a0 <i386_zmmh_names>,
>>      xmm_avx512_register_names = 0x0, ymm_avx512_register_names = 0x0,
>>      num_pkeys_regs = 0, pkru_regnum = -1, pkeys_register_names = 0x0,
>>      fsbase_regnum = -1, tdesc = 0x197b410, register_reggroup_p = 0x7a9578
>>         <i386_linux_register_reggroup_p(gdbarch*, int, reggroup const*)>,
>>      jb_pc_offset = 20, struct_return = pcc_struct_return, sigtramp_start = 0x0,
>>      sigtramp_end = 0x0,
>>      sigtramp_p = 0x7a99ae <i386_linux_sigtramp_p(frame_info_ptr)>,
>>      sigcontext_addr = 0x7a9c52 <i386_linux_sigcontext_addr(frame_info_ptr)>,
>>      sc_reg_offset = 0x159fc60 <i386_linux_sc_reg_offset>, sc_num_regs = 16,
>>      sc_pc_offset = -1, sc_sp_offset = -1, i386_mmx_type = 0x0,
>>      i386_ymm_type = 0x0, i386_zmm_type = 0x0, i387_ext_type = 0x0,
>>      i386_bnd_type = 0x0, record_regmap = 0xf5d5a0 <i386_record_regmap>,
>>      i386_intx80_record = 0x7aa2fc <i386_linux_intx80_sysenter_syscall_record(regcache*)>,
>>      i386_sysenter_record = 0x7aa2fc <i386_linux_intx80_sysenter_syscall_record(regcache*)>,
>>      i386_syscall_record = 0x7aa2fc <i386_linux_intx80_sysenter_syscall_record(regcache*)>, fpregset = 0xf5b5c0 <i386_fpregset>}
> 
> Hmm, so what I have been assuming is that you only get a tdesc with those register
> sets if target::read_description (either x86_linux_nat_target::read_description
> in x86-linux-nat.c for a native process or the gdbarch_core_read_description
> handler in (i386|amd64)-linux-tdep.c) returns a tdesc with it, and the ones I just
> mentioned all set a xsave_layout at the same time.  However, I think I had overlooked
> the remote target.  That is, if you are connected to a remote target but want to
> use gcore to write out a local core dump, that I think is the case that is breaking.
> 
> This case is a bit weird.  I had assumed I could avoid having to send the
> layout across the remote protocol because the individual registers are
> sent across the protocol and gdbserver is required to deal with the layout
> and reinterpret the XSAVE "block" as individual registers.  However, to write
> out a core dump note, we have to use some sort of XSAVE layout.  I could either
> go back to making it a new TARGET_OBJECT and it could be fetched across the
> wire from gdbserver (but only used for gcore, not for anything else), but
> you still have a compatiblity problem in that old gdbserver's won't know about
> it so you need a fallback based on the XCR0 mask.  Alternatively, we could
> just always use a fallback in this case of picking a layout based on the XCR0
> mask.  Previously this was always using the hardcoded Intel layout which is
> why this worked before.
> 
> It's a bit of a shame to have to fetch it over the wire, but probably that is
> the cleaner long term solution, even though it will require a fallback for now
> to pick an arbitrary layout based on the XCR0 mask.

So I have a couple of things to try here.  First, a simple fix for the crash
is this change (relative to the branch), but it does mean the core dumps generated
by gcore for a remote target will not contain extended XSAVE state like AVX, etc.:

diff --git a/gdb/i386-linux-tdep.c b/gdb/i386-linux-tdep.c
index c8b467a0416..a7d61f33a08 100644
--- a/gdb/i386-linux-tdep.c
+++ b/gdb/i386-linux-tdep.c
@@ -768,7 +768,7 @@ i386_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
  
    cb (".reg", 68, 68, &i386_gregset, NULL, cb_data);
  
-  if (tdep->xcr0 & X86_XSTATE_AVX)
+  if (tdep->xsave_layout.sizeof_xsave != 0)
      cb (".reg-xstate", tdep->xsave_layout.sizeof_xsave,
         tdep->xsave_layout.sizeof_xsave, &i386_linux_xstateregset,
         "XSAVE extended state", cb_data);

Arguably that is more correct as it lines up with the way the rest of the x86 arches
in this series.

The other change I have locally is one to generate a "fallback" layout to use in
this case that a target (such as remote) doesn't provide a layout.  I'm still not
quite happy with what I would want to do over the wire.  In some ways I'd rather
have a way to send across CPUID requests to mimic what we do for live processes
to determine the layout.  I really don't want to encode a binary form of the
current structure as it will likely change in the future for new XSAVE regions.
Perhaps an XML blob that described the total size and the size and offset of each
region could work.  Still, you would always need a fallback for old servers.

The fallback change:

diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index d5423681802..30968522824 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -8773,6 +8773,14 @@ i386_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
        gdbarch_free (gdbarch);
        return NULL;
      }
+
+  /* If the target didn't provide an XSAVE layout, generate a fallback
+     layout.  This currently happens with remote targets and is used
+     to determine the layout of the XSAVE register notes when
+     generating a core dump.  */
+  if (tdep->xcr0 != X86_XSTATE_X87_MASK && tdep->xcr0 != X86_XSTATE_SSE_MASK
+      && xsave_layout.sizeof_xsave == 0)
+    xsave_layout = i387_fallback_xsave_layout (tdep->xcr0);
    tdep->xsave_layout = xsave_layout;
  
    num_bnd_cooked = (tdep->bnd0r_regnum > 0 ? I387_NUM_BND_REGS : 0);
diff --git a/gdb/i387-tdep.c b/gdb/i387-tdep.c
index 47667da21c7..90bd7a5db9e 100644
--- a/gdb/i387-tdep.c
+++ b/gdb/i387-tdep.c
@@ -987,6 +987,48 @@ i387_guess_xsave_layout (uint64_t xcr0, size_t xsave_size,
    return true;
  }
  
+/* See i387-tdep.h.  */
+
+x86_xsave_layout
+i387_fallback_xsave_layout (uint64_t xcr0)
+{
+  x86_xsave_layout layout;
+  if (HAS_PKRU (xcr0))
+    {
+      layout.sizeof_xsave = 2696;
+      layout.avx_offset = 576;
+      layout.bndregs_offset = 960;
+      layout.bndcfg_offset = 1024;
+      layout.k_offset = 1088;
+      layout.zmm_h_offset = 1152;
+      layout.zmm_offset = 1664;
+      layout.pkru_offset = 2688;
+    }
+  else if (HAS_AVX512 (xcr0))
+    {
+      layout.sizeof_xsave = 2688;
+      layout.avx_offset = 576;
+      layout.bndregs_offset = 960;
+      layout.bndcfg_offset = 1024;
+      layout.k_offset = 1088;
+      layout.zmm_h_offset = 1152;
+      layout.zmm_offset = 1664;
+    }
+  else if (HAS_MPX (xcr0))
+    {
+      layout.sizeof_xsave = 1088;
+      layout.avx_offset = 576;
+      layout.bndregs_offset = 960;
+      layout.bndcfg_offset = 1024;
+    }
+  else
+    {
+      layout.sizeof_xsave = 832;
+      layout.avx_offset = 576;
+    }
+  return layout;
+}
+
  /* Extract from XSAVE a bitset of the features that are available on the
     target, but which have not yet been enabled.  */
  
diff --git a/gdb/i387-tdep.h b/gdb/i387-tdep.h
index e149e30e52e..8de68d00f6e 100644
--- a/gdb/i387-tdep.h
+++ b/gdb/i387-tdep.h
@@ -147,6 +147,10 @@ extern void i387_supply_fxsave (struct regcache *regcache, int regnum,
  extern bool i387_guess_xsave_layout (uint64_t xcr0, size_t xsave_size,
                                      x86_xsave_layout &layout);
  
+/* Generate a fallback XSAVE layout based on the XCR0 bitmask.  */
+
+extern x86_xsave_layout i387_fallback_xsave_layout (uint64_t xcr0);
+
  /* Similar to i387_supply_fxsave, but use XSAVE extended state.  */
  
  extern void i387_supply_xsave (struct regcache *regcache, int regnum,
  
Keith Seitz July 27, 2023, 9:36 p.m. UTC | #10
On 7/26/23 15:31, John Baldwin wrote:
> 
> So I have a couple of things to try here.  First, a simple fix for the crash
> is this change (relative to the branch), but it does mean the core dumps generated
> by gcore for a remote target will not contain extended XSAVE state like AVX, etc.:
> 
> diff --git a/gdb/i386-linux-tdep.c b/gdb/i386-linux-tdep.c
> index c8b467a0416..a7d61f33a08 100644
> --- a/gdb/i386-linux-tdep.c
> +++ b/gdb/i386-linux-tdep.c
> @@ -768,7 +768,7 @@ i386_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
> 
>     cb (".reg", 68, 68, &i386_gregset, NULL, cb_data);
> 
> -  if (tdep->xcr0 & X86_XSTATE_AVX)
> +  if (tdep->xsave_layout.sizeof_xsave != 0)
>       cb (".reg-xstate", tdep->xsave_layout.sizeof_xsave,
>          tdep->xsave_layout.sizeof_xsave, &i386_linux_xstateregset,
>          "XSAVE extended state", cb_data);
> 
> Arguably that is more correct as it lines up with the way the rest of the x86 arches
> in this series.

Yeah, that does fix a lot of problems. In the vein of "perfect is the enemy of good,"
this LGTM.

> The other change I have locally is one to generate a "fallback" layout to use in
> this case that a target (such as remote) doesn't provide a layout.

> 
> The fallback change:
[snip]

This causes a new regression in gdb.base/solib-display.exp:

# make check RUNTESTFLAGS="--target_board native-extended-gdbserver.exp" \
   TESTS=gdb.base/solib-display.exp
Running /root/fsf/linux/gdb/testsuite/../../../src/gdb/testsuite/gdb.base/solib-display.exp ...
FAIL: gdb.base/solib-display.exp: NO: continue two
FAIL: gdb.base/solib-display.exp: IN: continue two
FAIL: gdb.base/solib-display.exp: SEP: continue two

		=== gdb Summary ===

# of expected passes		32
# of unexpected failures	3

The issue is the same with all three failing tests (-m32/-m64 don't matter).
With this tweak, we now see:

warning: Unable to display "a_local": No symbol "a_local" in current context.
warning: Unable to display "a_static": No symbol "a_static" in current context.

This disables the display of the two variables, and they are omitted from
subsequent displays. [This happens on every x86_64 on which I've tested: Comet Lake,
Raptor Lake, and Sapphire Rapids.]

Keith
  
John Baldwin July 28, 2023, 4:35 p.m. UTC | #11
On 7/27/23 2:36 PM, Keith Seitz wrote:
> On 7/26/23 15:31, John Baldwin wrote:
>>
>> So I have a couple of things to try here.  First, a simple fix for the crash
>> is this change (relative to the branch), but it does mean the core dumps generated
>> by gcore for a remote target will not contain extended XSAVE state like AVX, etc.:
>>
>> diff --git a/gdb/i386-linux-tdep.c b/gdb/i386-linux-tdep.c
>> index c8b467a0416..a7d61f33a08 100644
>> --- a/gdb/i386-linux-tdep.c
>> +++ b/gdb/i386-linux-tdep.c
>> @@ -768,7 +768,7 @@ i386_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
>>
>>      cb (".reg", 68, 68, &i386_gregset, NULL, cb_data);
>>
>> -  if (tdep->xcr0 & X86_XSTATE_AVX)
>> +  if (tdep->xsave_layout.sizeof_xsave != 0)
>>        cb (".reg-xstate", tdep->xsave_layout.sizeof_xsave,
>>           tdep->xsave_layout.sizeof_xsave, &i386_linux_xstateregset,
>>           "XSAVE extended state", cb_data);
>>
>> Arguably that is more correct as it lines up with the way the rest of the x86 arches
>> in this series.
> 
> Yeah, that does fix a lot of problems. In the vein of "perfect is the enemy of good,"
> this LGTM.

Ok.

>> The other change I have locally is one to generate a "fallback" layout to use in
>> this case that a target (such as remote) doesn't provide a layout.
> 
>>
>> The fallback change:
> [snip]
> 
> This causes a new regression in gdb.base/solib-display.exp:
> 
> # make check RUNTESTFLAGS="--target_board native-extended-gdbserver.exp" \
>     TESTS=gdb.base/solib-display.exp
> Running /root/fsf/linux/gdb/testsuite/../../../src/gdb/testsuite/gdb.base/solib-display.exp ...
> FAIL: gdb.base/solib-display.exp: NO: continue two
> FAIL: gdb.base/solib-display.exp: IN: continue two
> FAIL: gdb.base/solib-display.exp: SEP: continue two
> 
> 		=== gdb Summary ===
> 
> # of expected passes		32
> # of unexpected failures	3
> 
> The issue is the same with all three failing tests (-m32/-m64 don't matter).
> With this tweak, we now see:
> 
> warning: Unable to display "a_local": No symbol "a_local" in current context.
> warning: Unable to display "a_static": No symbol "a_static" in current context.
> 
> This disables the display of the two variables, and they are omitted from
> subsequent displays. [This happens on every x86_64 on which I've tested: Comet Lake,
> Raptor Lake, and Sapphire Rapids.]

Humm, ok.  I should be able to look at this locally in a x86-64 VM I have.
I'm inclined to probably leave this off the series though for now while I
figure out the right solution to gcore of a remote process.