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.vivekanandara...@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 <kug...@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 <kug...@linaro.org>
PR target/68390
* gcc.target/arm/PR68390.c: New test.
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
