This is the mail archive of the gdb-patches@sources.redhat.com mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [Patch] Fix ABI incompatibilities on s390x


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]