Re: [Patch] Fix ABI incompatibilities on s390x

["Gerhard Tonn" <TON@de.ibm.com>]

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.