From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16499 invoked by alias); 26 Oct 2007 00:04:14 -0000 Received: (qmail 16488 invoked by uid 22791); 26 Oct 2007 00:04:12 -0000 X-Spam-Check-By: sourceware.org Received: from mtagate2.de.ibm.com (HELO mtagate2.de.ibm.com) (195.212.29.151) by sourceware.org (qpsmtpd/0.31) with ESMTP; Fri, 26 Oct 2007 00:04:07 +0000 Received: from d12nrmr1607.megacenter.de.ibm.com (d12nrmr1607.megacenter.de.ibm.com [9.149.167.49]) by mtagate2.de.ibm.com (8.13.8/8.13.8) with ESMTP id l9Q04453104686 for ; Fri, 26 Oct 2007 00:04:04 GMT Received: from d12av02.megacenter.de.ibm.com (d12av02.megacenter.de.ibm.com [9.149.165.228]) by d12nrmr1607.megacenter.de.ibm.com (8.13.8/8.13.8/NCO v8.5) with ESMTP id l9Q0442n2265258 for ; Fri, 26 Oct 2007 02:04:04 +0200 Received: from d12av02.megacenter.de.ibm.com (loopback [127.0.0.1]) by d12av02.megacenter.de.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id l9Q044XB016513 for ; Fri, 26 Oct 2007 02:04:04 +0200 Received: from tuxmaker.boeblingen.de.ibm.com (tuxmaker.boeblingen.de.ibm.com [9.152.85.9]) by d12av02.megacenter.de.ibm.com (8.12.11.20060308/8.12.11) with SMTP id l9Q044gn016510; Fri, 26 Oct 2007 02:04:04 +0200 Message-Id: <200710260004.l9Q044gn016510@d12av02.megacenter.de.ibm.com> Received: by tuxmaker.boeblingen.de.ibm.com (sSMTP sendmail emulation); Fri, 26 Oct 2007 02:04:04 +0200 Subject: Re: [patch] Add support for PPC Altivec registers in core files To: cseo@linux.vnet.ibm.com (Carlos Eduardo Seo) Date: Fri, 26 Oct 2007 00:13:00 -0000 From: "Ulrich Weigand" Cc: gdb-patches@sourceware.org (GDB Patches Mailing List) In-Reply-To: <471CE30E.9040705@linux.vnet.ibm.com> from "Carlos Eduardo Seo" at Oct 22, 2007 03:51:10 PM X-Mailer: ELM [version 2.5 PL2] MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit 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 X-SW-Source: 2007-10/txt/msg00687.txt.bz2 Carlos Eduardo Seo wrote: > Recently, a kernel patch by Mark Nelson added support for Altivec > registers in core dumps under a new note section called NT_PPC_VMX. Two > other patches (by myself and Roland McGrath) added support for the new > note section in binutils. Now, this patch adds support for Altivec > registers debugging in core files, using the contents under NT_PPC_VMX. I've noticed a couple of issues: > +extern void ppc_supply_vrregset (const struct regset *regset, > + struct regcache *regcache, > + int regnum, const void *vrregs, size_t len); Whitespace? Please use tabs consistently ... > +extern void ppc_collect_vrregset (const struct regset *regset, > + const struct regcache *regcache, > + int regnum, void *vrregs, size_t len); Likewise. > +ppc_altivec_support_p (struct gdbarch *gdbarch) > +{ > + struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); > + > + return (tdep->ppc_vr0_regnum >= 0 > + && tdep->ppc_vrsave_regnum >= 0 > + && (tdep->ppc_vrsave_regnum - 1) >= 0); The last check is superfluous. > +static int > +ppc_vrreg_offset (struct gdbarch_tdep *tdep, > + const struct ppc_reg_offsets *offsets, > + int regnum) > +{ > + if (regnum >= tdep->ppc_vr0_regnum > + && regnum < tdep->ppc_vr0_regnum + ppc_num_vrs) > + return offsets->vr0_offset + (regnum - tdep->ppc_vr0_regnum) * 8; Shouldn't that be * 16 instead of * 8? > + if (regnum == (tdep->ppc_vrsave_regnum - 1)) Parentheses are superfluous. > + return offsets->vscr_offset; > + > + if (regnum == tdep->ppc_vrsave_regnum) > + return offsets->vrsave_offset; > + > + return -1; > +} > + offset = ppc_vrreg_offset (tdep, offsets, regnum); > + if ((regnum != tdep->ppc_vrsave_regnum) && (regnum != (tdep->ppc_vrsave_regnum - 1))) Parentheses are superfluous as well. Also, the line is probably too long. > + ppc_supply_reg (regcache, regnum, vrregs, offset, 16); > + else if (regnum == (tdep->ppc_vrsave_regnum - 1)) > + ppc_supply_reg (regcache, regnum, > + vrregs, offsets->vscr_offset, 4); > + else > + ppc_supply_reg (regcache, regnum, > + vrregs, offsets->vrsave_offset, 4); In the latter two case, why don't you trust the offset ppc_vrreg_offset has returned? > + offset = ppc_vrreg_offset (tdep, offsets, regnum); > + if ((regnum != tdep->ppc_vrsave_regnum) && (regnum != (tdep->ppc_vrsave_regnum - 1))) > + ppc_collect_reg (regcache, regnum, vrregs, offset, 16); > + else if (regnum == (tdep->ppc_vrsave_regnum - 1)) > + ppc_collect_reg (regcache, regnum, > + vrregs, offsets->vscr_offset, 4); > + else > + ppc_collect_reg (regcache, regnum, > + vrregs, offsets->vrsave_offset, 4); Likewise. > --- gdb/corelow.c.orig > +++ gdb/corelow.c > @@ -499,6 +499,8 @@ get_core_registers (struct regcache *reg > ".reg2", 2, "floating-point", 0); > get_core_register_section (regcache, > ".reg-xfp", 3, "extended floating-point", 0); > + get_core_register_section (regcache, > + ".reg-ppc-vmx", 3, "ppc Altivec", 0); Hmmm. It would be nicer if this Altivec-specific statement didn't have to be in common code. But I don't see a simple alternative, so I won't object to it ... Are you planning on adding gcore support for Altivec register as well? Bye, Ulrich -- Dr. Ulrich Weigand GNU Toolchain for Linux on System z and Cell BE Ulrich.Weigand@de.ibm.com