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

[John Baldwin <jhb@FreeBSD.org>]

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).

-- 
John Baldwin