*gregset_t misuse (was Re: S390 GDB patch for Dignus Linux/390 compiler support)

[Greg Alexander <greg at dignus dot com>]

Thank you Daniel Jacobowitz, for reading and replying!
(thanks also to DJ Barrow, who sent similar advice)
I think the correct fix for my personal situation is to
find an appropriate set of kernel/glibc headers to provide
the prgregset_t that gdb expects.  But I still think there
is an issue in GDB that needs to be resolved.
Since it works most of the time for most of the people I
guess this isn't a burning issue, but since it doesn't work
for me now I think it's important :)

Read on for details ...

Daniel Jacobowitz wrote:
> This is a nonconformance in Linux for the S/390 then.  gdb_gregset_t is
> an elf_gregset_t on Linux, which -must- be an array type.  If it is a
> structure, that should be fixed - or at least you should hide it for
> GDB by defining GDB_GREGSET_T to an appropriate-size array.

The issue here is a cast from prgregset_t to gdb_gregset_t*
(proc-service.c:ps_l{get,set}regs(),
 thread-db.c:thread_db_{fetch,store}_registers() in calls to
 s390-nat.c:{fill,supply}_gregset()).  I cannot speak to what the
proper definition of elf_gregset_t is (it would seem to be a kernel
choice), I maintain that this cast is incorrect, and should be fixed.  

The trouble with the cast is that prgregset_t is the same type as
gdb_gregset_t (in most cases):
 prgregset_t on Linux/glibc is typedefed to elf_gregset_t (as is
    gregset_t) in sys/procfs.h -- if not defined by system includes,
    gdb_proc_service.h defaults it to gdb_gregset_t
 gdb_gregset_t is typedefed to GDB_GREGSET_T (gregset_t, i.e.,
    elf_gregset_t) in gregset.h
So pretty much however you slice it gdb_gregset_t and prgregset_t are
both typedefed to elf_gregset_t.  So the cast is one from (elf_gregset_t)
to (elf_gregset_t *)!  This is clearly unideal.  Either ps_lgetregs()
is getting passed an elf_gregset_t* incorrectly labelled as an
elf_gregset_t or fill_gregset() is getting passed an elf_gregset_t
labelled as an elf_gregset_t* (my suspicion is the latter)!  The function
type is wrong, and does not match the data being passed.  It seems
harmless enough but makes troubles like the one I'm trying to track
down just that much more difficult to fathom and makes just that much
more of the code dependent on an architecture-specific type (prgregset_t)
instead of the GDB-local type (gdb_gregset_t).

I'm having a hard time figuring out how exactly the ps_l{get,set}regs()
and thread_db_{fetch,store}_registers() functions get used, but my
proposal would be for them to use gdb_gregset_t instead of prgregset_t
(so that gdb_gregset_t can be defined to a pointer type if necessary --
prgregset_t is really an arch-specific type and should only be referenced
inside of arch-aware files).  I also propose that fill_gregset() and
supply_gregset() (which, in most implementations, already feature a cast
to another internal type as their first step) take gdb_gregset_t
instead of gdb_gregset_t*.  Right now with gdb_gregset_t and prgregset_t
and gdb_gregset_t* being used interchangably throughout the code, it is
impossible to write "correct" ports without violating C's type system.

It all comes down to the fact that gdb_gregset_t will probably be typedefed
to prgregset_t or prgregset_t*, but never both!

If anyone thinks I should stop discussing and start submitting patches,
please let me know and I'll consider it.  I think I've reached the end
of Dignus' business interest in the topic though. :)

Thanks,

- Greg