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

Ulrich Weigand
2008-02-18T18:42:56Z

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