diff mbox

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

Message ID 564BC70C.70805@linaro.org
State New
Headers show

Commit Message

Kugan Vivekanandarajah Nov. 18, 2015, 12:32 a.m. UTC
> 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.

Comments

Kugan Vivekanandarajah Dec. 1, 2015, 11 p.m. UTC | #1
>>

>> 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
Kugan Vivekanandarajah Jan. 25, 2016, 9:54 p.m. UTC | #2
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

>>


>
diff mbox

Patch

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