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

[John Baldwin]

On 7/28/23 10:58 AM, Simon Marchi wrote:
> 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).

It touches both this one and the similar FreeBSD one, so I've sent both.
This patch (patch 8) also now contains one extra change from Keith's
testing (testing sizeof_xsave != 0 to control use of .reg-xstate for
i386)

-- 
John Baldwin