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

[Simon Marchi <simark@simark.ca>]

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.

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.

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

Simon