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

Simon Marchi via Gdb-patches
2023-07-28T17:58:53Z

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