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

[Simon Marchi <simark@simark.ca>]

On 4/10/23 16:42, John Baldwin wrote:
> 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.

Perfect, makes sense.

>> 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.

I looked at these other methods and came to the same conclusion as you.
For gdbarch_core_xfer_siginfo it doesn't really introduce any extra
complexity, because we just pass down the offset/len parameters to
bfd_get_section_contents.  For gdbarch_core_xfer_shared_libraries,
seeing that the sole implementation (windows_core_xfer_shared_libraries)
just re-generates the whole XML string every time, I think we could
simplify its interface as well, making it return just an std::string,
and handle the partial read case in the target.  Not important for what
you're doing, I'm just thinking out loud.

But now it makes me wonder what really belongs as an object in the
xfer_partial interface, versus what belongs as just a regular target
method.  Clearly, the xfer partial interface is good to transfer some
raw data where the transferred size can be smaller than expected.  Like
when you make a read/write syscall.  But if the goal is to transfer a
structure from one side of GDB to the other (like we do with
x86_xsave_layout), wouldn't it be simpler to have a normal target
method that returns that struct?

Simon