From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 100080 invoked by alias); 21 Mar 2015 19:14:54 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 99264 invoked by uid 89); 21 Mar 2015 19:14:53 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_SOFTFAIL autolearn=no version=3.3.2 X-HELO: bigwig.baldwin.cx Received: from bigwig.baldwin.cx (HELO bigwig.baldwin.cx) (96.47.65.170) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (CAMELLIA256-SHA encrypted) ESMTPS; Sat, 21 Mar 2015 19:14:51 +0000 Received: from John-Baldwins-MacBook-Pro.local (d-69-161-105-82.cpe.metrocast.net [69.161.105.82]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 10E8AB915; Sat, 21 Mar 2015 15:14:49 -0400 (EDT) Message-ID: <550DC328.40800@FreeBSD.org> Date: Sat, 21 Mar 2015 19:14:00 -0000 From: John Baldwin User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Pedro Alves , gdb-patches@sourceware.org, "kettenis@gnu.org" Subject: Re: [PATCH] Add support for the x86 XSAVE extended state on FreeBSD/x86. References: <2672674.t3ZJOKnpzU@ralph.baldwin.cx> <5509D93A.5030707@redhat.com> In-Reply-To: <5509D93A.5030707@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-03/txt/msg00691.txt.bz2 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 > 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