On Wed, Mar 01, 2017 at 05:21:44AM -0600, Segher Boessenkool wrote:
> 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?
Yeah, it can probably go into GCC 5 if the branch is still open. The original
report was against GCC 6.
> 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?
I think all do, but I restricted it to powerpc64*-*-linux to be sure.
> > +/* { dg-require-effective-target powerpc_p8vector_ok } */
>
> Why this?
Because I forgot to remove it, when I cloned another test.
> > +/* 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.
I rewrote the comment.
> > +/* { 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 :-)
Does scan-assembler-times go past 1 line?
In any case, here is the diff for the changes I checked in:
[gcc]
2017-03-01 Michael Meissner <meiss...@linux.vnet.ibm.com>
PR target/79439
* config/rs6000/predicates.md (current_file_function_operand): Do
not allow self calls to be local if the function is replaceable.
[gcc/testsuite]
2017-03-01 Michael Meissner <meiss...@linux.vnet.ibm.com>
PR target/79439
* gcc.target/powerpc/pr79439.c: New test.
--
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meiss...@linux.vnet.ibm.com, phone: +1 (978) 899-4797
Index: gcc/config/rs6000/predicates.md
===================================================================
--- gcc/config/rs6000/predicates.md (revision 245812)
+++ 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 (working copy)
@@ -0,0 +1,29 @@
+/* { dg-do compile { target { powerpc64*-*-linux* && lp64 } } } */
+/* { dg-options "-O2 -fpic" } */
+
+/* On the Linux 64-bit ABIs, we should not eliminate NOP in the 'rec' call if
+ -fpic is used because rec can be interposed at link time (since it is
+ external), and the recursive call should call the interposed function. The
+ Linux 32-bit ABIs do not require NOPs after the BL instruction. */
+
+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 {\mbl f\M} 1 } } */
+/* { dg-final { scan-assembler-times {\mbl g\M} 1 } } */
+/* { dg-final { scan-assembler-times {\mbl rec\M} 1 } } */
+/* { dg-final { scan-assembler-times {\mnop\M} 3 } } */
