Re: [PATCH v4 06/13] gdb: Support XSAVE layouts for the current host in the FreeBSD x86 targets.

[Simon Marchi <simark@simark.ca>]

On 4/10/23 17:27, John Baldwin wrote:
> On 4/6/23 1:18 PM, Simon Marchi wrote:
>>> @@ -43,3 +46,51 @@ x86_fbsd_nat_target::low_new_fork (ptid_t parent, pid_t child)
>>>     child_state = x86_debug_reg_state (child);
>>>     *child_state = *parent_state;
>>>   }
>>> +
>>> +#ifdef PT_GETXSTATE_INFO
>>> +/* Implement the "xfer_partial" target_ops method.  */
>>> +
>>> +enum target_xfer_status
>>> +x86_fbsd_nat_target::xfer_partial (enum target_object object,
>>> +                   const char *annex, gdb_byte *readbuf,
>>> +                   const gdb_byte *writebuf,
>>> +                   ULONGEST offset, ULONGEST len,
>>> +                   ULONGEST *xfered_len)
>>> +{
>>> +  switch (object)
>>> +    {
>>> +    case TARGET_OBJECT_X86_XSAVE_LAYOUT:
>>> +      if (xsave_layout.sizeof_xsave == 0)
>>> +    return TARGET_XFER_E_IO;
>>
>> I'm wondering why you don't call probe_xsave_layout before using
>> xsave_layout here.  I guess because you assume that the child class will
>> have called probe_xsave_layout at some point before?
> 
> Yes.  Note that the default value of xsave_layout (all zeroes) will result
> in this failing to read the object if it hasn't been called (rather than
> undefined behavior).
> 
>> To make that more robust, could we access xsave_layout through a method
>> like:
>>
>>    const x86_xsave_layout &xsave_layout ();
>>
>> which would take care of the caching?  Callers wouldn't have to worry
>> about calling probe_xsave_layout before.  There could be another method
>> to access xsave_info.
> 
> Well, part of the trick here is that probe_xsave_layout takes a pid
> argument that it passes to ptrace () to fetch the layout, but at the
> point of xfer_partial I don't necessarily have a pid.  It's true that
> the only callers (the target ::read_description methods) all use
> inferior_ptid's pid, so I could perhaps use inferior_ptid directly,
> but that seems somewhat dubious.

Using inferior_ptid would probably be fine... I think?  It's how targets
know which thread's memory to read from or write to, for the
TARGET_OBJECT_MEMORY object.  The question is, will all calls to
xfer_partial with the TARGET_OBJECT_X86_XSAVE_LAYOUT object be done with
inferior_ptid set to a stopped thread that you can used to do the ptrace
query?

Ok, that made me think about an unlikely (but still possible
scenario) where the read_description target method can be called with
inferior_ptid == null_ptid:

(gdb) target native  # push the native target
(gdb) unset tdesc filename

I think the code as it is in this patch would try to call ptrace with
pid 0.

The approach of fetching the xsave layout using ptrace has this
particularity that you need a stopped thread to access it.  I'm
wondering, is there an advantage in fetching the xsave layout using
ptrace instead of the cpuid method?  The latter has the advantage of
working at any time.

>>> +
>>> +      if (offset > sizeof (xsave_layout))
>>> +    return TARGET_XFER_E_IO;
>>> +
>>> +      if (offset + len > sizeof (xsave_layout))
>>> +    len = sizeof (xsave_layout) - offset;
>>> +
>>> +      memcpy (readbuf, ((gdb_byte *) &xsave_layout) + offset, len);
>>> +      *xfered_len = len;
>>> +      return len == 0 ? TARGET_XFER_EOF : TARGET_XFER_OK;
>>> +    default:
>>> +      return fbsd_nat_target::xfer_partial (object, annex, readbuf, writebuf,
>>> +                        offset, len, xfered_len);
>>
>> Since this class inherits from x86bsd_nat_target<fbsd_nat_target>,
>> should this technically call
>> x86bsd_nat_target<fbsd_nat_target>::xfer_partial?  I don't really know,
>> I've never used this funky inheritance pattern myself.  And it won't
>> change anything in practice right now, because not target between this
>> one and fbsd_nat_target implements xfer_partial.
> 
> Oh, that is more correct, yes.

Ok, I think there was the same issue for the Linux class hierarchy too,
I don't remember if I pointed it out.

>>> diff --git a/gdb/x86-fbsd-nat.h b/gdb/x86-fbsd-nat.h
>>> index 0a1308f3584..1b5cdfc1a9b 100644
>>> --- a/gdb/x86-fbsd-nat.h
>>> +++ b/gdb/x86-fbsd-nat.h
>>> @@ -27,10 +32,32 @@
>>>     class x86_fbsd_nat_target : public x86bsd_nat_target<fbsd_nat_target>
>>>   {
>>> +  void low_new_fork (ptid_t parent, pid_t child) override;
>>> +
>>> +public:
>>>     bool supports_stopped_by_hw_breakpoint () override
>>>     { return true; }
>>
>> Is is on purpose that supports_stopped_by_hw_breakpoint is moved to the
>> public section?  I guess so, because it is public in reality
>> (inherited from target_ops).
> 
> Yes, that was an old bug, I can split that out into a separate patch though.

Bah... as you wish.  You can push an obvious patch if you'd like to keep
this one really clean.

Simon