[calls.c] PR rtl-optimization/67226: Take into account pretend_args_size when checking stack offsets for sibcall optimisation

Hi all,

This is a wrong-code PR similar to PR 65358. It occurs on targets/ABIs that can pass struct parameter
partially in registers and partially on the stack.  In this instance the target is arm.
In this bug we hit a problem during sibcall
optimisation at call expansion time where we have a partial argument to a function being passed down
to another function partially and so the incoming and outgoing arguments pointer are the same.
We miscompile the code if the incoming and outgoing stack slots overlap but are not identical.
In this particular testcase for the call to store_one_arg we want to store the parameter in
arg->value to the stack frame at argblock.
arg->value is:
(mem/c:BLK (plus:SI (reg/f:SI 104 virtual-incoming-args)
         (const_int 20 [0x14])) [1 from+0 S20 A32])

and argblock is:
(plus:SI (reg/f:SI 104 virtual-incoming-args)
     (const_int 16 [0x10]))

and the size of arg->value that we want to push to argblock is 12 bytes.
Of course, a write of 12 bytes from arg->value to argblock would overwrite
arg->value itself, so calls.c has logic to avoid it.
However, it doesn't work properly in this case.
What concerned me was that argblock has the value
(crtl->args.internal_arg_pointer + 16) rather than just crtl->args.internal_arg_pointer.
The code above the emit_push_insn call in store_one_arg that does the pushing of the rogue
argument that is supposed to check for the argument overlap case seems to assume that
argblock is just crtl->args.internal_arg_pointer

Looking around, that '16' is crtl->args.pretend_args_size. From what I understand, this is
non-zero when passing an argument partially in registers and we need to take it into account
when detecting these overlaps just above the call to emit_push_insn.

This patch fixes the issue by looking at the sum of arg->locate.offset.constant and
crtl->args.pretend_args_size rather than just arg->locate.offset.constant.

With this patch the sibcall is rejected as expected.
Bootstrapped and tested on arm, aarch64, x86_64.

I didn't see any codegen difference for arm-none-linux-gnueabihf on the whole of SPEC2006
so I hope this is not a high-impact change and it shouldn't reject any legitimate sibcall
cases.

I'm not terribly familiar with this code and the exact purposes of the various arg_data and
crtl->args fields, so please let me know if this is the wrong direction.

If this is right, ok for trunk?

The wrong-code has been reported the GCC 5 branch as well and I suspect 4.9 is also affected,
but I haven't checked. If this is ok, I'd like to test it on those branches for backporting
as well.

Thanks,
Kyrill

2015-11-24  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR rtl-optimization/67226
     * calls.c (store_one_arg): Take into account
     crtl->args.pretend_args_size when checking for overlap between
     arg->value and argblock + arg->locate.offset during sibcall
     optimization.

2015-11-24  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR rtl-optimization/67226
     * gcc.c-torture/execute/pr67226.c: New test.

On 25/11/15 12:45, Bernd Schmidt wrote:
> On 11/25/2015 01:41 PM, Bernd Schmidt wrote:

>> /* arg.locate doesn't contain the pretend_args_size offset, it's part of

>>     argblock.  Ensure we don't count it in I.  */

>> #ifdef STACK_GROWS_DOWNWARD

>>    i -= crtl->args.pretend_args_size

>> #else etc.

>

> Hmm, yours looks equivalent, just addressing the problem from the other direction, except for the STACK_GROWS_DOWNWARD thing. If you fix that, either approach is OK, but watch formatting here (needs extra parens):


Thanks, I'll go with your version, except that for GCC 6 we have removed conditional compilation on STACK_GROWS_DOWNWARD
so this would be written as:
if (STACK_GROWS_DOWNWARD)
   i -= crtl->args.pretend_args_size;

What should we do when we don't have STACK_GROWS_DOWNWARD? Do we need to write:
if (STACK_GROWS_DOWNWARD)
   i -= crtl->args.pretend_args_size;
else
   i += crtl->args.pretend_args_size;

?

Kyrill

>

>> +          int argblock_offset = arg->locate.offset.constant

>> +                     + crtl->args.pretend_args_size;

>

> and a bit later it looks like there's a linebreak you could eliminate because things now fit into 80 characters.

>

>

> Bernd

>

On 25/11/15 14:12, Bernd Schmidt wrote:
> On 11/25/2015 03:10 PM, Kyrill Tkachov wrote:

>

>> What should we do when we don't have STACK_GROWS_DOWNWARD? Do we need to

>> write:

>> if (STACK_GROWS_DOWNWARD)

>>    i -= crtl->args.pretend_args_size;

>> else

>>    i += crtl->args.pretend_args_size;

>

> I think so. I mean, this should mirror this code here, right?

>

>       /* The argument block when performing a sibling call is the

>      incoming argument block.  */

>       if (pass == 0)

>     {

>       argblock = crtl->args.internal_arg_pointer;

>       if (STACK_GROWS_DOWNWARD)

>         argblock

>           = plus_constant (Pmode, argblock, crtl->args.pretend_args_size);

>       else

>         argblock

>           = plus_constant (Pmode, argblock, -crtl->args.pretend_args_size);

>


Yes, you're right.
I'll send out an updated version of the patch shortly.

Kyrill

>

> Bernd

>