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

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

Carlos Eduardo Seo wrote:

> Ok, here's my first shot at this new approach for writing core files. I 
> had to add a new BFD function (binutils patch is attached as well - I'll 
> submit this to the binutils mailing list later) and I didn't include the 
> fill_register fallbacks in the new implementation.

OK.

> I believe a similar loop could also be used in -tdep.c files to read the 
> core files as well, right?

Yes, a similar loop should be used to read core files, but this doesn't
go into -tdep.c files, but rather into corelow.c:get_core_registers.

> Only wrote the code for ppc, but this could be easily expanded to other 
> archs, adding modifications similar to those I did in ppc-linux-tdep.c.
> 
> I think this first code is very raw, but I believe we can improve it and 
> use this approach for all archs in the future.

I'd very much prefer if we could just switch over all archs at the same
time -- we already have enough partial transitions in GDB as is :-/

This should in fact be quite easy to achieve, as the only other arch
that supports any non-standard reg section today is i386 with its
.reg-xfp.  We'd only need to implement this, and a default fallback
for all other archs that only supports ".reg" and ".reg2".


>    fprintf_unfiltered (file,
> +                      "gdbarch_dump: core_regset_sections = %s\n",
> +                      (char *) gdbarch->core_regset_sections);

This doesn't work -- core_regset_sections is not a string.

> +v:struct core_regset_section *:core_regset_sections:const char *name, int len::::::(char *) gdbarch->core_regset_sections

You should either use some routine that formats the (host) pointer
properly (similar to gdb_print_host_address) or else maybe just
disable debug output for this field.

> +static struct core_regset_section ppc_regset_sections[] =
> +{
> +  {".reg2", 264},
> +  {".reg-ppc-vmx", 544},
> +  {NULL, 0}
> +};

Formatting: space after { and before }.

I'm wondering if it wouldn't be better to include ".reg" in the
table, and just treat it separately where necessary.


> +  if (core_regset_p && sect_list != NULL)
> +    while ((sect_list + i)->sect_name != NULL)
> +      {
> +	if ((regset = gdbarch_regset_from_core_section (gdbarch,
> +						     (sect_list + i)->sect_name,
> +						     (*(sect_list + i)).size))
> +	     != NULL && regset->collect_regset != NULL)
> +	  {
> +	    gdb_regset = xmalloc ((*(sect_list + i)).size);
> +	    regset->collect_regset (regset, regcache, -1,
> +				    gdb_regset, (*(sect_list + i)).size);
> +	    note_data = (char *) elfcore_write_register_note (obfd,
> +							   note_data,
> +							   note_size,
> +							   (sect_list + i)->sect_name,
> +							   gdb_regset,
> +							   (*(sect_list + i)).size);
> +	    xfree (gdb_regset);
> +	  }
> +	  i++;
> +      }

If you'd just use sect_list++, all those *(sect_list + i) 
could go away.

> +  /* 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.  */

As mentioned above, please just convert everything in one go.

> 2008-02-03  Carlos Eduardo Seo  <cseo@linux.vnet.ibm.com>
> 
> 	* elf.c (elfcore_write_register_note): New function.
> 	* elf-bfd.h (elfcore_write_register_note): New prototype.

These look fine to me, but need to be posted to the binutils list.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com