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

Ulrich Weigand
2008-03-17T19:07:18Z

Carlos Eduardo Seo wrote:

> +# Supported register notes in core files
'.' at the end.

> +v:struct core_regset_section *:core_regset_sections:const char *name, int len::::default_regset_sections::(char *) '0'

Not '0' -- just 0. (That *is* a difference!)

> @@ -2653,35 +2657,31 @@ linux_nat_do_thread_registers (bfd *obfd
> lwp,
> stop_signal, &gregs);
>
> - if (core_regset_p
> - && (regset = gdbarch_regset_from_core_section (gdbarch, ".reg2",
> - sizeof (fpregs))) != NULL
> - && regset->collect_regset != NULL)
> - regset->collect_regset (regset, regcache, -1,
> - &fpregs, sizeof (fpregs));
> - else
> - fill_fpregset (regcache, &fpregs, -1);
> -
> - note_data = (char *) elfcore_write_prfpreg (obfd,
> - note_data,
> - note_size,
> - &fpregs, sizeof (fpregs));

Ah. I think I asked you to do this, but I overlooked something
here: there are quite a number of Linux targets that either do
not define gdbarch_regset_from_core_section or do not implement
a collect_regset function.

We don't want to break those, so we'll probably have to keep
the fall-back to fill_fpregset for now.

> -#ifdef FILL_FPXREGSET
> - if (core_regset_p
> - && (regset = gdbarch_regset_from_core_section (gdbarch, ".reg-xfp",
> - sizeof (fpxregs))) != NULL
> - && regset->collect_regset != NULL)
> - regset->collect_regset (regset, regcache, -1,
> - &fpxregs, sizeof (fpxregs));
> - else
> - fill_fpxregset (regcache, &fpxregs, -1);
> -
> - note_data = (char *) elfcore_write_prxfpreg (obfd,
> - note_data,
> - note_size,
> - &fpxregs, sizeof (fpxregs));
> -#endif

But *this* definitely can go away, as the i386 target does
use gdbarch_regset_from_core_section with collect_regset.

> + /* 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. */
> +
> + if (core_regset_p && sect_list != NULL)

sect_list cannot really be NULL as it has a default.

> + while ((++sect_list)->sect_name != NULL)

This implicitly assumes that ".reg" must be the very first element.
I'd rather see an explicit check of the type

/* ".reg" was already handled above. */
if (strcmp (sect_list->sect_name, ".reg") == 0)
continue;

(And similarly for ".reg2" if we keep the special case above.)

> + if ((regset = gdbarch_regset_from_core_section (gdbarch,
> + sect_list->sect_name,
> + (*sect_list).size))

I'd rather use sect_list->size (here and elsewhere).

> +#include <sys/procfs.h>
> +#include "gregset.h"
> #include "i386-tdep.h"
> #include "i386-linux-tdep.h"
> #include "glibc-tdep.h"
> #include "solib-svr4.h"
> #include "symtab.h"
> +#include "regset.h"

These need Makefile.in dependency list changes.

> +#include <sys/procfs.h>
> +#include "gregset.h"
> #include "version.h"
> +#include "regset.h"

Likewise.

> Index: src/gdb/arch-utils.h

> +#include "regset.h"
> struct gdbarch;
> struct frame_info;
> struct minimal_symbol;
> struct type;
> struct gdbarch_info;
>
> +extern struct core_regset_section default_regset_sections[];

I'd prefer *not* to include a new header file here; a forward
declaration of the struct should be enough:
struct core_regset_section;

Bye,
Ulrich

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