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

[Kyrill Tkachov <kyrylo dot tkachov at arm dot com>]

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.

commit 28b13fd99664480bfb7f5a606dd89719a0aa0b6f
Author: Kyrylo Tkachov <kyrylo.tkachov@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;
+}