This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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.

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.

>    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.  */

>  
> +static const struct target_desc *
> +amd64fbsd_read_description (struct target_ops *ops)
> +{
> +#ifdef PT_GETXSTATE_INFO
> +  static int xsave_probed;
> +  static uint64_t xcr0;
> +#endif
> +  struct reg regs;
> +  int is64;
> +
> +  if (ptrace (PT_GETREGS, ptid_get_pid (inferior_ptid),
> +	      (PTRACE_TYPE_ARG3) &regs, 0) == -1)
> +    perror_with_name (_("Couldn't get registers"));
> +  is64 = (regs.r_cs == GSEL (GUCODE_SEL, SEL_UPL));
> +#ifdef PT_GETXSTATE_INFO
> +  if (!xsave_probed)
> +    {
> +      struct ptrace_xstate_info info;
> +
> +      if (ptrace (PT_GETXSTATE_INFO, ptid_get_pid (inferior_ptid),
> +		  (PTRACE_TYPE_ARG3) &info, sizeof(info)) == 0)

Space before parens.

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


> @@ -169,6 +172,71 @@ static int amd64fbsd_jmp_buf_reg_offset[] =
>    0 * 8				/* %rip */
>  };
>  
> +static const struct target_desc *
> +amd64fbsd_core_read_description (struct gdbarch *gdbarch,
> +				 struct target_ops *target,
> +				 bfd *abfd)

Likewise, comment.

> +/* 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

Thanks,
Pedro Alves


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]