This is the mail archive of the
gdb-patches@sources.redhat.com
mailing list for the GDB project.
Re: [Patch] Fix ABI incompatibilities on s390x
- From: Jim Blandy <jimb at redhat dot com>
- To: Gerhard Tonn <GerhardTonn at gammatau dot de>
- Cc: ton at de dot ibm dot com, gdb-patches at sources dot redhat dot com
- Date: 03 Feb 2003 18:55:47 -0500
- Subject: Re: [Patch] Fix ABI incompatibilities on s390x
- References: <03020212430000.00722@tau>
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