Re: [PATCH] gdb/x86: use size of XSAVE area of enabled features

[Simon Marchi <simon.marchi@polymtl.ca>]

On 9/27/23 16:20, John Baldwin wrote:
> On 9/27/23 5:15 PM, Simon Marchi wrote:
>> Since commit b42405a1594 ("gdb: Update x86 Linux architectures to
>> support XSAVE layouts."), the test gdb.base/gcore.exp fails on my AMD
>> Ryzen 3700X machine:
>>
>>      FAIL: gdb.base/gcore.exp: corefile restored all registers
>>
>> The test gets the register state (saves the output of "info
>> all-registers"), saves a core with the "gcore" command, loads the core,
>> and checks the register state against the one previously saved.  The
>> problem is that when reading registers from the core file, the last half
>> of ymm registers is unavailable:
>>
>>      (gdb) print $ymm0.v32_int8
>>      $1 = {0, -77, -23, -9, -1, 127, 0, 0, 0, -77, -23, -9, -1, 127, 0, 0, <unavailable> <repeats 16 times>}
>>
>> One strange thing with this machine is that the bitset of state
>> components supported by XCR0 is 0x207, meaning "x87 | SSE | AVX | PKRU",
>> but XCR0 at runtime is 0x7, meaning "x87 | SSE | AVX".  So, PKRU appears
>> to be supported by the processor, but disabled by the kernel.  I didn't
>> find why yet.
>>
>>  From CPUID leaf EAX=0Dh, ECX=00h, GDB can get:
>>
>>   - from EBX: max size of the XSAVE area required by features currently
>>     enabled in XCR0.  On my machine, it's 0x340 (832).
>>   - from ECX: max size of the XSAVE area required by all features
>>     supported by XCR0.  On my machine, it's 0x380 (896).
>>
>> At runtime, GDB uses ECX (max size required by all supported features)
>> to fill the x86_xsave_layout::sizeof_xsave.  So, when writing the core
>> file note for the XSAVE state, it writes a note of size 896, even though
>> it doesn't write the PKRU state.  When loading back the core, GDB tries
>> to figure out the layout of the XSAVE area based on what features are
>> enabled in XCR0 and the size of the note (the size of the XSAVE area).
>> Since my combination of XCR0 and size of XSAVE area doesn't match any
>> combination known by GDB, GDB falls back to a gdbarch supporting only
>> x87 and SSE.
>>
>> This patch changes GDB to populate the x86_xsave_layout::sizeof_xsave
>> field (and consequently the size of the XSAVE state note in core files)
>> using EBX, the size of the XSAVE area required by currently enabled
>> features in XCR0.  This makes i387_guess_xsave_layout recognize my case
>> with this condition:
>>
>>    else if (HAS_AVX (xcr0) && xsave_size == 832)
>>      {
>>        /* Intel and AMD CPUs supporting AVX.  */
>>        layout.avx_offset = 576;
>>      }
>>
>> In other words, just as if my machine didn't support PKRU at all.
>>
>> Another reason why I think this change makes sense is that XSAVE state
>> notes in kernel-generated cores on this machine have size 832.  So this
>> change makes GDB-generated cores more similar to kernel-generated ones,
>> reducing the diversity of XSAVE state notes that GDB needs to be able to
>> figure out.
>>
>> Note that if PKRU was enabled on my machine, then the effective XSAVE
>> area size would be 896 bytes.  We would need to add a case in
>> i387_guess_xsave_layout for that combination, since there is no
>> currently.  But I don't have a way to test that right now, since I don't
>> know why PKRU is disabled.
>>
>> Change-Id: If64f30307f3a2e5ca3e1fd1cb7379ea840805a85
>> ---
>>   gdb/nat/x86-xstate.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/gdb/nat/x86-xstate.c b/gdb/nat/x86-xstate.c
>> index 9fdc572356ab..5ae014af4f49 100644
>> --- a/gdb/nat/x86-xstate.c
>> +++ b/gdb/nat/x86-xstate.c
>> @@ -42,11 +42,11 @@ xsave_feature_offset (uint64_t xcr0, int feature)
>>   int
>>   x86_xsave_length ()
>>   {
>> -  uint32_t ecx;
>> +  uint32_t ebx;
>>   -  if (!x86_cpuid_count (0xd, 0, nullptr, nullptr, &ecx, nullptr))
>> +  if (!x86_cpuid_count (0xd, 0, nullptr, &ebx, nullptr, nullptr))
>>       return 0;
>> -  return ecx;
>> +  return ebx;
>>   }
>>     /* See x86-xstate.h.  */
>>
>> base-commit: 4befded43f524d0840bb88fff7b77415b73a3851
> 
> Reviewed-By: John Baldwin <jhb@FreeBSD.org>
> 
> One further note is that the Linux x86 arches use x86_xsave_length() to infer
> ("guess") the size of the XSAVE register set that the Linux kernel writes out
> in core dumps.  On FreeBSD x86 arches, GDB is able to query this size directly
> from the kernel via ptrace.  My use of ECX for this guess earlier was just not
> the best guess.  In the case that the kernel enables all of the available
> features, then ECX and EBX have the same values, so this only matters for a
> system where the kernel has enabled a subset of available XSAVE extensions.

Thanks, will push.  I'll copy paste that note in the commit message for
posterity, it might help people (including me) in the future.

Simon