Re: [Patch] Fix ABI incompatibilities on s390x

[Jim Blandy <jimb at redhat dot com>]

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.