Re: [Patch] Fix ABI incompatibilities on s390x

[Gerhard Tonn <GerhardTonn@gammatau.de>]

> I'm afraid I have the same concerns about this patch as I did about
> the first one.

> It seems that there are two main differences between the s390 and
> s390x ABI's:
> - the s390 registers are larger.
> - the s390x does not have a special DOUBLE_ARG class of arguments that
>   it handles specially.

> Most of the first sort of difference you can account for using
> REGISTER_SIZE, but there is one case that cannot be handled this way.
> You handle this by testing GDB_TARGET_IS_ESAME.

> Since there are several bits of code that know about the existence of
> DOUBLE_ARGs (i.e., we have to exclude DOUBLE_ARGs from the other
> classes of arguments), you handle each of those by testing
> GDB_TARGET_IS_ESAME.

> Please handle the first case by defining a function:

> static int
> is_power_of_two (unsigned int n)
> {
>   return ((n & (n-1)) == 0);
> }

> and then saying ! (is_power_of_two (length) && length <= REGISTER_SIZE) 
> in pass_by_copy_ref.

> Please handle the second case by explicitly calling is_double_arg from
> is_simple_arg, and then change is_double_arg as follows:

> static int
> is_double_arg (struct type *type)
> {
>   unsigned length = TYPE_LENGTH (type);
> 
>   /* The s390x ABI doesn't handle DOUBLE_ARGS specially.  */
>   if (GDB_TARGET_IS_ESAME)
>     return 0;
> 
>   return ((is_integer_like (type)
>            || is_struct_like (type))
>           && length == 8);
> }

Good idea. Thanks for your time and your help.

BTW., I have run the test suite after each of my changes. The results look 
pretty good.

Gerhard


2003-02-02  Gerhard Tonn  <ton@de.ibm.com>

	* s390-tdep.c (is_simple_arg): Substitute hardcoded register size by
	  macro REGISTER_SIZE and check for double args. Long double checks
	  are dropped, since they are not supported by s390 and s390x.
	(is_power_of_two): New.
	(pass_by_copy_ref): Call is_power_of_two() instead of explicit checks of 	 	
	  length. Long double checks are dropped, since they are not supported
	  by s390 and s390x.
	(is_double_arg): Return 0 for s390x architecture.
	(s390_push_arguments): Substitute hardcoded register size by macro
	  REGISTER_SIZE. Consider ABI when returning values of type struct.
	  Substitute hardcoded number of floating point registers by macro
	  S390_NUM_FP_PARAMETER_REGISTERS. Substitute hardcoded
	  stack parameter alignment value by macro
	  S390_STACK_PARAMETER_ALIGNMENT. Substitute hardcoded stack frame
	  overhead value by macro S390_STACK_FRAME_OVERHEAD.

--- s390-tdep.c.bak	2003-02-01 19:13:31.000000000 +0000
+++ s390-tdep.c	2003-02-02 08:40:40.000000000 +0000
@@ -94,7 +94,9 @@
 #define S390X_SIGREGS_FP0_OFFSET      (216)
 #define S390_UC_MCONTEXT_OFFSET    (256)
 #define S390X_UC_MCONTEXT_OFFSET   (344)
-#define S390_STACK_FRAME_OVERHEAD  (GDB_TARGET_IS_ESAME ? 160:96)
+#define S390_STACK_FRAME_OVERHEAD  16*REGISTER_SIZE+32
+#define S390_STACK_PARAMETER_ALIGNMENT  REGISTER_SIZE
+#define S390_NUM_FP_PARAMETER_REGISTERS (GDB_TARGET_IS_ESAME ? 4:2)
 #define S390_SIGNAL_FRAMESIZE  (GDB_TARGET_IS_ESAME ? 160:96)
 #define s390_NR_sigreturn          119
 #define s390_NR_rt_sigreturn       173
@@ -1369,13 +1371,18 @@
 
   /* This is almost a direct translation of the ABI's language, except
      that we have to exclude 8-byte structs; those are DOUBLE_ARGs.  */
-  return ((is_integer_like (type) && length <= 4)
+  return ((is_integer_like (type) && length <= REGISTER_SIZE)
           || is_pointer_like (type)
-          || (is_struct_like (type) && length != 8)
-          || (is_float_like (type) && length == 16));
+          || (is_struct_like (type) && !is_double_arg (type)));
 }
 
 
+static int
+is_power_of_two (unsigned int n)
+{
+  return ((n & (n - 1)) == 0);
+}
+
 /* Return non-zero if TYPE should be passed as a pointer to a copy,
    zero otherwise.  TYPE must be a SIMPLE_ARG, as recognized by
    `is_simple_arg'.  */
@@ -1384,8 +1391,8 @@
 {
   unsigned length = TYPE_LENGTH (type);
 
-  return ((is_struct_like (type) && length != 1 && length != 2 && length != 
4)
-          || (is_float_like (type) && length == 16));
+  return (is_struct_like (type)
+          && !(is_power_of_two (length) && length <= REGISTER_SIZE));
 }
 
 
@@ -1417,6 +1424,10 @@
 {
   unsigned length = TYPE_LENGTH (type);
 
+  /* The s390x ABI doesn't handle DOUBLE_ARGS specially.  */
+  if (GDB_TARGET_IS_ESAME)
+    return 0;
+
   return ((is_integer_like (type)
            || is_struct_like (type))
           && length == 8);
@@ -1542,9 +1553,9 @@
         
         sp = round_down (sp, alignment_of (type));
 
-        /* SIMPLE_ARG values get extended to 32 bits.  Assume every
-           argument is.  */
-        if (length < 4) length = 4;
+        /* SIMPLE_ARG values get extended to REGISTER_SIZE bytes. 
+           Assume every argument is.  */
+        if (length < REGISTER_SIZE) length = REGISTER_SIZE;
         sp -= length;
       }
   }
@@ -1565,13 +1576,17 @@
     int gr = 2;
     CORE_ADDR starg = sp;
 
+    /* A struct is returned using general register 2 */
+    if (struct_return)
+      gr++;
+
     for (i = 0; i < nargs; i++)
       {
         struct value *arg = args[i];
         struct type *type = VALUE_TYPE (arg);
         
         if (is_double_or_float (type)
-            && fr <= 2)
+            && fr <= S390_NUM_FP_PARAMETER_REGISTERS * 2 - 2)
           {
             /* When we store a single-precision value in an FP register,
                it occupies the leftmost bits.  */
@@ -1598,7 +1613,7 @@
             write_register_gen (S390_GP0_REGNUM + gr,
                                 VALUE_CONTENTS (arg));
             write_register_gen (S390_GP0_REGNUM + gr + 1,
-                                VALUE_CONTENTS (arg) + 4);
+                                VALUE_CONTENTS (arg) + REGISTER_SIZE);
             gr += 2;
           }
         else
@@ -1614,9 +1629,9 @@
 
             if (is_simple_arg (type))
               {
-                /* Simple args are always either extended to 32 bits,
-                   or pointers.  */
-                starg = round_up (starg, 4);
+                /* Simple args are always extended to 
+                   REGISTER_SIZE bytes.  */
+                starg = round_up (starg, REGISTER_SIZE);
 
                 /* Do we need to pass a pointer to our copy of this
                    argument?  */
@@ -1624,18 +1639,19 @@
                   write_memory_signed_integer (starg, pointer_size,
                                                copy_addr[i]);
                 else
-                  /* Simple args are always extended to 32 bits.  */
-                  write_memory_signed_integer (starg, 4,
+                  /* Simple args are always extended to 
+                     REGISTER_SIZE bytes. */
+                  write_memory_signed_integer (starg, REGISTER_SIZE,
                                                extend_simple_arg (arg));
-                starg += 4;
+                starg += REGISTER_SIZE;
               }
             else
               {
                 /* You'd think we should say:
                    starg = round_up (starg, alignment_of (type));
                    Unfortunately, GCC seems to simply align the stack on
-                   a four-byte boundary, even when passing doubles.  */
-                starg = round_up (starg, 4);
+                   a four/eight-byte boundary, even when passing doubles. */
+                starg = round_up (starg, S390_STACK_PARAMETER_ALIGNMENT);
                 write_memory (starg, VALUE_CONTENTS (arg), length);
                 starg += length;
               }
@@ -1646,7 +1662,7 @@
   /* Allocate the standard frame areas: the register save area, the
      word reserved for the compiler (which seems kind of meaningless),
      and the back chain pointer.  */
-  sp -= 96;
+  sp -= S390_STACK_FRAME_OVERHEAD;
 
   /* Write the back chain pointer into the first word of the stack
      frame.  This will help us get backtraces from within functions