| Message ID | 564A57BA.7050504@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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; +} +
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.