This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Add support for the x86 XSAVE extended state on FreeBSD/x86.
- From: John Baldwin <jhb at FreeBSD dot org>
- To: Pedro Alves <palves at redhat dot com>, gdb-patches at sourceware dot org, "kettenis at gnu dot org" <kettenis at gnu dot org>
- Date: Sat, 21 Mar 2015 15:14:48 -0400
- Subject: Re: [PATCH] Add support for the x86 XSAVE extended state on FreeBSD/x86.
- Authentication-results: sourceware.org; auth=none
- References: <2672674 dot t3ZJOKnpzU at ralph dot baldwin dot cx> <5509D93A dot 5030707 at redhat dot com>
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