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

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

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 3106 bytes --]

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.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: calls-pretend.patch --]
[-- Type: text/x-patch; name=calls-pretend.patch, Size: 2381 bytes --]

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;
+}

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

[Bernd Schmidt]

On 11/24/2015 04:12 PM, Kyrill Tkachov wrote:
> 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]))
>
> Looking around, that '16' is crtl->args.pretend_args_size.

> 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.

Ok, I have this in gdb now trying to understand the issue.

Isn't the problem simply that we're comparing two values and one of them 
is offset by pretend_args_size? I.e. after this,

     if (XEXP (x, 0) != crtl->args.internal_arg_pointer)
       i = INTVAL (XEXP (XEXP (x, 0), 1));

shouldn't it be sufficient to just undo the pretend_args_size offset 
like this:

/* 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.


Bernd

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

[Bernd Schmidt]

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):

> +	      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

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

[Kyrill Tkachov]

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
>

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

[Bernd Schmidt]

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);


Bernd

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

[Kyrill Tkachov]

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
>

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

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 1964 bytes --]


On 25/11/15 14:14, Kyrill Tkachov wrote:
>
> 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.
>

And here it is.
This fixes the bug on arm and tests on arm look ok.
I've kicked off bootstraps and tests on arm, aarch64 and x86_64.
Ok for trunk if they come back clean?
The first version of the patch that I posted should be equivalent to this one on those targets
(as you noted).

I'll prepare a backport for GCC 5 and 4.9 as it occurs there as well.
We'll have to use the #ifdef check for STACK_GROWS_DOWNWARD on those branches...

Thanks,
Kyrill

2015-11-25  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
             Bernd Schmidt  <bschmidt@redhat.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-25  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

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

> Kyrill
>
>>
>> Bernd
>>
>


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: calls-pretend.patch --]
[-- Type: text/x-patch; name=calls-pretend.patch, Size: 2075 bytes --]

commit cec1dd7490b3a5931aefac5aeafdb1380af9437f
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..6cbe019 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -4939,6 +4939,13 @@ store_one_arg (struct arg_data *arg, rtx argblock, int flags,
 	      if (XEXP (x, 0) != crtl->args.internal_arg_pointer)
 		i = INTVAL (XEXP (XEXP (x, 0), 1));
 
+	      /* arg.locate doesn't contain the pretend_args_size offset,
+		 it's part of argblock.  Ensure we don't count it in I.  */
+	      if (STACK_GROWS_DOWNWARD)
+		i -= crtl->args.pretend_args_size;
+	      else
+		i += crtl->args.pretend_args_size;
+
 	      /* expand_call should ensure this.  */
 	      gcc_assert (!arg->locate.offset.var
 			  && arg->locate.size.var == 0
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;
+}

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

[Bernd Schmidt]

On 11/25/2015 03:54 PM, Kyrill Tkachov wrote:
> And here it is.
> This fixes the bug on arm and tests on arm look ok.
> I've kicked off bootstraps and tests on arm, aarch64 and x86_64.
> Ok for trunk if they come back clean?

Sure. Thanks! (Backport too.)


Bernd