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

["Ulrich Weigand" <uweigand@de.ibm.com>]

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