Re: [PATCH v5 09/19] gdb: Update x86 FreeBSD architectures to support XSAVE layouts.

[Simon Marchi <simark@simark.ca>]

On 5/3/23 19:45, John Baldwin wrote:
> On 5/3/23 10:14 AM, Simon Marchi wrote:
>>> @@ -285,7 +291,9 @@ i386fbsd_core_read_description (struct gdbarch *gdbarch,
>>>                   struct target_ops *target,
>>>                   bfd *abfd)
>>>   {
>>> -  return i386_target_description (i386fbsd_core_read_xcr0 (abfd), true);
>>> +  x86_xsave_layout layout;
>>> +  return i386_target_description (i386_fbsd_core_read_xsave_info (abfd, layout),
>>> +                  true);
>>
>> Reading this gives me some questions.  Just thinking out loud, nothing
>> necessarily actionable at the moment
>>
>> I found it strange that i386_fbsd_core_read_xsave_info fills an
>> x86_xsave_layout object that we don't use.  We get xcr0 to generate an
>> appropriate target description here, and later we'll call
>> target_fetch_x86_xsave_layout (when initializing the gdbarch) to get the
>> x86_xsave_layout and save it in the i386_gdbarch_tdep object.  I'm
>> wondering if, design-wise, this means that the target_desc object should
>> carry the xsave layout information.  It would be saved in the tdesc
>> here, and i386_gdbarch_init would just get it from the tdesc.
>>
>> It's probably not as simple as that, since target descriptions are
>> transferred as XML from remote targets, and you still have to consider
>> older target descriptions that wouldn't include that information.  But
>> I'm just trying to think what the ideal design would be.
> 
> It's kind of odd as the layout doesn't affect the set of registers that
> are available (and today tdesc's are AFAICT just about which registers
> the target provides).

Yeah, so I'm thinking that this could be some arch-specific information
in addition to the register list.

> The reason I extended the existing method that reads xcr0 is that I
> needed the section size along with the value of xcr0 to compute the
> layout via i386_set_xsave_layout and I was trying to avoid duplicating
> the code to fetch the section.  I could perhaps change the function
> to return the value of xcr0 and the section size instead if that is
> less confusing?

I don't think any change is necessary to your code, it's ok like that.

>>> diff --git a/gdb/i386-fbsd-tdep.h b/gdb/i386-fbsd-tdep.h
>>> index cb991af9e49..f96c00d45eb 100644
>>> --- a/gdb/i386-fbsd-tdep.h
>>> +++ b/gdb/i386-fbsd-tdep.h
>>> @@ -20,10 +20,16 @@
>>>   #ifndef I386_FBSD_TDEP_H
>>>   #define I386_FBSD_TDEP_H
>>>   +#include "gdbsupport/x86-xstate.h"
>>>   #include "regset.h"
>>>   -/* Get XSAVE extended state xcr0 from core dump.  */
>>> -extern uint64_t i386fbsd_core_read_xcr0 (bfd *abfd);
>>> +/* Validate and fetch XSAVE extended state xcr0 and extended area
>>> +   layout from core dump.  */
>>> +uint64_t i386_fbsd_core_read_xsave_info (bfd *abfd, x86_xsave_layout &layout);
>>
>> I was a bit confused when I read the comment above for the first time.
>> Can you rephrase it to make it clear that the function returns the XSAVE
>> extended state, and fills LAYOUT?
>>
>> Also, what does "validate" mean, what happens if the thing we validate
>> is not valid?
> 
> Hmm, the intent is "Validate and fetch xcr0 and the XSAVE layout".  In
> particular, this does not return the extended state (which would be the
> value of all the AVX registers, etc.) but instead returns only the mask of
> enabled state (xcr0) and the layout of the extended area.
> 
> If xcr0 does not appear valid or the section doesn't seem to have a valid
> size (i386_set_xsave_layout returns false) then the function claims that
> only SSE is reported by returning X86_XSTATE_SSE_MASK.  That is the
> validation that "validate" is referring to.

Maybe it's the two "and" in the sentence that make it ambiguous.  So,
perhaps:

  Read the xcr0 value from the ABFD core file.  If it appears to be valid,
  return it and fill LAYOUT with values inferred from that value.

  Otherwise, return X86_XSTATE_SSE_MASK as a fallback and leave LAYOUT
  untouched.

Simon