Re: [PATCH] Add support for the x86 XSAVE extended state on FreeBSD/x86.

[John Baldwin <jhb at FreeBSD dot org>]

On 3/18/15 3:59 PM, Pedro Alves wrote:
> Hi John,
> 
> The bfd/ changes are owned by binutils@.  The repo is the same,
> so you'll end up pushing it all in one commit, but you'll need an
> OK from the binutils maintainers for that bit.  Please resend
> with binutils@ in TO/CC.

Will do for the updated patch.

> Overall this looks very good to me.  Mark do you have comments?
> 
>> @@ -9490,6 +9493,8 @@ elfcore_write_xstatereg (bfd *abfd, char *buf, int *bufsiz,
>>  			 const void *xfpregs, int size)
>>  {
>>    char *note_name = "LINUX";
>> +  if (get_elf_backend_data (abfd)->elf_osabi == ELFOSABI_FREEBSD)
>> +      note_name = "FreeBSD";
> 
> Alignment here looks odd.

Yes, will fix.
 
>>    return elfcore_write_note (abfd, buf, bufsiz,
>>  			     note_name, NT_X86_XSTATE, xfpregs, size);
>>  }
> 
> We've been trying to make sure that all functions have
> an intro comment.  For hook implementations, the comment
> should just point at the hook implemented.  Something like:
> 
> /* Implement the to_read_description method.  */

Ok.

>> +      if (ptrace (PT_GETXSTATE_INFO, ptid_get_pid (inferior_ptid),
>> +		  (PTRACE_TYPE_ARG3) &info, sizeof(info)) == 0)
> 
> Space before parens.

Ok.
 
>> +  if (x86_xsave_len != 0)
>> +    {
>> +      switch (xcr0 & X86_XSTATE_ALL_MASK)
>> +	{
>> +	case X86_XSTATE_MPX_AVX512_MASK:
>> +	case X86_XSTATE_AVX512_MASK:
>> +	  if (is64)
>> +	    return tdesc_amd64_avx512;
>> +	  else
>> +	    return tdesc_i386_avx512;
>> +	case X86_XSTATE_MPX_MASK:
>> +	  if (is64)
>> +	    return tdesc_amd64_mpx;
>> +	  else
>> +	    return tdesc_i386_mpx;
>> +	case X86_XSTATE_AVX_MASK:
>> +	  if (is64)
>> +	    return tdesc_amd64_avx;
>> +	  else
>> +	    return tdesc_i386_avx;
>> +	default:
>> +	  if (is64)
>> +	    return tdesc_amd64;
>> +	  else
>> +	    return tdesc_i386;
>> +	}
> 
> These xcr0 -> tdesc mappings need to appear in multiple places.
> I wonder whether it'd make sense to put them in a single helper
> function (in the fbsd tdep file) that takes "xcr0" and "is64" as
> parameters, and returns the corresponding tdesc.

There are a couple of options I've thought about for this.  One
has been to have a shared to_read_description implementation in
an x86fbsd-nat.c (Linux uses a shared one in x86-linux-nat.c).
However, these case statements are also not really FreeBSD (or BSD)
specific.  What if I added functions in amd64-tdep.c and i386-tdep.c
that returned the correct target description for a given xcr0
value?  Something like:

struct target_desc *
i386_target_description(uint64_t xcr0)
{

  /* i386 switch statement here */
}

That could be reused for the core read_description callback as
well as the native ones.  This could also be reused by other
systems that grow XSAVE support in the future.


>> +/* Iterate over core file register note sections.  */
>> +
>> +static void
>> +amd64fbsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
>> +					iterate_over_regset_sections_cb *cb,
>> +					void *cb_data,
>> +					const struct regcache *regcache)
>> +{
>> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>> +
>> +  cb (".reg", tdep->sizeof_gregset, &i386_gregset, NULL, cb_data);
>> +  cb (".reg2", tdep->sizeof_fpregset, &amd64_fpregset, NULL, cb_data);
>> +  cb (".reg-xstate", regcache ? X86_XSTATE_MAX_SIZE : 0,
>> +      &amd64fbsd_xstateregset, "XSAVE extended state", cb_data);
> 
> I think you'll want to update this per:
> 
> commit dde9acd693251ccbe28d2d9c6c8b3cdc8ca884ed
> Author:     Andreas Arnez <arnez@linux.vnet.ibm.com>
> AuthorDate: Wed Jan 14 17:53:23 2015 +0000
> 
>     x86: Use correct .reg-xstate section size

Ah, yes, thanks for the pointer.  I wanted to do that anyway to
generate more accurate notes for gcore.

-- 
John Baldwin