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

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

On 2023-07-28 12:30, John Baldwin wrote:
> 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.

I previously considered suggesting making the *_core_read_xsave_info
functions return gdb::optional<int>, where they would have returned an
empty optional if the core does not have xsave info.  The calling code
could then fall back to some other strategy.  Returning 0 achieves the
same thing in a different way, it's fine with me (as long as it's
properly documented).  I prefer that over the "passing a default value"
approach.

If the changes only touch this patch, you can send an update just for
this one (instead of the whole series).

Simon