Re: [Patch] Fix ABI incompatibilities on s390x

Re: [Patch] Fix ABI incompatibilities on s390x

[Gerhard Tonn]

"Gerhard Tonn" <TON@de.ibm.com> writes:
>> I don't think that it is worthwhile to duplicate the functions. The only
>> difference between s390 and s390x is really the REGISTER_SIZE. I will
>> rework the patch so that this becomes more clear. I will also address the
>> other issues.

>Certainly, if two functions can be made identical by the appropriate
>use of REGISTER_SIZE, then there's no reason to duplicate them.
>However, I thought I had seen another sort of conditionality which is
>definitely of the sort we are trying to avoid.

>I'll watch for your new patches.  Thanks again for your work.

Please find below the revised patch. The number of floating point registers is
another difference between s390 and s390x besides the register size.

As soon as you have approved the patch  we will start the copyright assignment . I
will let you know any progress.

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

	* s390-tdep.c (is_simple_arg): Substitute hardcoded register size by
	  macro REGISTER_SIZE and consider s390x register size for structs.
	(pass_by_copy_ref): Likewise.
	(is_double_arg): Likewise.
	(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	Tue Nov 26 11:49:54 2002
+++ s390-tdep.c	Tue Nov 26 12:17:05 2002
@@ -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,9 +1371,9 @@
 
   /* 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_struct_like (type) && (GDB_TARGET_IS_ESAME || length != 8))
           || (is_float_like (type) && length == 16));
 }
 
@@ -1384,7 +1386,8 @@
 {
   unsigned length = TYPE_LENGTH (type);
 
-  return ((is_struct_like (type) && length != 1 && length != 2 && length != 4)
+  return ((is_struct_like (type) && length != 1 && length != 2 && length != 4
+           && (!GDB_TARGET_IS_ESAME || length != 8))
           || (is_float_like (type) && length == 16));
 }
 
@@ -1419,7 +1422,7 @@
 
   return ((is_integer_like (type)
            || is_struct_like (type))
-          && length == 8);
+          && !GDB_TARGET_IS_ESAME && length == 8);
 }
 
 
@@ -1542,9 +1545,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 +1568,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 +1605,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 +1621,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 +1631,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 +1654,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



--
Regards,
Gerhard

Re: [Patch] Fix ABI incompatibilities on s390x

[Andrew Cagney]

[-- Attachment #1: Type: text/plain, Size: 71 bytes --]

Kevin,
Since you're playing with an s390, can you look at this?
Andrew

[-- Attachment #2: mailbox-message://ac131313@movemail/fsf/gdb/patches#25697139 --]
[-- Type: message/rfc822, Size: 9545 bytes --]

From: Gerhard Tonn <ton@de.ibm.com>
To: gdb-patches@sources.redhat.com
Subject: Re: [Patch] Fix ABI incompatibilities on s390x
Date: Thu, 23 Jan 2003 11:59:40 +0100
Message-ID: <03012312571000.14253@fmtc804>


"Gerhard Tonn" <TON@de.ibm.com> writes:
>> I don't think that it is worthwhile to duplicate the functions. The only
>> difference between s390 and s390x is really the REGISTER_SIZE. I will
>> rework the patch so that this becomes more clear. I will also address the
>> other issues.

>Certainly, if two functions can be made identical by the appropriate
>use of REGISTER_SIZE, then there's no reason to duplicate them.
>However, I thought I had seen another sort of conditionality which is
>definitely of the sort we are trying to avoid.

>I'll watch for your new patches.  Thanks again for your work.

Please find below the revised patch. The number of floating point registers is
another difference between s390 and s390x besides the register size.

As soon as you have approved the patch  we will start the copyright assignment . I
will let you know any progress.

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

	* s390-tdep.c (is_simple_arg): Substitute hardcoded register size by
	  macro REGISTER_SIZE and consider s390x register size for structs.
	(pass_by_copy_ref): Likewise.
	(is_double_arg): Likewise.
	(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	Tue Nov 26 11:49:54 2002
+++ s390-tdep.c	Tue Nov 26 12:17:05 2002
@@ -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,9 +1371,9 @@
 
   /* 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_struct_like (type) && (GDB_TARGET_IS_ESAME || length != 8))
           || (is_float_like (type) && length == 16));
 }
 
@@ -1384,7 +1386,8 @@
 {
   unsigned length = TYPE_LENGTH (type);
 
-  return ((is_struct_like (type) && length != 1 && length != 2 && length != 4)
+  return ((is_struct_like (type) && length != 1 && length != 2 && length != 4
+           && (!GDB_TARGET_IS_ESAME || length != 8))
           || (is_float_like (type) && length == 16));
 }
 
@@ -1419,7 +1422,7 @@
 
   return ((is_integer_like (type)
            || is_struct_like (type))
-          && length == 8);
+          && !GDB_TARGET_IS_ESAME && length == 8);
 }
 
 
@@ -1542,9 +1545,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 +1568,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 +1605,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 +1621,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 +1631,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 +1654,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



--
Regards,
Gerhard

Re: [Patch] Fix ABI incompatibilities on s390x

[Jim Blandy]

Gerhard Tonn <ton@de.ibm.com> writes:
> "Gerhard Tonn" <TON@de.ibm.com> writes:
> >> I don't think that it is worthwhile to duplicate the functions. The only
> >> difference between s390 and s390x is really the REGISTER_SIZE. I will
> >> rework the patch so that this becomes more clear. I will also address the
> >> other issues.
> 
> >Certainly, if two functions can be made identical by the appropriate
> >use of REGISTER_SIZE, then there's no reason to duplicate them.
> >However, I thought I had seen another sort of conditionality which is
> >definitely of the sort we are trying to avoid.
> 
> >I'll watch for your new patches.  Thanks again for your work.
> 
> Please find below the revised patch. The number of floating point registers is
> another difference between s390 and s390x besides the register size.
> 
> As soon as you have approved the patch  we will start the copyright assignment . I
> will let you know any progress.

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);
}

I'll be happy to review the patch again once these changes have been
made.

A while back, I spent several weeks fixing bugs in the parameter
passing code of the s390 GDB port that had just been contributed ---
bugs that could have been found and fixed by anyone willing to run the
test suite.  But this is not unusual; of the half-dozen or so gdb
ports I've been asked to fix over the years, the parameter passing
code is the part that is most often broken.  Those sorts of problems
usually occupy the better part of the time required to bring the test
suite into good shape for a given architecture.  I think other GDB
developers with exposure to a broad range of architectures can cite
similar experiences.

So I think it is very important that we not waste future developers'
time by allowing the same sorts of coding practices that have led to
such poor reliability in the past.  As I said in my first review of
the patch, one of the hallmarks of parameter-passing code that will
not survive the test of time is code that is shared between several
ABI variants.  Each variant has its quirks (i.e., the special
handling, or lack thereof, for DOUBLE_ARG arguments), and its bugs
(which are not always carried from one variant to another, as the
double and float singleton bug was on the s390 family), and in the
end, being able to cope with each ABI as a separate entity is the
better approach.

What I'm suggesting above is a compromise, that at least localizes the
architecture tests to a single place, which is clearly related to the
important distinction between the ABI's.

Re: [Patch] Fix ABI incompatibilities on s390x

[Gerhard Tonn]

> 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

Re: [Patch] Fix ABI incompatibilities on s390x

[Jim Blandy]

Okay, this looks good.  You're testing on both the s390 and the s390x,
right?  Assuming you are, then I think we're ready to start the legal
machinery.

I don't know if this is a problem, but this patch doesn't apply to
today's sources: the write_register_gen calls need to be changed to
deprecated_write_register_gen calls, given Andrew's change from
November:

2002-11-02  Andrew Cagney  <cagney@redhat.com>

	* regcache.h (deprecated_read_register_gen): Rename
	read_register_gen.
	(deprecated_write_register_gen): Rename write_register_gen.
	* i387-tdep.c: Update.
	* x86-64-linux-nat.c: Update
	* wince.c: Update.
	* thread-db.c: Update.
	* win32-nat.c: Update.
	* mips-tdep.c: Update.
	* d10v-tdep.c: Update.
	* cris-tdep.c: Update.
	* remote-sim.c: Update.
	* remote-rdi.c: Update.
	* remote-rdp.c: Update.
	* frame.c: Update.
	* target.c: Update.
	* blockframe.c: Update.
	* x86-64-tdep.c: Update.
	* xstormy16-tdep.c: Update.
	* sh-tdep.c: Update.
	* s390-tdep.c: Update.
	* rs6000-tdep.c: Update.
	* sparc-tdep.c: Update.
	* i386-tdep.c: Update.
	* dwarf2cfi.c: Update.
	* regcache.c: Update.

If you're working with pre-November sources, you may want to update
and re-test.

Gerhard Tonn <GerhardTonn@gammatau.de> writes:

> > 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

Re: [Patch] Fix ABI incompatibilities on s390x

[Gerhard Tonn]

On Tuesday 04 February 2003 00:55, you wrote:
> Okay, this looks good.  You're testing on both the s390 and the s390x,
> right?  Assuming you are, then I think we're ready to start the legal
> machinery.
>
> I don't know if this is a problem, but this patch doesn't apply to
> today's sources: the write_register_gen calls need to be changed to
> deprecated_write_register_gen calls, given Andrew's change from
> November:
>

I have tested it for both architectures on gdb 5.3. I will adapt it to head, 
start the copyright assignment and then repost it here.

Thanks,
Gerhard

Re: [Patch] Fix ABI incompatibilities on s390x

[Gerhard Tonn]

Hi,
I don't think that it is worthwhile to duplicate the functions. The only
difference between s390 and s390x is really the REGISTER_SIZE. I will
rework the patch so that this becomes more clear. I will also address the
other issues.

Regards / Mit freundlichen Gruessen
Gerhard

Gerhard Tonn, Linux for eServer Development, +(49)-7031-16-4716, Lotus
Notes: ton@ibmde,
   Internet: ton@de.ibm.com


Jim Blandy <jimb@redhat.com> on 21.11.2002 06:16:41

To:    Gerhard Tonn/Germany/IBM@IBMDE
cc:    gdb-patches@sources.redhat.com
Subject:    Re: [Patch] Fix ABI incompatibilities on s390x




Gerhard Tonn <ton@de.ibm.com> writes:
> attached is a patch that fixes some ABI incompatibilities for the s390x
> architecture.

Hi, thanks for working on this!

First of all, this patch incudes a number of different fixes, all
mixed together:
- struct return fixes
- push_arguments fixes
- get_frame_info fixes

These each need to be submitted as separate patches.  (I understand
that IBM has a special agreement regarding how copyright assignment is
handled; perhaps the patches could be reviewed separately, but then
assigned as a single patch.)

> 2002-11-19  Gerhard Tonn  <ton@de.ibm.com>
>
>     * s390-tdep.c (s390_push_arguments, s390_get_frame_info):
>        Fix the s390x ELF ABI implementation bugs.

If our Changelog entries all said, "Fixed bugs in this code", they
wouldn't be very useful.  :)

We have had very bad experiences with trying to make a single function
serve two different ABI's in the past.  (mips_push_arguments seems to
have been cleaned up since I last looked; it was a real mess.)  So
while using things like 'REGISTER_SIZE' and
'S390_STACK_PARAMETER_ALIGNMENT' are clearly a good idea, for the sake
of the other stuff I'd like to see a separate 's390x_push_arguments'
function written that does things right for the s390x's ABI.  The
helper functions like `is_simple_arg' should be duplicated, rather
than testing GDB_TARGET_IS_ESAME.

I understand this may seem pedantic --- after all, it's just a few
minor differences, why duplicate all that code? --- but I think
history will back me up.

Some other comments:

> + || (is_struct_like (type) && (GDB_TARGET_IS_ESAME ? 1 : length != 8))

That idiom shows up a lot in this patch --- wouldn't it be more
legible to write:

    || (is_struct_like (type) && (GDB_TARGET_IS_ESAME || length != 8))

?  Of course, a lot of these will go away entirely when the s390/s390x
functions are split.

[Patch] Fix ABI incompatibilities on s390x

[Gerhard Tonn]

Hi,
attached is a patch that fixes some ABI incompatibilities for the s390x
architecture.

2002-11-19  Gerhard Tonn  <ton@de.ibm.com>

	* s390-tdep.c (s390_push_arguments, s390_get_frame_info):              
	   Fix the s390x ELF ABI implementation bugs.

--- gdb-5.2.cvs20020401/gdb/s390-tdep.c	Mon Apr  1 03:09:43 2002
+++ gdb-5.2.cvs20020401-new/gdb/s390-tdep.c	Fri Jul 26 11:53:57 2002
@@ -95,6 +95,8 @@
 #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_PARAMETER_ALIGNMENT  (GDB_TARGET_IS_ESAME ? 8:4)
+#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
@@ -1378,9 +1380,9 @@
 
   /* 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_struct_like (type) && (GDB_TARGET_IS_ESAME ? 1 : length != 8))
           || (is_float_like (type) && length == 16));
 }
 
@@ -1393,7 +1395,8 @@
 {
   unsigned length = TYPE_LENGTH (type);
 
-  return ((is_struct_like (type) && length != 1 && length != 2 && length != 4)
+  return ((is_struct_like (type) && length != 1 && length != 2 && length != 4
+           && (GDB_TARGET_IS_ESAME ? length != 8 : 1))
           || (is_float_like (type) && length == 16));
 }
 
@@ -1551,9 +1554,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;
       }
   }
@@ -1574,13 +1577,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.  */
@@ -1601,13 +1608,13 @@
 
             gr++;
           }
-        else if (is_double_arg (type)
+        else if (!GDB_TARGET_IS_ESAME && is_double_arg (type)
                  && gr <= 5)
           {
             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
@@ -1618,14 +1625,14 @@
             
             /* If we skipped r6 because we couldn't fit a DOUBLE_ARG
                in it, then don't go back and use it again later.  */
-            if (is_double_arg (type) && gr == 6)
+            if (!GDB_TARGET_IS_ESAME && is_double_arg (type) && gr == 6)
               gr = 7;
 
             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?  */
@@ -1633,18 +1640,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;
               }
@@ -1655,7 +1663,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


--
Regards,
Gerhard

Re: [Patch] Fix ABI incompatibilities on s390x

[Jim Blandy]

Gerhard Tonn <ton@de.ibm.com> writes:
> attached is a patch that fixes some ABI incompatibilities for the s390x
> architecture.

Hi, thanks for working on this!

First of all, this patch incudes a number of different fixes, all
mixed together:
- struct return fixes
- push_arguments fixes
- get_frame_info fixes

These each need to be submitted as separate patches.  (I understand
that IBM has a special agreement regarding how copyright assignment is
handled; perhaps the patches could be reviewed separately, but then
assigned as a single patch.)

> 2002-11-19  Gerhard Tonn  <ton@de.ibm.com>
> 
> 	* s390-tdep.c (s390_push_arguments, s390_get_frame_info):              
> 	   Fix the s390x ELF ABI implementation bugs.

If our Changelog entries all said, "Fixed bugs in this code", they
wouldn't be very useful.  :)

We have had very bad experiences with trying to make a single function
serve two different ABI's in the past.  (mips_push_arguments seems to
have been cleaned up since I last looked; it was a real mess.)  So
while using things like 'REGISTER_SIZE' and
'S390_STACK_PARAMETER_ALIGNMENT' are clearly a good idea, for the sake
of the other stuff I'd like to see a separate 's390x_push_arguments'
function written that does things right for the s390x's ABI.  The
helper functions like `is_simple_arg' should be duplicated, rather
than testing GDB_TARGET_IS_ESAME.

I understand this may seem pedantic --- after all, it's just a few
minor differences, why duplicate all that code? --- but I think
history will back me up.

Some other comments:

> + || (is_struct_like (type) && (GDB_TARGET_IS_ESAME ? 1 : length != 8))

That idiom shows up a lot in this patch --- wouldn't it be more
legible to write:

    || (is_struct_like (type) && (GDB_TARGET_IS_ESAME || length != 8))

?  Of course, a lot of these will go away entirely when the s390/s390x
functions are split.

Re: [Patch] Fix ABI incompatibilities on s390x

[Andrew Cagney]

> (I understand
> that IBM has a special agreement regarding how copyright assignment is
> handled; perhaps the patches could be reviewed separately, but then
> assigned as a single patch.)

Yes, that is correct, the patch-set will need a digital assignment. 
Gerhard, if you need details let me know.

Transfering as a single final patch at the end is a good idea.

(don't forget the 5.3 branch once this has been cleared.)

Andrew