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.tkac...@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.tkac...@arm.com> PR rtl-optimization/67226 * gcc.c-torture/execute/pr67226.c: New test.
commit 28b13fd99664480bfb7f5a606dd89719a0aa0b6f Author: Kyrylo Tkachov <kyrylo.tkac...@arm.com> Date: Mon Nov 23 13:19:57 2015 +0000 PR rtl-optimization/67226: Take into account pretend_args_size when checking stack offsets for sibcall optimisation diff --git a/gcc/calls.c b/gcc/calls.c index b56556a..233e1e6 100644 --- a/gcc/calls.c +++ b/gcc/calls.c @@ -4944,17 +4944,19 @@ store_one_arg (struct arg_data *arg, rtx argblock, int flags, && arg->locate.size.var == 0 && CONST_INT_P (size_rtx)); - if (arg->locate.offset.constant > i) + int argblock_offset = arg->locate.offset.constant + + crtl->args.pretend_args_size; + if (argblock_offset > i) { - if (arg->locate.offset.constant < i + INTVAL (size_rtx)) + if (argblock_offset < i + INTVAL (size_rtx)) sibcall_failure = 1; } - else if (arg->locate.offset.constant < i) + else if (argblock_offset < i) { /* Use arg->locate.size.constant instead of size_rtx because we only care about the part of the argument on the stack. */ - if (i < (arg->locate.offset.constant + if (i < (argblock_offset + arg->locate.size.constant)) sibcall_failure = 1; } diff --git a/gcc/testsuite/gcc.c-torture/execute/pr67226.c b/gcc/testsuite/gcc.c-torture/execute/pr67226.c new file mode 100644 index 0000000..c533496 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/execute/pr67226.c @@ -0,0 +1,42 @@ +struct assembly_operand +{ + int type, value, symtype, symflags, marker; +}; + +struct assembly_operand to_input, from_input; + +void __attribute__ ((__noinline__, __noclone__)) +assemblez_1 (int internal_number, struct assembly_operand o1) +{ + if (o1.type != from_input.type) + __builtin_abort (); +} + +void __attribute__ ((__noinline__, __noclone__)) +t0 (struct assembly_operand to, struct assembly_operand from) +{ + if (to.value == 0) + assemblez_1 (32, from); + else + __builtin_abort (); +} + +int +main (void) +{ + to_input.value = 0; + to_input.type = 1; + to_input.symtype = 2; + to_input.symflags = 3; + to_input.marker = 4; + + from_input.value = 5; + from_input.type = 6; + from_input.symtype = 7; + from_input.symflags = 8; + from_input.marker = 9; + + t0 (to_input, from_input); + + return 0; +}