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

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

[Kugan]

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

Following testcase fails on ARM (from
https://bugs.linaro.org/show_bug.cgi?id=1900).

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


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?

Thanks,
Kugan

gcc/ChangeLog:

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

	* config/arm/arm.c (arm_function_ok_for_sibcall): Disable sibcall to
	indirect function when TARGET_HARD_FLOAT.

gcc/testsuite/ChangeLog:

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

	* gcc.target/arm/variadic_sibcall.c: New test.


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

diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index a379121..8b560bc 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -6681,6 +6681,12 @@ arm_function_ok_for_sibcall (tree decl, tree exp)
 	 register.  */
       rtx a, b;
 
+      /* When it is an indirect call (i.e, decl == NULL), it could be
+	 returning its result in a VFP or could be a variadic function.
+	 Thus return false.  */
+      if (!decl && TARGET_HARD_FLOAT)
+	return false;
+
       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/variadic_sibcall.c b/gcc/testsuite/gcc.target/arm/variadic_sibcall.c
index e69de29..86f07fe 100644
--- a/gcc/testsuite/gcc.target/arm/variadic_sibcall.c
+++ b/gcc/testsuite/gcc.target/arm/variadic_sibcall.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;
+}
+

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

[Charles Baylis]

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

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

[Kugan]

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
> 

Hi Charles,

I wrongly thought that for indirect call we wouldn't know if it is
variadic or not. I should check stdarg_p here.

But we should really fix aapcs_allocate_return_reg as it is simply
setting  pcs_variant = arm_pcs_default without checking if this is
stdarg_p. I will send an updated patch.

Thanks,
Kugan

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

[Kugan]

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:

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)));
+
       a = arm_function_value (TREE_TYPE (exp), decl, false);
       b = arm_function_value (TREE_TYPE (DECL_RESULT (cfun->decl)),
                              cfun->decl, false);


Thanks,
Kugan

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

[Ramana Radhakrishnan]

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.


regards
Ramana

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

[Kugan]

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

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

[Kugan]

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


> 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.
> 
> 
Hi Ramana,

With further testing on bare-metal, I found that for the following decl
has to be null for indirect functions.

  if (TARGET_AAPCS_BASED
      && arm_abi == ARM_ABI_AAPCS
      && decl
      && DECL_WEAK (decl))
    return false;

Here is the updated patch and ChangeLog. Sorry for the noise.

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: 1371 bytes --]

diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index a379121..0dae7da 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -6680,8 +6680,13 @@ arm_function_ok_for_sibcall (tree decl, tree exp)
 	 a VFP register but then need to transfer it to a core
 	 register.  */
       rtx a, b;
+      tree fn_decl = decl;
 
-      a = arm_function_value (TREE_TYPE (exp), decl, false);
+      /* If it is an indirect function pointer, get the function type.  */
+      if (!decl)
+	fn_decl = TREE_TYPE (TREE_TYPE (CALL_EXPR_FN (exp)));
+
+      a = arm_function_value (TREE_TYPE (exp), fn_decl, false);
       b = arm_function_value (TREE_TYPE (DECL_RESULT (cfun->decl)),
 			      cfun->decl, false);
       if (!rtx_equal_p (a, b))
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;
+}
+

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

[Ramana Radhakrishnan]

On 18/11/15 00:32, Kugan wrote:
>> > 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.
>> > 
>> > 
> Hi Ramana,
> 
> With further testing on bare-metal, I found that for the following decl
> has to be null for indirect functions.
> 
>   if (TARGET_AAPCS_BASED
>       && arm_abi == ARM_ABI_AAPCS
>       && decl
>       && DECL_WEAK (decl))
>     return false;

Ok .. yes that's right.

> 
> Here is the updated patch and ChangeLog. Sorry for the noise.
> 
> 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.
> 

s/PR/pr in the test name and put this in gcc.c-torture/execute instead - there is nothing ARM specific about the test. Tests in gcc.target/arm should really only be architecture specific. This isn't.

> 
> 
> 
> p.txt
> 
> 
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index a379121..0dae7da 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -6680,8 +6680,13 @@ arm_function_ok_for_sibcall (tree decl, tree exp)
>  	 a VFP register but then need to transfer it to a core
>  	 register.  */
>        rtx a, b;
> +      tree fn_decl = decl;

Call it decl_or_type instead - it's really that ... 

>  
> -      a = arm_function_value (TREE_TYPE (exp), decl, false);
> +      /* If it is an indirect function pointer, get the function type.  */
> +      if (!decl)
> +	fn_decl = TREE_TYPE (TREE_TYPE (CALL_EXPR_FN (exp)));
> +

This is probably just my mail client - but please watch out for indentation.

> +      a = arm_function_value (TREE_TYPE (exp), fn_decl, false);
>        b = arm_function_value (TREE_TYPE (DECL_RESULT (cfun->decl)),
>  			      cfun->decl, false);
>        if (!rtx_equal_p (a, b))


OK with those changes.

Ramana

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

[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.
>>
> 
> s/PR/pr in the test name and put this in gcc.c-torture/execute instead - there is nothing ARM specific about the test. Tests in gcc.target/arm should really only be architecture specific. This isn't.
> 
>>
>>
>>
>> p.txt
>>
>>
>> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
>> index a379121..0dae7da 100644
>> --- a/gcc/config/arm/arm.c
>> +++ b/gcc/config/arm/arm.c
>> @@ -6680,8 +6680,13 @@ arm_function_ok_for_sibcall (tree decl, tree exp)
>>  	 a VFP register but then need to transfer it to a core
>>  	 register.  */
>>        rtx a, b;
>> +      tree fn_decl = decl;
> 
> Call it decl_or_type instead - it's really that ... 
> 
>>  
>> -      a = arm_function_value (TREE_TYPE (exp), decl, false);
>> +      /* If it is an indirect function pointer, get the function type.  */
>> +      if (!decl)
>> +	fn_decl = TREE_TYPE (TREE_TYPE (CALL_EXPR_FN (exp)));
>> +
> 
> This is probably just my mail client - but please watch out for indentation.
> 
>> +      a = arm_function_value (TREE_TYPE (exp), fn_decl, false);
>>        b = arm_function_value (TREE_TYPE (DECL_RESULT (cfun->decl)),
>>  			      cfun->decl, false);
>>        if (!rtx_equal_p (a, b))
> 
> 
> OK with those changes.
> 
> Ramana
> 


Hi Ramana,

This issue also remains in 4.9 and 5.0 branches. Is this OK to backport
to the release branches.

Thanks,
Kugan

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

[Kugan]

This issue also remains in 4.9 and 5.0 branches. Is this OK to backport
to the release branches.

Thanks,
Kugan

On 02/12/15 10:00, Kugan wrote:
> 
>>>
>>> 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.
>>>
>>
>> s/PR/pr in the test name and put this in gcc.c-torture/execute instead - there is nothing ARM specific about the test. Tests in gcc.target/arm should really only be architecture specific. This isn't.
>>
>>>
>>>
>>>
>>> p.txt
>>>
>>>
>>> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
>>> index a379121..0dae7da 100644
>>> --- a/gcc/config/arm/arm.c
>>> +++ b/gcc/config/arm/arm.c
>>> @@ -6680,8 +6680,13 @@ arm_function_ok_for_sibcall (tree decl, tree exp)
>>>  	 a VFP register but then need to transfer it to a core
>>>  	 register.  */
>>>        rtx a, b;
>>> +      tree fn_decl = decl;
>>
>> Call it decl_or_type instead - it's really that ... 
>>
>>>  
>>> -      a = arm_function_value (TREE_TYPE (exp), decl, false);
>>> +      /* If it is an indirect function pointer, get the function type.  */
>>> +      if (!decl)
>>> +	fn_decl = TREE_TYPE (TREE_TYPE (CALL_EXPR_FN (exp)));
>>> +
>>
>> This is probably just my mail client - but please watch out for indentation.
>>
>>> +      a = arm_function_value (TREE_TYPE (exp), fn_decl, false);
>>>        b = arm_function_value (TREE_TYPE (DECL_RESULT (cfun->decl)),
>>>  			      cfun->decl, false);
>>>        if (!rtx_equal_p (a, b))
>>
>>
>> OK with those changes.
>>
>> Ramana
>>

>