From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2123 invoked by alias); 13 May 2008 21:40:17 -0000 Received: (qmail 2108 invoked by uid 22791); 13 May 2008 21:40:16 -0000 X-Spam-Check-By: sourceware.org Received: from mtagate8.de.ibm.com (HELO mtagate8.de.ibm.com) (195.212.29.157) by sourceware.org (qpsmtpd/0.31) with ESMTP; Tue, 13 May 2008 21:39:52 +0000 Received: from d12nrmr1607.megacenter.de.ibm.com (d12nrmr1607.megacenter.de.ibm.com [9.149.167.49]) by mtagate8.de.ibm.com (8.13.8/8.13.8) with ESMTP id m4DLdD4A537074 for ; Tue, 13 May 2008 21:39:13 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.7) with ESMTP id m4DLdDVk4116480 for ; Tue, 13 May 2008 23:39:13 +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 m4DLdCnd032652 for ; Tue, 13 May 2008 23:39:13 +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 m4DLdCUJ032649; Tue, 13 May 2008 23:39:12 +0200 Message-Id: <200805132139.m4DLdCUJ032649@d12av02.megacenter.de.ibm.com> Received: by tuxmaker.boeblingen.de.ibm.com (sSMTP sendmail emulation); Tue, 13 May 2008 23:39:12 +0200 Subject: Re: [RFC] Add support for PPC Altivec registers in gcore To: cseo@linux.vnet.ibm.com (Carlos Eduardo Seo) Date: Wed, 14 May 2008 04:22:00 -0000 From: "Ulrich Weigand" Cc: gdb-patches@sourceware.org (GDB Patches Mailing List) In-Reply-To: <4824B100.30509@linux.vnet.ibm.com> from "Carlos Eduardo Seo" at May 09, 2008 05:16:00 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: 2008-05/txt/msg00418.txt.bz2 Carlos Eduaro Seo wrote: > I also separated the ppc and x86 implementations from the main gcore > stuff, to make it easier to analyze. > > Is this close to what you had in mind? Thanks, this is looking very good. I've only a couple of minor issues: > +# Supported register notes in a core file Please add a '.' at the end. > +/* Data structure for the supported register notes in a core file */ Likewise. > + /* The loop below uses the new struct core_regset_section, which stores > + the supported section names and sizes for the core file. Note that > + note PRSTATUS needs to be treated specially. But the other notes are > + structurally the same, so they can benefit from the new struct. */ Always use two spaces after '.'. > + /* .reg was already handled above */ '.' at the end. > + if ((regset = gdbarch_regset_from_core_section (gdbarch, > + sect_list->sect_name, > + sect_list->size)) > + != NULL && regset->collect_regset != NULL) I think we should assert that regset and regset->collect_regset are non-NULL here. If a target provides a core_regset_sections list, it simply needs to ensure that its regset_from_core_section will recognize all sections in the list. > + /* For architectures that does not have the struct core_regset_section implemented, > + we use the old method. When all the architectures have the new support, the code > + below should be deprecated. The FILL_FPXREGSET fallback was removed since i386 > + has the new method implemented. */ Again two spaces after '.'. Please make sure that the line length does not exceed 80 characters. Also, I think the sentence about FILL_FPXREGSET is superfluous -- that's already been addressed, no need to keep a reminder to it in the code. Finally, "... the code below should be deprecated" ... once all architectures have the new support, the code below should be *deleted*. It is already deprecated (i.e. new code should not be using it any longer) today. > @@ -1075,7 +1075,6 @@ gdb_print_host_address (const void *addr > > fprintf_filtered (stream, "0x%lx", (unsigned long) addr); > } > - This whitespace change probably was unintentional? > +const char * > +host_address_to_string (const void *addr) > +{ > + char *str = get_cell (); > + sprintf (str, "0x%lx", (unsigned long) addr); > + return str; > +} Please move gdb_print_host_address and host_address_to_string next to each other, so that it becomes clearer that the comment in gdb_print_host_address also applies to host_address_to_string. > @@ -35,6 +35,18 @@ > #include "solib-svr4.h" > #include "symtab.h" > #include "arch-utils.h" > +#include "regset.h" Please update the Makefile.in dependency list as well. > +/* Supported register note sections */ '.' at the end. > +static struct core_regset_section i386_regset_sections[] = As this is Linux-specific, maybe i386_linux_regset_section ? > + { ".reg", 144 }, > + { ".reg2", 108 }, > +#ifdef FILL_FPXREGSET > + { ".reg-xfp", 512 }, > +#endif > + { NULL, 0 } Do we actually need the #ifdef any more? There shouldn't be any harm in unconditionally including those ... > + /* Install supported register note sections */ '.' at the end. > Index: src/gdb/ppc-linux-tdep.c > =================================================================== > --- src.orig/gdb/ppc-linux-tdep.c > +++ src/gdb/ppc-linux-tdep.c > @@ -45,6 +45,14 @@ > #include "features/rs6000/powerpc-altivec64l.c" > #include "features/rs6000/powerpc-e500l.c" > > +static struct core_regset_section ppc_regset_sections[] = Again ppc_linux_ ... as long as it is Linux-specific. With those minor modifications, I think this patch should be OK (assuming testing passes, of course). Thanks, Ulrich -- Dr. Ulrich Weigand GNU Toolchain for Linux on System z and Cell BE Ulrich.Weigand@de.ibm.com