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

Ulrich Weigand
2008-03-27T19:54:45Z

Carlos Eduardo Seo wrote:

> >> +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!)
> >
>
> Yes, I know. But gcc 4.x will complain about reading through a null
> pointer if I use 0.

As Andreas pointed out, (char *) '0' is definitely broken -- this
will interpret the numerical value of '0' (i.e. 48 in ASCII) as
pointer value and try to dereference it, which will certainly crash.

I had suggested to use a null pointer as this will be caught at
run-time and printed as "(null)". However, re-thinking it, this is
not actually guaranteed by the standard -- which is probably why
you're seeing a warning ...

Unfortunately, I've noticed another, more serious problem:

> Index: src/gdb/arch-utils.c
> ===================================================================
> --- src.orig/gdb/arch-utils.c
> +++ src/gdb/arch-utils.c
> @@ -31,11 +31,19 @@
> #include "gdbcore.h"
> #include "osabi.h"
> #include "target-descriptions.h"
> -
> +#include <sys/procfs.h>
> +#include "gregset.h"
> #include "version.h"
> +#include "regset.h"
>
> #include "floatformat.h"
>
> +struct core_regset_section default_regset_sections[] =
> +{
> + { ".reg", sizeof (gdb_gregset_t) },
> + { ".reg2", sizeof (gdb_fpregset_t) },
> + { NULL, 0 }
> +};

This isn't really feasible: you cannot simply include the
platform-specific <sys/procfs.h> into a generic file -- it
may not even be available on some platforms; and on other
platforms it may not provide the required gregset_t type.

I'm sorry for all the back-and-forth on this, but it seems
we do have to implement a Linux-specific fallback to that;
I don't see how we can do a generic default. To do so,
you'll need to let the gdbarch variable default to NULL,
and in linux_nat_do_thread_registers perform the fpregset
fallback if that is the case. ppc and i386 would then be
the only architectures that install the new gdbarch variable;
over time, the remaining Linux platforms should follow suite
and we can remove the fallbacks.

> Index: src/gdb/ppc-linux-tdep.c
> ===================================================================
> --- src.orig/gdb/ppc-linux-tdep.c
> +++ src/gdb/ppc-linux-tdep.c
> @@ -37,6 +37,16 @@
> #include "trad-frame.h"
> #include "frame-unwind.h"
> #include "tramp-frame.h"
> +#include <sys/procfs.h>

You cannot include a platform-specific header into a -tdep file either,
this file is also used for cross-debugging from arbitrary hosts.

> +#include "gregset.h"
> +
> +static struct core_regset_section ppc_regset_sections[] =
> +{
> + { ".reg", sizeof (gdb_gregset_t) },
> + { ".reg2", sizeof (gdb_fpregset_t) },

However, you can simply use all numerical values here -- the sizes
of the register sections in PowerPC core files are constants.

> + { ".reg-ppc-vmx", 544 },
> + { NULL, 0 }
> +};

The same applies to the i386 changes:

> +++ src/gdb/i386-linux-tdep.c
> @@ -28,12 +28,24 @@
> #include "reggroups.h"
> #include "dwarf2-frame.h"
> #include "gdb_string.h"
> -
> +#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"
> +
> +static struct core_regset_section i386_regset_sections[] =
> +{
> + { ".reg", sizeof (gdb_gregset_t) },
> + { ".reg2", sizeof (gdb_fpregset_t) },
> +#ifdef FILL_FPXREGSET
> + { ".reg-xfp", sizeof (gdb_fpxregset_t) },
> +#endif
> + { NULL, 0 }
> +};

One more issue in linux-nat.c:

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

The ++sect_list still skips the first entry unconditionally.
You should increment sect_list at the *end* of the loop.

Once again, sorry for leading you down a wrong track here.
Thanks for your continued work to fix this problem ...

Bye,
Ulrich

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