Re: [PATCH v4 02/13] core: Support fetching TARGET_OBJECT_X86_XSAVE_LAYOUT from architectures.

[John Baldwin <jhb@FreeBSD.org>]

On 4/6/23 12:28 PM, Simon Marchi wrote:
> On 3/17/23 21:08, John Baldwin wrote:
>> Add gdbarch_core_xfer_x86_xsave_layout to fetch the x86 XSAVE layout
>> structure from a core file.
>>
>> Current OS's do not export the offsets of XSAVE state components in
>> core dumps, so provide an i387_set_xsave_layout helper function to set
>> offsets based on known combinations of XCR0 masks and total state
>> sizes.  Eventually when core dumps do contain this information this
>> function should only be used as a fall back for older core dumps.
> 
> Not a big deal, but it's a bit odd to add the i387_set_xsave_layout
> function here, but not use it.  I could see it being added with the
> first patch that uses and, and the current patch just adding the
> gdbarch_core_xfer_x86_xsave_layout gdbarch method + the call in
> corelow.c.

Humm, the reason I added it here is that it is part of OS-independent
infrastructure that will later be used by OS-specific ABIs.  Adding
this function (or other infrastructure) as part of the commit adding
FreeBSD amd64/i386 support seemed a bit odd to me.  I wanted it to be
clear which parts are OS-specific vs not in the history in part so it
is easier for other OS's to add XSAVE support in the future.

> More minor comments below.
> 
>> diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
>> index 92c501d2a72..0a1c5d927df 100644
>> --- a/gdb/gdbarch_components.py
>> +++ b/gdb/gdbarch_components.py
>> @@ -1666,6 +1666,19 @@ of bytes read (zero indicates EOF, a negative value indicates failure).
>>       predicate=True,
>>   )
>>   
>> +Method(
>> +    comment="""
>> +Read offset OFFSET of TARGET_OBJECT_X86_XSAVE_LAYOUT from core file
>> +into buffer READBUF with length LEN.  Return the number of bytes read
>> +(zero indicates EOF, a negative value indicates failure).
>> +""",
>> +    type="LONGEST",
>> +    name="core_xfer_x86_xsave_layout",
>> +    params=[("gdb_byte *", "readbuf"), ("ULONGEST", "offset"), ("ULONGEST", "len")],
>> +    predicate=True,
>> +    invalid=True,
> 
> You can remove `invalid=True`, following 564cddf8edc7 ("gdbarch: make
> invalid=True the default for all Components").
> 
> I have yet to see how this is going to be implemented (in subsequent
> patches), but I wonder if we really need to pass use readbuf / offset /
> len in this API.  Wouldn't it be possible to pass a pointer or reference
> to an x86_xsave_layout object, and have the arch fill it?  I understand
> that this matches the taret_read interface (we have to shoehorn the
> x86_xsave_layout through that interface), but I don't think it needs to
> propagate to the gdbarch method.

Hmm, passing down the readbuf, offset, len fields is consistent with other
gdbarch methods called from core_target::xfer_partial such as
gdbarch_core_xfer_shared_libraries and gdbarch_core_xfer_siginfo.

That said, it would simplify the implementation in the architectures if
core_target::xfer_partial handled the partial reads and always read the
full thing from the gdbarch.  In particular unlike the other above methods,
the thing being read is a fixed size.  I'll make that change as I think
it will be nicer, thanks.

>> diff --git a/gdb/i387-tdep.c b/gdb/i387-tdep.c
>> index df0a6058adc..7026dfed6e4 100644
>> --- a/gdb/i387-tdep.c
>> +++ b/gdb/i387-tdep.c
>> @@ -898,6 +898,61 @@ static int xsave_pkeys_offset[] =
>>     (xsave + xsave_pkeys_offset[regnum - I387_PKRU_REGNUM (tdep)])
>>   
>>   
>> +/* See i387-tdep.h.  */
>> +
>> +bool
>> +i387_set_xsave_layout (uint64_t xcr0, size_t xsave_size,
>> +		       x86_xsave_layout *layout)
>> +{
>> +  if (HAS_PKRU(xcr0) && xsave_size == 2696)
> 
> Space before parenthesis (a few times in this function).

Fixed.

-- 
John Baldwin