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

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

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