Re: [PATCH] i386: Use a fallback XSAVE layout for remote targets

[Joel Brobecker <brobecker@adacore.com>]

Hello,

On Tue, Nov 21, 2023 at 02:22:18PM -0800, John Baldwin wrote:
> On 11/21/23 2:09 PM, John Baldwin wrote:
> > If a target provides a target description including registers from the
> > XSAVE extended region, but does not provide an XSAVE layout, use a
> > fallback XSAVE layout based on the included registers.  This fallback
> > layout matches GDB's behavior in earlier releases which assumes the
> > layout from Intel CPUs.
> > 
> > This fallback layout is currently only used for remote targets since
> > native targets which support XSAVE provide an explicit layout derived
> > from CPUID.
> > 
> > PR gdb/30912
> > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30912
> 
> This fixes the regression for me locally.  It would perhaps be cleaner
> to isolate the regression to the remote target by moving the new logic
> in i386_gdbarch_init into remote::fetch_x86_xsave_layout.  The one
> caveat of that is getting access to the right tdesc.  I could perhaps
> change target::fetch_x86_xsave_layout to pass down the tdesc pointer
> (or maybe the gdbarch_info?).  Alternatively it might be sufficient to
> just use target_current_description() as the tdesc in remote.c?  It
> wasn't clear to me if the relevant tdesc in this case was always the
> same as target_current_description() (though I suspect it probably is).

I don't know if it would be cleaner, actually. Maybe safer in the sense
that it would further limit the possible impact zone of your patch, but
I thought the way you approached it was nicely general so that, if we
had any other kind of target system that suffered from the same issue,
we'd get the same fix as part of it.

Just my limited 2 cents, though. It could be that I'm seeing this
from the wrong perspective, or misunderstood something...

-- 
Joel