Gerhard Tonn <ton@xxxxxxxxxx> 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@xxxxxxxxxx>
>
> * 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.
|
