S390 GDB patch for Dignus Linux/390 compiler support

S390 GDB patch for Dignus Linux/390 compiler support

[Greg Alexander]

At the end of this email is a patch to gdb-5.1.1/gdb/s390-tdep.c.
This patch adds another condition in s390_get_frame_info() to
understand the function prologues generated by the Dignus
Systems/C and Systems/C++ 390 compilers (which can run on Linux/390)

The change is to support using S (subtract) instead of AHI for
decrementing the stack pointer on entry to a function.  We do not
use AHI because our compiler is made to work on older chips where
AHI is not available.  There is code in s390-tdep.c to handle a
similar prologue GCC will generate for extremely large stack frames,
but it is not quite identical because we store the offset in our
general constant pool (rather than generating a BRAS for a single
constant).  I also made it no longer use const_pool_state to
determine the value of good_prologue at the end of the function
because with gcc it can be 0 or 2 and with Dignus it is always 1
(we do not use an AHI to adjust it, because our constant pool works
so differently).  That covers all of the values it can have, so it's
always good so far as I'm concerned.

I'm also attaching a version of this patch built against
5.1.90_20020315 (almost the same).  The "current" tree appears to
have an identical s390-tdep.c, so it should work on that too.

Here's the ChangeLog for that patch:

2002-03-15  Greg Alexander  <greg@dignus.com>

	* s390-tdep.c: support Dignus compiler prologues



There is another issue which concerns me, though, when it comes to
S390 support.  The 5.1.1 sources did not compile for S390
straight out of the box.  There were four similar cases where I
had to add an ampersand:
proc-service.c:236 (in ps_lgetregs)
proc-service.c:253 (in ps_lsetregs)
thread-db.c:804 (in thread_db_fetch_registers)
thread-db.c:837 (in thread_db_store_registers)

These are all calls to fill_gregset() or supply_gregset():
  fill_gregset ((gdb_gregset_t *) gregset, -1);
  supply_gregset ((gdb_gregset_t *) gregset);

It turns out, tracing back all the typedefs, that this cast is wrong.
gregset is of type gdb_gregset_t, so casting it to gdb_gregset_t*
doesn't make much sense.  On the platforms I checked (except S390)
the first thing these functions do is take their argument and cast
it back to gdb_gregset_t.  So overall it has no effect on the code,
because gdb_gregset_t is, on most platforms, already a pointer type.
However, on S390, gdb_gregset_t is a structure type, and the
fill_gregset() and supply_gregset() (s390-nat.c) functions expect
a pointer to the structure.  The really easy/hacky fix is to add
something like #ifdef S390 around the affected calls, but that
seems unacceptable.  Alternatively, though it's a bit of effort to
determine which typedefs exactly need to change, we could make the
s390 gdb_gregset_t structure actually be a pointer to the structure.
But that seems wrong, too, because we still have all these calls
that are passing around the wrong type.  Ideally, someone would
decide how these functions really ought to be prototyped, and
adjust all implementations and calls to use the proper arguments.
Do you want me to do this?  To adjust them all to actually use the
types as they are defined now (i.e., add the ampersands in the
callers and derefs in all of the implementations) shouldn't take
too much time.

We can't just leave the common code alone and change the s390-nat.c
definition of the fill_gregset/supply_gregset functions because
that cast only succeeds at compile-time in the specific circumstance
where gdb_gregset_t is already a pointer (or scalar) type.

It seems that as of now, anybody compiling the S390 gdb simply
by hand adds the ampersands (this was present in one of the patches
from D.J. Barrow).  Is this really the current build methodology
for gdb, or am I missing something?

Thank you everyone for GDB!  I use it every day.
Especially thanks to D.J. Barrow at IBM for all the work on the 
Linux/390 port.

- Greg

--------patch against 5.1.1 release-------------
--- s390-tdep.c	2002/02/26 16:34:06	1.1
+++ s390-tdep.c	2002/03/15 16:47:42
@@ -204,6 +204,7 @@
   bfd_byte instr[S390_MAX_INSTR_SIZE];
   CORE_ADDR test_pc = pc, test_pc2;
   CORE_ADDR orig_sp = 0, save_reg_addr = 0, *saved_regs = NULL;
+  CORE_ADDR constant_pool_addr = 0;
   int valid_prologue, good_prologue = 0;
   int gprs_saved[S390_NUM_GPRS];
   int fprs_saved[S390_NUM_FPRS];
@@ -398,11 +399,13 @@
 	  else
 	    {
 	      /* Check for BASR gpr13,gpr0 used to load constant pool pointer to r13 in old compiler */
+	      /* Also used by Dignus compilers */
 	      if (instr[0] == 0xd && (instr[1] & 0xf) == 0
 		  && ((instr[1] >> 4) == CONST_POOL_REGIDX))
 		{
 		  const_pool_state = 1;
 		  valid_prologue = 1;
+		  constant_pool_addr = test_pc + instrlen;
 		  continue;
 		}
 	    }
@@ -500,6 +503,28 @@
 	  valid_prologue = 1;
 	  continue;
 	}
+      /* Alternatively check for a S or SG R15,offs(CONST_POOL_REGIDX)
+	 emitted by Dignus compilers.
+	 constant_pool_addr must be set upon encountering a
+	 BASR to set CONST_POOL_REGIDX. */
+      if ((save_link_state == 1) && (const_pool_state == 1)
+	  && (constant_pool_addr != 0)
+	  && ((GDB_TARGET_IS_ESAME
+	       && (instr[0] == 0xe3) && (instr[1] == 0xf0)
+	       && ((instr[2]>>4) == CONST_POOL_REGIDX)
+	       && (instr[5] == 0x09))
+	      || ((instr[0] == 0x5b) && (instr[1] == 0xf0)
+	          && ((instr[2]>>4) == CONST_POOL_REGIDX))))
+	{
+	  unsigned int offs = ((instr[2] & 0x0f) << 8) + instr[3];
+	  if (fextra_info)
+	    target_read_memory (constant_pool_addr + offs,
+				(char *) &fextra_info->stack_bought,
+				sizeof (fextra_info->stack_bought));
+	  save_link_state = 3;
+	  valid_prologue = 1;
+	  continue;
+	}
       /* check for LA gprx,offset(15) used for varargs */
       if ((instr[0] == 0x41) && ((instr[2] >> 4) == 0xf) &&
 	  ((instr[1] & 0xf) == 0))
@@ -557,7 +582,6 @@
   if (good_prologue)
     {
       good_prologue = (((got_state == 0) || (got_state == 2)) &&
-		       ((const_pool_state == 0) || (const_pool_state == 2)) &&
 		       ((save_link_state == 0) || (save_link_state == 4)) &&
 		       ((varargs_state == 0) || (varargs_state == 2)));
     }
--------END OF PATCH-------------
--------patch against 5.1.90_20020315 "branch"-------------

--- s390-tdep.c.orig	Sun Feb 24 17:31:19 2002
+++ s390-tdep.c	Fri Mar 15 16:18:35 2002
@@ -219,6 +219,7 @@
   bfd_byte instr[S390_MAX_INSTR_SIZE];
   CORE_ADDR test_pc = pc, test_pc2;
   CORE_ADDR orig_sp = 0, save_reg_addr = 0, *saved_regs = NULL;
+  CORE_ADDR constant_pool_addr = 0;
   int valid_prologue, good_prologue = 0;
   int gprs_saved[S390_NUM_GPRS];
   int fprs_saved[S390_NUM_FPRS];
@@ -482,11 +483,13 @@
 	  else
 	    {
 	      /* Check for BASR gpr13,gpr0 used to load constant pool pointer to r13 in old compiler */
+	      /* Also used by Dignus compilers */
 	      if (instr[0] == 0xd && (instr[1] & 0xf) == 0
 		  && ((instr[1] >> 4) == CONST_POOL_REGIDX))
 		{
 		  const_pool_state = 1;
 		  valid_prologue = 1;
+		  constant_pool_addr = test_pc + instrlen;
 		  continue;
 		}
 	    }
@@ -585,6 +588,28 @@
 	  valid_prologue = 1;
 	  continue;
 	}
+      /* Alternatively check for a S or SG R15,offs(CONST_POOL_REGIDX)
+	 emitted by Dignus compilers.
+	 constant_pool_addr must be set upon encountering a
+	 BASR to set CONST_POOL_REGIDX. */
+      if ((save_link_state == 1) && (const_pool_state == 1)
+	  && (constant_pool_addr != 0)
+	  && ((GDB_TARGET_IS_ESAME
+	       && (instr[0] == 0xe3) && (instr[1] == 0xf0)
+	       && ((instr[2]>>4) == CONST_POOL_REGIDX)
+	       && (instr[5] == 0x09))
+	      || ((instr[0] == 0x5b) && (instr[1] == 0xf0)
+	          && ((instr[2]>>4) == CONST_POOL_REGIDX))))
+	{
+	  unsigned int offs = ((instr[2] & 0x0f) << 8) + instr[3];
+	  if (fextra_info)
+	    target_read_memory (constant_pool_addr + offs,
+				(char *) &fextra_info->stack_bought,
+				sizeof (fextra_info->stack_bought));
+	  save_link_state = 3;
+	  valid_prologue = 1;
+	  continue;
+	}
       /* check for LA gprx,offset(15) used for varargs */
       if ((instr[0] == 0x41) && ((instr[2] >> 4) == 0xf) &&
 	  ((instr[1] & 0xf) == 0))
@@ -656,8 +681,7 @@
           instrlen = got_load_len;
         }
         
-      good_prologue = (((const_pool_state == 0) || (const_pool_state == 2)) &&
-		       ((save_link_state == 0) || (save_link_state == 4)) &&
+      good_prologue = (((save_link_state == 0) || (save_link_state == 4)) &&
 		       ((varargs_state == 0) || (varargs_state == 2)));
     }
   if (fextra_info)
--------END OF PATCH-------------

Re: S390 GDB patch for Dignus Linux/390 compiler support

[Daniel Jacobowitz]

No comment on the rest, but...

On Fri, Mar 15, 2002 at 04:11:42PM -0500, Greg Alexander wrote:
> 
> There is another issue which concerns me, though, when it comes to
> S390 support.  The 5.1.1 sources did not compile for S390
> straight out of the box.  There were four similar cases where I
> had to add an ampersand:
> proc-service.c:236 (in ps_lgetregs)
> proc-service.c:253 (in ps_lsetregs)
> thread-db.c:804 (in thread_db_fetch_registers)
> thread-db.c:837 (in thread_db_store_registers)
> 
> These are all calls to fill_gregset() or supply_gregset():
>   fill_gregset ((gdb_gregset_t *) gregset, -1);
>   supply_gregset ((gdb_gregset_t *) gregset);
> 
> It turns out, tracing back all the typedefs, that this cast is wrong.
> gregset is of type gdb_gregset_t, so casting it to gdb_gregset_t*
> doesn't make much sense.  On the platforms I checked (except S390)
> the first thing these functions do is take their argument and cast
> it back to gdb_gregset_t.  So overall it has no effect on the code,
> because gdb_gregset_t is, on most platforms, already a pointer type.
> However, on S390, gdb_gregset_t is a structure type, and the
> fill_gregset() and supply_gregset() (s390-nat.c) functions expect
> a pointer to the structure.  The really easy/hacky fix is to add

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.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

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

[Greg Alexander]

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

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

[Daniel Jacobowitz]

On Mon, Mar 18, 2002 at 10:58:16AM -0500, Greg Alexander wrote:
> 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

No.  ps_lgetregs is not a gdb-called function.  It's called from
libthread_db.so, which is provided by the system.  Its argument must be
a prgregset_t.  Therefore prgregset_t must also be a pointer or array type.

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

Remember what I said?  From an i386-linux <sys/procfs.h>:

/* And the whole bunch of them.  We could have used `struct
   user_regs_struct' directly in the typedef, but tradition says that
   the register set is an array, which does have some peculiar
   semantics, so leave it that way.  */
#define ELF_NGREG (sizeof (struct user_regs_struct) / sizeof(elf_greg_t))
typedef elf_greg_t elf_gregset_t[ELF_NGREG];


S/390 needs to do the same thing.  It's very hard to compensate for an
incorrect prgregset_t, because of some peculiarities in the type system
of C w.r.t. arrays and pointer decay.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

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

[Michael Snyder]

Greg Alexander wrote:
> 
> 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

We want them to have the same prototype on Linux as on
Solaris.  Can you persuade Sun to change their existing
interface?  ;-)

The idea was originally that the same interface could be used
to debug threads on Linux and on Solaris.  We haven't quite
been able to achieve that, but it's still a worthwhile goal.

Re: S390 GDB patch for Dignus Linux/390 compiler support

[Andrew Cagney]

Hello,

I can't find a copyright assignment (to the FSF) for yourself or a 
disclaimer from Dignus.

Assuming this is the case can you please let JimB know so that you can 
be forwarded the relevant information.

enjoy,
Andrew