Re: [PATCH v6 00/15] Handle variable XSAVE layouts

[John Baldwin <jhb@FreeBSD.org>]

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,


-- 
John Baldwin