| Message ID | 20170301063714.GA14043@ibm-tiger.the-meissners.org |
|---|---|
| State | New |
| Headers | show |
On Wed, Mar 01, 2017 at 01:37:14AM -0500, Michael Meissner wrote: > This patch fixes PR target/79439, which is a recursive call when the 64-bit > code is compiled with -fpic doesn't have the NOP after the call. It is > possible for the function to be overriden at link time. In such a case, the > call should call the module that is overriding the call, rather than itself. > > The following patch was tested on a little endian Power8 Linux system (64-bit > only), a big endian Power8 Linux system (both 32-bit and 64-bit), and a big > endian Power7 Linux system (both 32-bit and 64-bit). There were no regressions > in the test suite, and I verified that the new test ran successfully in 64-bit > mode. Can I check this patch into the trunk? Yes, thanks! > Since the bug was reported against GCC 6, can I apply the patch to GCC 6 > assuming the patch applies cleanly and has no regressions after a burn in > period on the GCC 7 trunk? Of course. Also for GCC 5, if it is worth fixing it there? Some questions/comments about the testcase: > Index: gcc/testsuite/gcc.target/powerpc/pr79439.c > =================================================================== > --- gcc/testsuite/gcc.target/powerpc/pr79439.c (revision 0) > +++ gcc/testsuite/gcc.target/powerpc/pr79439.c (revision 0) > @@ -0,0 +1,26 @@ > +/* { dg-do compile { target { powerpc*-*-* && lp64 } } } */ Is this enough? Do all 64-bit ABIs have the insn to be patched after call instructions? > +/* { dg-require-effective-target powerpc_p8vector_ok } */ Why this? > +/* Bug 79439 -- we should not eliminate NOP in 'rec' call because it can be > + interposed at link time for 64-bit ABIs. We need -fpic to tell the compiler > + functions may be interposed. */ That reads as "cannot be interposed on 32-bit ABIs", which isn't what you mean I think. > +/* { dg-final { scan-assembler-times {\mnop\M} 3 } } */ You can also check they follow a "bl" insn immediately (scan-assembler does not scan single lines, but the whole output). Something like { scan-assembler-times {\mbl \S+\s+nop\M} 3 } Or maybe this is overkill here :-) Segher
Index: gcc/config/rs6000/predicates.md =================================================================== --- gcc/config/rs6000/predicates.md (revision 245787) +++ gcc/config/rs6000/predicates.md (working copy) @@ -1110,7 +1110,8 @@ (define_predicate "current_file_function (and (match_code "symbol_ref") (match_test "(DEFAULT_ABI != ABI_AIX || SYMBOL_REF_FUNCTION_P (op)) && (SYMBOL_REF_LOCAL_P (op) - || op == XEXP (DECL_RTL (current_function_decl), 0)) + || (op == XEXP (DECL_RTL (current_function_decl), 0) + && !decl_replaceable_p (current_function_decl))) && !((DEFAULT_ABI == ABI_AIX || DEFAULT_ABI == ABI_ELFv2) && (SYMBOL_REF_EXTERNAL_P (op) Index: gcc/testsuite/gcc.target/powerpc/pr79439.c =================================================================== --- gcc/testsuite/gcc.target/powerpc/pr79439.c (revision 0) +++ gcc/testsuite/gcc.target/powerpc/pr79439.c (revision 0) @@ -0,0 +1,26 @@ +/* { dg-do compile { target { powerpc*-*-* && lp64 } } } */ +/* { dg-require-effective-target powerpc_p8vector_ok } */ +/* { dg-options "-O2 -fpic" } */ + +/* Bug 79439 -- we should not eliminate NOP in 'rec' call because it can be + interposed at link time for 64-bit ABIs. We need -fpic to tell the compiler + functions may be interposed. */ + +int f (void); + +void +g (void) +{ +} + +int +rec (int a) +{ + int ret = 0; + if (a > 10 && f ()) + ret += rec (a - 1); + g (); + return a + ret; +} + +/* { dg-final { scan-assembler-times {\mnop\M} 3 } } */