Re: Incorrect code due to indirect tail call of varargs function with hard float ABI

[Kugan <kugan.vivekanandarajah@linaro.org>]

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



On 17/11/15 21:05, Ramana Radhakrishnan wrote:
> Hi Kugan,
> 
> It does look like an issue.
> 
> Please open a bug report.
> 
>>
>>
>> On 17/11/15 12:00, Charles Baylis wrote:
>>> On 16 November 2015 at 22:24, Kugan <kugan.vivekanandarajah@linaro.org> wrote:
>>>
>>>> Please note that we have a sibcall from "broken" to "indirect".
>>>>
>>>> "direct" is variadic function so it is conforming to AAPCS base standard.
>>>>
>>>> "broken" is a non-variadic function and will return the value in
>>>> floating point register for TARGET_HARD_FLOAT. Thus we should not be
>>>> doing sibcall here.
>>>>
>>>> Attached patch fixes this. Bootstrap and regression testing is ongoing.
>>>> Is this OK if no issues with the testing?
>>>
>>> Hi Kugan,
>>>
>>> It looks like this patch should work, but I think this is an overly
>>> conservative fix, as it prevents all sibcalls for hardfloat targets.
>>> It would be better if only variadic sibcalls were prevented on
>>> hardfloat. You can check for variadic calls by checking the
>>> function_type in the call expression (exp) using stdarg_p().
>>>
>>> As an example to show how to test for variadic function calls, this is
>>> how to test it in gdb:
>>>
>>> (gdb) b arm_function_ok_for_sibcall
>>> Breakpoint 1 at 0xdae59c: file
>>> /home/cbaylis/srcarea/gcc/gcc-git/gcc/config/arm/arm.c, line 6634.
>>> (gdb) r
>>> <snip>...
>>> Breakpoint 1, arm_function_ok_for_sibcall (decl=0x0, exp=0x7ffff6104ce8)
>>>     at /home/cbaylis/srcarea/gcc/gcc-git/gcc/config/arm/arm.c:6634
>>> 6634      if (cfun->machine->sibcall_blocked)
>>> (gdb) print debug_tree(exp)
>>>  <call_expr 0x7ffff6104ce8
>>>     type <real_type 0x7ffff62835e8 double sizes-gimplified DF
>>>         size <integer_cst 0x7ffff626cf18 constant 64>
>>>         unit size <integer_cst 0x7ffff626cf30 constant 8>
>>>         align 64 symtab 0 alias set -1 canonical type 0x7ffff62835e8
>>> precision 64
>>>         pointer_to_this <pointer_type 0x7ffff62837e0>>
>>>     side-effects addressable
>>>     fn <ssa_name 0x7ffff6275708
>>>         type <pointer_type 0x7ffff60e9540 type <function_type 0x7ffff60e9348>
>>> <snip>...
>>> (gdb) print stdarg_p((tree)0x7ffff60e9348)    <--- from function_type ^^^^^
>>> $2 = true
>>>
>>
>> How about:
> 
> 
> 
> A run time testcase and a changelog would also be needed.
> 
>>
>> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
>> index a379121..2376d66 100644
>> --- a/gcc/config/arm/arm.c
>> +++ b/gcc/config/arm/arm.c
>> @@ -6681,6 +6681,13 @@ arm_function_ok_for_sibcall (tree decl, tree exp)
>>          register.  */
>>        rtx a, b;
>>
>> +      /* If it is an indirect function pointer, get the function type.  */
>> +      if (!decl
>> +         && POINTER_TYPE_P (TREE_TYPE (CALL_EXPR_FN (exp)))
>> +         && (TREE_CODE (TREE_TYPE (TREE_TYPE (CALL_EXPR_FN (exp))))
>> +             == FUNCTION_TYPE))
>> +       decl = TREE_TYPE (TREE_TYPE (CALL_EXPR_FN (exp)));
>> +
> 
> If decl is null it's guaranteed to be an indirect function call - drop the additional checks in the if clause.
> 
> 
>>        a = arm_function_value (TREE_TYPE (exp), decl, false);
>>        b = arm_function_value (TREE_TYPE (DECL_RESULT (cfun->decl)),
>>                               cfun->decl, false);
>>
> 
> 
> Please resubmit with a testcase, Changelog and after testing.

Hi Ramana,

Thanks for the review. I have opened a gcc bug-report for this. I tested
the attached patch for  arm-none-linux-gnueabihf and
arm-none-linux-gnueabi with no new regressions. Is this OK?


Thanks,
Kugan

gcc/ChangeLog:

2015-11-18  Kugan Vivekanandarajah  <kuganv@linaro.org>

	PR target/68390
	* config/arm/arm.c (arm_function_ok_for_sibcall): Get function type
	for indirect function call.

gcc/testsuite/ChangeLog:

2015-11-18  Kugan Vivekanandarajah  <kuganv@linaro.org>

	PR target/68390
	* gcc.target/arm/PR68390.c: New test.



[-- Attachment #2: p.txt --]
[-- Type: text/plain, Size: 1345 bytes --]

diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index a379121..a4509f4 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -6681,6 +6681,10 @@ arm_function_ok_for_sibcall (tree decl, tree exp)
 	 register.  */
       rtx a, b;
 
+      /* If it is an indirect function pointer, get the function type.  */
+      if (!decl)
+	decl = TREE_TYPE (TREE_TYPE (CALL_EXPR_FN (exp)));
+
       a = arm_function_value (TREE_TYPE (exp), decl, false);
       b = arm_function_value (TREE_TYPE (DECL_RESULT (cfun->decl)),
 			      cfun->decl, false);
diff --git a/gcc/testsuite/gcc.target/arm/PR68390.c b/gcc/testsuite/gcc.target/arm/PR68390.c
index e69de29..86f07fe 100644
--- a/gcc/testsuite/gcc.target/arm/PR68390.c
+++ b/gcc/testsuite/gcc.target/arm/PR68390.c
@@ -0,0 +1,27 @@
+/* { dg-do run }  */
+/* { dg-options "-O2" } */
+
+__attribute__ ((noinline))
+double direct(int x, ...)
+{
+  return x*x;
+}
+
+__attribute__ ((noinline))
+double broken(double (*indirect)(int x, ...), int v)
+{
+  return indirect(v);
+}
+
+int main ()
+{
+  double d1, d2;
+  int i = 2;
+  d1 = broken (direct, i);
+  if (d1 != i*i)
+    {
+      __builtin_abort ();
+    }
+  return 0;
+}
+
diff --git a/gcc/testsuite/gcc.target/arm/variadic_sibcall.c b/gcc/testsuite/gcc.target/arm/variadic_sibcall.c
deleted file mode 100644
index e69de29..0000000