combine_pending_stack_adjustment_and_call checkin

[Jan Hubicka]

> I did post a message.
> 
>   http://gcc.gnu.org/ml/gcc-bugs/2000-04/msg00747.html
Thanks for pointer.. I see it is in my ss1000 inbox too, but I didn't
configured the forwarding yet. Sorry for the confusion.
> 
> I'm not sure what you are saying.  It seems to me that my code is
> correct (modulo any typos of course); it correctly handles the
> situation where:

I think you are bit missunderstanding the code (or as I believe it is intended
to work). I am basically trying to explain you my point of view and get to kind
of consensus about what fix is better (and get approval at the end if my one
shows to be the right thing). (please can you ensure that my fix does the work
for you as well?)

The code expected to calculate proper alinment taking into account the
stack_pointer_delta is in compute_argument_block_size:

      preferred_stack_boundary /= BITS_PER_UNIT;
      if (preferred_stack_boundary < 1)
        preferred_stack_boundary = 1;
      args_size->constant = (((args_size->constant
                               + stack_pointer_delta
                               + preferred_stack_boundary - 1)
                              / preferred_stack_boundary
                              * preferred_stack_boundary)
                             - stack_pointer_delta);

This says "if you push args_size->constant instead of unadjusted_args_size
you will get propertly aligned stack". 

This is true once the compute_argument_block_size finishes, but accidentally
the other arguments are precomputed, that are allowed to change
preferred_stack_boundary, that results in missalignment.
Take a look at code you've changed:

      /* If we push args individually in reverse order, perform stack alignment
         before the first push (the last arg).  */
      if (PUSH_ARGS_REVERSED && argblock == 0
          && args_size.constant != unadjusted_args_size)
        {
          /* When the stack adjustment is pending, we get better code
             by combining the adjustments.  */
          if (pending_stack_adjust
              && ! (flags & (ECF_CONST | ECF_PURE))
              && ! inhibit_defer_pop)
            combine_pending_stack_adjustment_and_call
              (unadjusted_args_size,
               &args_size,
               preferred_unit_stack_boundary);
          else if (argblock == 0)
            anti_adjust_stack (GEN_INT (args_size.constant
                                        - unadjusted_args_size));
        }
      /* Now that the stack is properly aligned, pops can't safely
         be deferred during the evaluation of the arguments.  */
      NO_DEFER_POP;

it does call your function, that repairs the missalignment (by doing exactly
same work as calculate_argument_block_size did) only in quite special case,
when some alignment is needed according to the old cauclations
(args_size.constant != unadjusted_args_size) and otherwise it just obeys the
old calculations and missalignes the stack.

I am telling this, since your comment:
The bottom line is that I don't think this code:

	  !           args_size.constant = (unadjusted_args_size
	  !                                 + ((pending_stack_adjust
	  !                                     + args_size.constant
	  !                                     - unadjusted_args_size)
	  !                                    % (preferred_stack_boundary
	  !                                       / BITS_PER_UNIT)));
	  !           adjust = (pending_stack_adjust - args_size.constant
	was quite right since it didn't have to do with stack_pointer_delta,
	which is the thing that tells us how unaligned the stack is at the
	moment.

seems to show your confusion here.  The code is not taking into account those
values, since this is already done by the compute_argument_block_size.
> 
> I'm not sure that it fixes *all* problems in this code -- but I'm
> pretty sure that the function behaves as spec'd in its comment.

purpose why I was sending the testcase is that I strongly belive that
my fix is better in the way that it fixes both testcases and not just
the special case.
> 
> Although I'm not sure it's relevant to your posting, note that calling
> `alloca' inside a function argument is not allowed by ISO C.  That
> doesn't mean the compiler should crash -- just that actually doing
> something sensible there isn't strictly necessary.  Giving an error
> message would be ideal.
Gcc does code to handle this w/o problems (after two my fixes).  If you want
warning, I can add it easilly.   I am using alloca only to force precalculation
there. There are other ways to reach this, only these are more complex.
The main trick is that the basic function require exacly 16 bytes of
argument space.  If you change your testcase to do so, I guess it will fail
too.
> 
> 
>   o There is a more general solution that subsumes the code
>     I checked in?
Thats what I believe to.
> 
>     In that case, we should remove my code, and replace it with
>     something else.

I need approval, thats why I am trying to conveince someone about corecness of
that fix.  Also while I believe that your fix is somewhat missleading, I can
definitivly be wrong.  This is weird problem and talking about it with someone
would definitivly help IMO. My mail was meant as the beggining of the
discussion basically and not "you are wrong" style of emails.

> I think any further changes to this code need to be well thought-out
> and well-commented.  I believe that to be the case for the code I
> checked in.  That is the case for the code I checked in -- even if
> it's incorrect, I spent hours thinking about it, and attempting to
> write a coherent justification in its comments.

Yes, I perfectly agree. As you have noticed, in the past number of my checkings
to this area was complette fiasco.  What I am doing for a month now is thinking
about this code again and again and trying to reorganize whole calls.c to get
it more maitainable.  It is definitivly nasty piece of code and full of traps
one can fall to.  I don't think this is actually neccesary.

In fact I am quite proud of my stack_pointer_delta code, even when it is
causing problems all the time and I am getting emails about it daily.  These
problems existed before my patches went in and I think it is great that they
shows as abort now so calls.c can get fixed gradually.  I am very surprised to
see how many such bugs actually exist.

I also am planning to do one pass trought calls.c and comment traps I've falled
to.  I think I've reached kind of insight into that code, maybe I can try to
write some comment at the beggining explaining main functionality too.

I am currently using special protocol for my calls.c patches. Basically
I bootstrap each on two platforms, do just small steps and don't checkin
more than one such patch daily to make easier to track down what broke.

Honza