Re: [PATCH], PR target/79434, fix PowerPC recursive calls that can replaced at runtime

[Michael Meissner <meissner at linux dot vnet dot ibm dot com>]

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  <meissner@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  <meissner@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: meissner@linux.vnet.ibm.com, phone: +1 (978) 899-4797

Attachment: pr79439.patch02b
Description: Text document