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

John Baldwin
2023-07-25T18:59:23Z

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