Re: [PATCH v6 08/15] gdb: Update x86 Linux architectures to support XSAVE layouts.

[John Baldwin <jhb@FreeBSD.org>]

On 7/27/23 2:48 PM, Simon Marchi wrote:
> 
>> Ooo, that's a good catch.  This function is shared with amd64, so I think
>> it's best if it keeps returning an xcr0 value, but we could patch
>> i386_linux_core_read_description to instead do this:
>>
>> static const struct target_desc *
>> i386_linux_core_read_description (struct gdbarch *gdbarch,
>>                    struct target_ops *target,
>>                    bfd *abfd)
>> {
>>    /* Linux/i386.  */
>>    x86_xsave_layout layout;
>>    uint64_t xcr0 = i386_linux_core_read_xsave_info (abfd, layout);
>>    if (xcr0 == X86_XSTATE_X87_MASK
>>        && bfd_get_section_by_name (abfd, ".reg-xfp") != NULL)
>>      xcr0 = X86_XSTATE_SSE_MASK;
>>
>>    return i386_linux_read_description (xcr0);
>> }
> 
> And i386_linux_core_read_xsave_info would return X86_XSTATE_X87_MASK if
> it does not find a .reg-xstate section?

Hmmm.  It would need to do something like that yes.  I realize I have a
bug for i386-fbsd as well where it would return SSE when it should be
returning X87.  For amd64, both Linux and FreeBSD use FXSAVE (which
includes SSE) as .reg2 (generic FP regs).  For i386, both use FSAVE (which
only includes X87) for .reg2, and Linux/i386 also has .reg-fxp that uses
FXSAVE.  This means that the "default" xcr0 value really should be SSE
for amd64, and X87 for i386.

I was considering returning a bool from the helper methods
(i386_*_core_read_xsave_info yesterday (it makes the new gdbarch method
implementations slightly easier to read IMO).  Other options are to add a
parameter to the helper that is the "default" value of xcr0 to use, or to
return a value of 0 for xcr0 when no valid XSAVE info is found.  Returning
0 is pretty close to the bool approach without requiring a dummy xcr0 arg
in the gdbarch methods, so I think I'll go with that.

In that case, i386_linux_core_read_description would go back to what it
was in this patch, but the amd64 version would change slightly:

static const struct target_desc *
amd64_linux_core_read_description (struct gdbarch *gdbarch,
				  struct target_ops *target,
				  bfd *abfd)
{
   /* Linux/x86-64.  */
   x86_xsave_layout layout;
   uint64_t xcr0 = i386_linux_core_read_xsave_info (abfd, layout);
   if (xcr0 == 0)
     xcr0 = X86_XSTATE_SSE_MASK;

   return amd64_linux_read_description (xcr0 & X86_XSTATE_ALL_MASK,
				       gdbarch_ptr_bit (gdbarch) == 32);
}

Namely to add the 'xcr == 0' case.  If that approach seems sensible I
will post a new series since it touches both this patch and the FreeBSD
one.

-- 
John Baldwin