This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC] Add support for PPC Altivec registers in gcore
- From: "Ulrich Weigand" <uweigand at de dot ibm dot com>
- To: cseo at linux dot vnet dot ibm dot com (Carlos Eduardo Seo)
- Cc: gdb-patches at sourceware dot org (GDB Patches Mailing List)
- Date: Tue, 13 May 2008 23:39:12 +0200 (CEST)
- Subject: Re: [RFC] Add support for PPC Altivec registers in gcore
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