Bug 79439 - Missing nop instruction after recursive call corrupts TOC register
Summary: Missing nop instruction after recursive call corrupts TOC register
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 6.3.1
: P3 normal
Target Milestone: ---
Assignee: Michael Meissner
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-02-09 11:02 UTC by Florian Weimer
Modified: 2017-03-10 20:54 UTC (History)
7 users (show)

See Also:
Host:
Target: ppc64le-redhat-linux
Build:
Known to work:
Known to fail:
Last reconfirmed: 2017-02-09 00:00:00


Attachments
Potential patch to check if self calls are replaceable. (926 bytes, patch)
2017-02-28 23:55 UTC, Michael Meissner
Details | Diff
Potential patch to check if self calls are replaceable #2 (840 bytes, patch)
2017-03-01 00:00 UTC, Michael Meissner
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Florian Weimer 2017-02-09 11:02:22 UTC
Consider this test case:

int f (void);

void
g (void)
{
}

void
rec (int a)
{
  if (f ())
    rec (a + 1);
  rec (a);
  g ();
}

GCC 6.3.1 generates the following code on ppc64le-redhat-linux (Fedora 25):

rec:
.LCF1:
0:      addis 2,12,.TOC.-.LCF1@ha
        addi 2,2,.TOC.-.LCF1@l
        .localentry     rec,.-rec
        mflr 0
        std 31,-8(1)
        mr 31,3
        std 0,16(1)
        stdu 1,-48(1)
        bl f
        nop
        cmpdi 7,3,0
        beq 7,.L3
        addi 3,31,1
        extsw 3,3
        bl rec
.L3:
        mr 3,31
        bl rec
        bl g
        nop
        addi 1,1,48
        ld 0,16(1)
        ld 31,-8(1)
        mtlr 0
        blr
        .long 0
        .byte 0,0,0,1,128,1,0,0


That is, there is no nop instruction the after the “bl rec” instructions.  The ELF v2 ABI requires the presence of this instruction, so that the static linker can replace it with a load of the saved TOC pointer to restore the original value of the TOC pointer register.  

Apparently, GCC assumes that the call is always to a function in the same module, so the TOC pointer restore is never needed.  But “rec” can be interposed from another translation unit and the original function definition can be access through dlsym and executed, so this assumption is incorrect.  The missing TOC pointer restore means that the PLT stubs supplied by the static linker will not work, and subsequent function calls can crash.

This was originally reported as a glibc bug:

  <https://sourceware.org/bugzilla/show_bug.cgi?id=21116>

The test case is LAPACK, which is apparently written in Fortran, but I expect that this is a target issue which exists independent of the front end.
Comment 1 Segher Boessenkool 2017-02-09 12:15:49 UTC
What command line options does this need?  I get different assembly
(also with GCC 6), since GCC recognises that rec can never return:

        .globl rec
        .type   rec, @function
rec:
.LCF1:
0:      addis 2,12,.TOC.-.LCF1@ha
        addi 2,2,.TOC.-.LCF1@l
        .localentry     rec,.-rec
        mflr 0
        std 31,-8(1)
        mr 31,3
        std 0,16(1)
        stdu 1,-48(1)
        .p2align 4,,15
.L3:
        bl f
        nop
        cmpdi 7,3,0
        beq 7,.L3
        addi 3,31,1
        extsw 3,3
        bl rec
        .long 0
        .byte 0,0,0,1,128,1,0,0
Comment 2 Florian Weimer 2017-02-09 12:20:47 UTC
(In reply to Segher Boessenkool from comment #1)
> What command line options does this need?

Sorry, I used -O2 -fpic.

Indeed, GCC seems to perform target-independent optimizations based on an assumption that the recursive call calls the same function as defined in the translation unit.  If this is the desired behavior, the proper fix would be to use a local reference for the recursive call.  This would make those optimizations consistent, and interposition would work to some extent.  Of course, the recursive calls wouldn't be interposed anymore, but the TOC corruption would at least be gone.
Comment 3 Segher Boessenkool 2017-02-09 12:21:46 UTC
-fpic does the trick.  Confirmed.
Comment 4 Segher Boessenkool 2017-02-09 12:37:17 UTC
We have (in rs6000/predicates.md):

;; Return 1 if the operand is a SYMBOL_REF for a function known to be in
;; this file.
(define_predicate "current_file_function_operand"
  (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))
                    && !((DEFAULT_ABI == ABI_AIX
                          || DEFAULT_ABI == ABI_ELFv2)
                         && (SYMBOL_REF_EXTERNAL_P (op)
                             || SYMBOL_REF_WEAK (op)))")))

so this seems to be on purpose.
Comment 5 David Edelsohn 2017-02-09 13:01:09 UTC
current_file_function_operand probably needs to add a test for flag_semantic_interposition when the ABI mandates interpolation.
Comment 6 Jakub Jelinek 2017-02-09 13:09:22 UTC
(In reply to David Edelsohn from comment #5)
> current_file_function_operand probably needs to add a test for
> flag_semantic_interposition when the ABI mandates interpolation.

Maybe better just call decl_replaceable_p (current_function_decl) if not
SYMBOL_REF_LOCAL_P (or something similar).
Comment 7 Alexander Monakov 2017-02-09 15:24:23 UTC
Note that there's also a bug in the middle-end since gcc makes an assumption that such calls are always self-recursive on any target; see comments 6-9 in bug 56727.
Comment 8 Michael Meissner 2017-02-28 23:55:57 UTC
Created attachment 40851 [details]
Potential patch to check if self calls are replaceable.
Comment 9 Michael Meissner 2017-03-01 00:00:00 UTC
Created attachment 40852 [details]
Potential patch to check if self calls are replaceable #2

I deleted a comment by mistake in the first patch.
Comment 10 Michael Meissner 2017-03-01 18:33:53 UTC
Author: meissner
Date: Wed Mar  1 18:33:21 2017
New Revision: 245813

URL: https://gcc.gnu.org/viewcvs?rev=245813&root=gcc&view=rev
Log:
[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.


Added:
    trunk/gcc/testsuite/gcc.target/powerpc/pr79439.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/rs6000/predicates.md
    trunk/gcc/testsuite/ChangeLog
Comment 11 Michael Meissner 2017-03-06 22:47:35 UTC
Author: meissner
Date: Mon Mar  6 22:47:03 2017
New Revision: 245930

URL: https://gcc.gnu.org/viewcvs?rev=245930&root=gcc&view=rev
Log:
[gcc]
2017-03-06  Michael Meissner  <meissner@linux.vnet.ibm.com>

	Back port from trunk
	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-06  Michael Meissner  <meissner@linux.vnet.ibm.com>

	Back port from trunk
	2017-03-01  Michael Meissner  <meissner@linux.vnet.ibm.com>

	PR target/79439
	* gcc.target/powerpc/pr79439.c: New test.



Added:
    branches/gcc-6-branch/gcc/testsuite/gcc.target/powerpc/pr79439.c
      - copied, changed from r245813, trunk/gcc/testsuite/gcc.target/powerpc/pr79439.c
Modified:
    branches/gcc-6-branch/gcc/ChangeLog
    branches/gcc-6-branch/gcc/config/rs6000/predicates.md
    branches/gcc-6-branch/gcc/testsuite/ChangeLog
Comment 12 Michael Meissner 2017-03-10 20:53:50 UTC
Author: meissner
Date: Fri Mar 10 20:53:18 2017
New Revision: 246058

URL: https://gcc.gnu.org/viewcvs?rev=246058&root=gcc&view=rev
Log:
[gcc]
2017-03-10  Michael Meissner  <meissner@linux.vnet.ibm.com>

	Back port from trunk
	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-10  Michael Meissner  <meissner@linux.vnet.ibm.com>

	Back port from trunk
	2017-03-01  Michael Meissner  <meissner@linux.vnet.ibm.com>

	PR target/79439
	* gcc.target/powerpc/pr79439.c: New test.


Added:
    branches/gcc-5-branch/gcc/testsuite/gcc.target/powerpc/pr79439.c
      - copied unchanged from r245930, branches/gcc-6-branch/gcc/testsuite/gcc.target/powerpc/pr79439.c
Modified:
    branches/gcc-5-branch/gcc/ChangeLog
    branches/gcc-5-branch/gcc/config/rs6000/predicates.md
    branches/gcc-5-branch/gcc/testsuite/ChangeLog
Comment 13 Michael Meissner 2017-03-10 20:54:51 UTC
Fixed with check-ins to trunk, and back-ports to gcc 6 and gcc 5.