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

[Simon Marchi <simark@simark.ca>]

On 11/27/23 16:04, John Baldwin wrote:
> On 11/27/23 12:47 PM, John Baldwin wrote:
>> On 11/27/23 11:30 AM, Simon Marchi wrote:
>>> On 11/21/23 17:22, 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).
>>>
>>> My understanding is that the fallback is used for targets that can debug
>>> x86, where xcr0 may claim that the inferior has extended state
>>> registers, but where the target is unable to provide an xsave layout.
>>> There probably won't ever be another target than the remote target
>>> in this situation, so if you can make the fix local to the remote
>>> target that would be nice.  But really I would also be fine with what
>>> you have.
> 
> I took at stab at moving this into the remote target, but ran into an
> ugly bit which is that this requires calling these new functions
> from i38[67]-tdep.c from remote.c.  This means remote::fetch_x86_xsave_layout
> would need a suitable #define so it could be conditional on the the i386
> target being enabled.  The posted patch does avoid this problem by keeping
> the logic internal to i38[67]-tdep.c.

Ack, thanks for trying.

Simon