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

[John Baldwin <jhb@FreeBSD.org>]

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.

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

>> +    }
>> +}
>> +
>> +void
>> +x86_fbsd_nat_target::probe_xsave_layout (pid_t pid)
>> +{
>> +  if (xsave_probed)
>> +    return;
>> +
>> +  xsave_probed = true;
>> +
>> +  if (ptrace (PT_GETXSTATE_INFO, pid, (PTRACE_TYPE_ARG3) &xsave_info,
>> +	      sizeof (xsave_info)) != 0)
>> +    return;
>> +  if (xsave_info.xsave_len != 0)
>> +    x86_fetch_xsave_layout (xsave_info.xsave_mask, xsave_info.xsave_len,
>> +			    xsave_layout);
>> +}
>> +#endif
>> 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.

>> +protected:
>> +  void probe_xsave_layout (pid_t pid);
>> +
>> +  struct ptrace_xstate_info xsave_info;
>> +  x86_xsave_layout xsave_layout;
>> +
>> +private:
>> +  bool xsave_probed;
> 
> Private and protected members should be prefixed with m_.

Fixed.

>> +#endif
>>   };
>>   
>> +#ifdef PT_GETXSTATE_INFO
>> +#endif
> 
> I guess these last two lines are not needed?

Oops, yes, leftovers from earlier versions.

-- 
John Baldwin