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/11/23 9:46 AM, Simon Marchi wrote:
> On 4/11/23 12:19, John Baldwin wrote:
>> On 4/10/23 7:23 PM, Simon Marchi wrote:
>>> 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.
>>
>> Well, you need the value of xcr0 as well as the total size, though it
>> is possible to execute XGETBV in userspace I believe.  I had wanted to
>> ensure we were using the same mask ptrace is going to export vs assuming
>> that the mask of the running GDB process will match the mask of all
>> target processes.  I guess in practice we are assuming that though via
>> the caching.
> 
> Who sets xcr0, the kernel or the userspace?  In theory could we have
> xcr0 values that differ per process?  Could the xcr0 value for a process
> change over time?  To be clear, I'm just asking to understand better, I
> don't suggest to support these cases if they don't happen in reality.

Only the kernel sets it because the kernel is the one that allocates
space for the XSAVE area for each thread to save/restore state during
context switches.  Also, the relevant instruction (XSETBV) is kernel
only.  Possibly a kernel could choose to alter it between processes, but
if that were the case that would be an argument to not do any caching
and always check it in read_description() for the relevant inferior.
Neither Linux nor FreeBSD do this though, both determine a value to use
at boot and then never change it.  (Well, not in a way relevant to GDB,
FreeBSD can choose to use a smaller mask for guest VMs while the guest
is running, but that value of XCR0 is never visible to GDB, and even if
GDB is debugging the userspace portion of the hypervisor it always sees
the "host" values of XSAVE registers, not guest values.)

>> Note that the read_description methods for both Linux and
>> FreeBSD already use ptrace for other things.  Both use ptrace to decide
>> if the inferior is using a 64-bit vs 32-bit tdesc.  For 32-bit both use
>> ptrace to determine if there is support for a custom ptrace op to fetch
>> SSE/XMM register state (PTRACE_GETFPXREGS on Linux, PT_GETXMMREGS on
>> FreeBSD), so this case is already quite broken.
> 
> Heh, you're right, I ran those commands on x86 Linux and didn't even
> notice the error message!
> 
>      (gdb) target native
>      Done.  Use the "run" command to start a process.
>      (gdb) unset tdesc filename
>      Couldn't get CS register: No such process.
> 
> So yeah, perhaps make sure this corner case is relatively correctly
> handled.  That probably means skipping over the ptrace check if
> inferior_ptid is null_ptid, and make sure you don't cache that value (so
> we actually probe it next time around).

I think the right answer for theses cases is that the read_description
methods need to just return a specific base-line description if
inferior_ptid == null_ptid before doing any of the ptrace, etc. checks.
Probably for 64-bit it means a simple 64-bit tdesc with SSE for example.

-- 
John Baldwin