This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC 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] PR/14262 (ABI corner case confusion)


On Thu, 2004-03-11 at 06:06, Ulrich Weigand wrote:
> Well, seeing as the current behaviour is self-inconsistent in that
> gcc generates code on the caller side that is incompatible with the
> code it generates on the callee side, it would appear unlikely that
> we can break targets by fixing this ...

True, but we should still be cautious.  The calling convention code is
very complicated, and it is very easy to break it without realizing it.

> As I understand it the default is 'usually pad upward, but pad short
> arguments downward on big-endian machines' (comment before DEFAULT_
> FUNCTION_ARG_PADDING in expr.h).  This means that a 12-byte structure
> would deliberately have different padding that a 4-byte one.  (And
> for 12-byte structures caller and callee would agree, b.t.w.)

It is a little more complicated than that.  FUNCTION_ARG_PADDING is used
for arguments passed in memory, but it is not consistently used for
arguments passed in registers.  It would be very unwise to change this
except as part of a major gcc revision, e.g. gcc-4, because of the risk
of silent ABI changes for some targets whose ABI is defined by gcc.

As of gcc-3.4, it will be used for args in regs if BLOCK_REG_PADDING is
defined.  This is why I mentioned BLOCK_REG_PADDING as a possible
solution.  This is a new invention, that does use FUNCTION_ARG_PADDING. 
This is deliberately a new macro, so that there was no risk of breaking
old targets.

> default.  However, if BLOCK_REG_PADDING is not defined, gcc should
> default at least to self-consistent behaviour

True.  An unanswered question here is why we get inconsistent behaviour
here.  Maybe there is something else broken.

The callee assumes that the BLKmode argument is in the low part because
locate_and_pad_parm places it there.  Looking backwards, I see that this
changed in between gcc-3.2 and gcc-3.3.

In gcc-3.2, locate_and_pad_parm does not pad BLKmode arguments, it gives
them the whole stack slot.  This matches the code in calls.c which loads
a BLKmode arg into the entire reg.  In gcc-3.3, locate_and_pad_parm has
two pad tests, neither of which check for BLKmode.

There is a corresponding change to assign_parms.  assign_parms is
supposed to write the whole register when storing a BLKmode arg to the
stack.  It calls move_block_from_reg in gcc-3.2 and earlier.  In
gcc-2.5.8 it writes the whole register.  In gcc-2.6.3 it does not. 
Lovely, this one is my fault.  It was a patch I wrote to try to fix
problems with the mips calling convention, which also passes BLKmode
args in register, and which had ugly hacks int the mips backend to try
to make this work.  This seems to be the whole cause of the mess,
because this includes a locate_and_pad_parm change which prompted the
later changes I described above which further confused the situation.

At this point, it appears that this has been broken for a long time, and
we have already had several silent ABI changes.  Hence fixing the
inconsistency should take priority over concerns about silent ABI
changes.

So based on that analysis, I would say that your original
load_register_parameters patch is OK.

> But I thought that was the whole original point of 
> store_unaligned_arguments_into_pseudos: provide a second, generally slower, 
> way to load such arguments that the standard way cannot load (because the 
> standard way makes alignment assumptions that these arguments do not match).
> You only want to go the 'slow path' if you have to -- thus the alignment 
> tests.

True.  I was making the assumption that there was a general problem that
happened to only turn up on STRICT_ALIGNMENT.  I was wrong, because I
was looking at the wrong side of this.  The problem did not originate in
calls.c.  The problem originated in function.c.  At this point,
function.c has changed so many times that it makes no sense to try to
change it back, so we need to change calls.c to match function.c.  Which
your patch does.

> If we want store_unaligned_arguments_into_pseudos to handle a second
> problem as well, we need to adjust the 'slow path' tests to take this
> into account.  (E.g. handle arguments that are either unaligned or
> BLKmode shorter than word size; something like this.)

The code uses the bitfield insert/extract function which will generate
efficient code if efficient code is possible.  I don't think this is a
problem, but it does seem to be moot now.

Except for the problem that the current code in load_register_parms can
read past the end of memory.  The code could be changed to call
mode_for_size and load the object directly without shifting if
possible.  That solves part of the problem.  Otherwise, we probably have
to use the bitfield insert/extract trick here, but at the moment we have
no proof that the code is broken, so this can probably be deferred until
later.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com


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