Re: [RFC] Add support for PPC Altivec registers in gcore

Ulrich Weigand
2008-05-13T21:40:17Z

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);
> }
> -\f

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