From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 53229 invoked by alias); 18 Mar 2015 20:00:13 -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 53208 invoked by uid 89); 18 Mar 2015 20:00:12 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 18 Mar 2015 20:00:06 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t2IJxviL001856 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Wed, 18 Mar 2015 15:59:57 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t2IJxtBB004133; Wed, 18 Mar 2015 15:59:56 -0400 Message-ID: <5509D93A.5030707@redhat.com> Date: Wed, 18 Mar 2015 20:00:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: John Baldwin , 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> In-Reply-To: <2672674.t3ZJOKnpzU@ralph.baldwin.cx> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2015-03/txt/msg00562.txt.bz2 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) ®s, 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 AuthorDate: Wed Jan 14 17:53:23 2015 +0000 x86: Use correct .reg-xstate section size Thanks, Pedro Alves