diff mbox

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

Message ID 564B8655.1060509@linaro.org
State Superseded
Headers show

Commit Message

Kugan Vivekanandarajah Nov. 17, 2015, 7:56 p.m. UTC 
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.
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
diff mbox

Patch

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