[ELF][PPC64] Improve "call lacks nop" diagnostic and make it compatible with GCC<5.5 and GCC<6.4

[MaskRay]

Summary

GCC before r245813 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79439)
did not emit nop after b/bl. This can happen with recursive calls.
r245813 was back ported to GCC 5.5 and GCC 6.4.

This is common, for example, libstdc++.a(locale.o) shipped with GCC 4.9
and many objects in netlib lapack can cause lld to error. gold allows
such calls to the same section. Our __plt_foo symbol's section field
is used for ThunkSection, so we can't implement a similar loosen rule
easily. But we can make use of its file field which is currently NULL.

[MaskRay]

🤖 🤖 🤖

[MaskRay]

Mention this is https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79439

[MaskRay]

Add more comments

[sfertile]

Relaxing the check will allow those old miss-compiled objects to link, but we are loosing some utility here as well: we are turning a valid link time failure into a potential runtime failure. Even worse a runtime failure that depends on the environment at load time so the failure is not consistent. Could we guard the error relaxation with a specific option so that 1) It must be opted into specifically and 2) We can eventually phase the option out once the OS that used gcc 5.* and 6.* go out of service?

[MaskRay]

Sorry for my being rechless...

In D71639#1801733, @sfertile wrote:

Relaxing the check will allow those old miss-compiled objects to link, but we are loosing some utility here as well: we are turning a valid link time failure into a potential runtime failure. Even worse a runtime failure that depends on the environment at load time so the failure is not consistent. Could we guard the error relaxation with a specific option so that 1) It must be opted into specifically and 2) We can eventually phase the option out once the OS that used gcc 5.* and 6.* go out of service?

I think gold does a very similar thing here. Now the question is whether we consider this diagnostic helpful...

Let me contribute one data point: It seems that now this error only flags hand-written assembly that does not have nop after bl. I have tested in our internal code base. There were many link failures due to precompiled libstdc++/Fortran LAPACK object files. With this fix, there is no link failure.

[sfertile]

I can definitely see it from the opposite side: We want LLD to be usable on these systems that shipped with this bug in libstdc++ and gcc/gfortran. And adding a link option can be hard to do in some build setups so its not as simple as I make it sound. What worries me though is that one of the compilers could subtly break and with the linker[s] being sloppy with the error then we don't find out until long after the toolchain has shipped.

I think you are right that this will only be triggered on handwritten assembly right now. That is still useful even though not many people write assembly now days. Even experts can mess up, for example taking code written for an executable, then linking it into a shared object since it looks like its already position independent could trigger this error because a tail-call that was valid in an exec isn't in the shared object. These kinds of details aren't really documented anywhere. They are difficult to figure out with the error, but much much worse when you are diagnosing runtime corruptions.

[MaskRay]

In D71639#1802237, @sfertile wrote:

I can definitely see it from the opposite side: We want LLD to be usable on these systems that shipped with this bug in libstdc++ and gcc/gfortran. And adding a link option can be hard to do in some build setups so its not as simple as I make it sound. What worries me though is that one of the compilers could subtly break and with the linker[s] being sloppy with the error then we don't find out until long after the toolchain has shipped.

I think you are right that this will only be triggered on handwritten assembly right now. That is still useful even though not many people write assembly now days. Even experts can mess up, for example taking code written for an executable, then linking it into a shared object since it looks like its already position independent could trigger this error because a tail-call that was valid in an exec isn't in the shared object. These kinds of details aren't really documented anywhere. They are difficult to figure out with the error, but much much worse when you are diagnosing runtime corruptions.

I am hesitant to call it a gcc/gfortran bug (that's why I don't use the term "bug" to describe the issue). The GCC report was originally motivated by a glibc use case that does dlopen RTLD_LOCAL. RTLD_LOCAL does not work in various situations and it just appears to work in some circumstances. In a C++ context it will likely be an one-definition-rule violation.

For the hand-written assembly case, to trigger an uncaught error after this patch, the code needs to have the following setting:

// a.so
bar:
  bl foo
  # no nop
  ...

.globl foo   # foo defined by a.so is preempted by the executable or another shared object at runtime
foo:
  ...

The symbol preemption here makes it not a likely scenario. If you still feel strong about this, I can add an option to warn such cases, and make it off by default (if it is on by default, Linux distributions will have to turn off this option. And in the future, when we remove the code, the distributions will have to delete it)

[sfertile]

I agree that the runtime symbol preemption makes the failure unlikely. I disagree its not a bug though: it doesn't matter that FORTRAN and C++ have the 'one-definition-rule' because the generated code isn't ABI compliant. A compiler producing a valid C++/Fortran program that isn't ABI compliant is definitely a bug. The case I am much more concerned about is code like this:

    .abiversion 2

    .text
    .globl  Nop_Needed
    .p2align        2
    .type   Nop_Needed,@function
Nop_Needed:
.Lfunc_begin0:
    blr
    .long   0
    .quad   0
.Lfunc_end0:
    .size   Nop_Needed, .Lfunc_end0-.Lfunc_begin0

    .globl  No_Nop_Needed
    .p2align        2
    .type   No_Nop_Needed,@function
No_Nop_Needed:                          # @No_Nop_Needed
.Lfunc_begin1:
.Lfunc_gep1:
    addis 2, 12, .TOC.-.Lfunc_gep1@ha
    addi 2, 2, .TOC.-.Lfunc_gep1@l
.Lfunc_lep1:
    .localentry     No_Nop_Needed, .Lfunc_lep1-.Lfunc_gep1
    li 3, 0
    b Nop_Needed
    .long   0
    .quad   0
.Lfunc_end1:
    .size   No_Nop_Needed, .Lfunc_end1-.Lfunc_begin1

I realize now though that if you assemble this with clang instead of gas, we don't get the error because clang will not emit a relocation for b Nop_Needed. This is by design ( clang may have done IPO between the 2 function definitions) but breaks ELF shared-object interposition semantics :frown:. If assembled with gas, or if the functions are in different sections, then with both bfd and gold I get the call lacks nop, can't restore toc; error with linkers from binutils 2.26,2.27 and 2.29 and don't with LLD after your change.

I can add an option to warn such cases, and make it off by default (if it is on by default, Linux distributions will have to turn off this option. And in the future, when we remove the code, the distributions will have to delete it)

Why would Linux distributions have to turn it off? How many Linux distros were compiled with a gcc with this problem? RHEL 7.x will be affected but I think RHEL 8.x has fixed system compilers. Ubuntu and Debian should not need to relax the error either. (Jessie might have the problem but that goes out of service soon).

Is there a way to configure the toolchain driver to add the option when targeting/configuring for RHEL 7.x only? For example I think Ubuntu switched to always PIE with release 18.04, that must be done through the driver configuration. Could we do something similar?

Sorry for my being rechless...

Sorry I missed this earlier I should have got to reviewing it before Xmas break. FWIW I don't think your change is reckless. This is a huge problem for any of the distros affected and we do need to fix it, and relaxing the error to make LLD work on those systems is unavoidable. I feel strongly that the code given in my example should always be a fatal link time error. It can lead to a shared object that is corrupt when linked into one particular program but perfectly valid when linked into a different program. The case you presented I think can be relaxed although ideally we do it in a way where similar future codegen problems can't sneak in.

[MaskRay]

In D71639#1803561, @sfertile wrote:

I agree that the runtime symbol preemption makes the failure unlikely. I disagree its not a bug though: it doesn't matter that FORTRAN and C++ have the 'one-definition-rule' because the generated code isn't ABI compliant. A compiler producing a valid C++/Fortran program that isn't ABI compliant is definitely a bug. The case I am much more concerned about is code like this:

    .abiversion 2

    .text
    .globl  Nop_Needed
    .p2align        2
    .type   Nop_Needed,@function
Nop_Needed:
.Lfunc_begin0:
    blr
    .long   0
    .quad   0
.Lfunc_end0:
    .size   Nop_Needed, .Lfunc_end0-.Lfunc_begin0

    .globl  No_Nop_Needed
    .p2align        2
    .type   No_Nop_Needed,@function
No_Nop_Needed:                          # @No_Nop_Needed
.Lfunc_begin1:
.Lfunc_gep1:
    addis 2, 12, .TOC.-.Lfunc_gep1@ha
    addi 2, 2, .TOC.-.Lfunc_gep1@l
.Lfunc_lep1:
    .localentry     No_Nop_Needed, .Lfunc_lep1-.Lfunc_gep1
    li 3, 0
    b Nop_Needed
    .long   0
    .quad   0
.Lfunc_end1:
    .size   No_Nop_Needed, .Lfunc_end1-.Lfunc_begin1

I realize now though that if you assemble this with clang instead of gas, we don't get the error because clang will not emit a relocation for b Nop_Needed. This is by design ( clang may have done IPO between the 2 function definitions) but breaks ELF shared-object interposition semantics :frown:. If assembled with gas, or if the functions are in different sections, then with both bfd and gold I get the call lacks nop, can't restore toc; error with linkers from binutils 2.26,2.27 and 2.29 and don't with LLD after your change.

I can add an option to warn such cases, and make it off by default (if it is on by default, Linux distributions will have to turn off this option. And in the future, when we remove the code, the distributions will have to delete it)

Why would Linux distributions have to turn it off? How many Linux distros were compiled with a gcc with this problem? RHEL 7.x will be affected but I think RHEL 8.x has fixed system compilers. Ubuntu and Debian should not need to relax the error either. (Jessie might have the problem but that goes out of service soon).

Is there a way to configure the toolchain driver to add the option when targeting/configuring for RHEL 7.x only? For example I think Ubuntu switched to always PIE with release 18.04, that must be done through the driver configuration. Could we do something similar?

Sorry for my being rechless...

Sorry I missed this earlier I should have got to reviewing it before Xmas break. FWIW I don't think your change is reckless. This is a huge problem for any of the distros affected and we do need to fix it, and relaxing the error to make LLD work on those systems is unavoidable. I feel strongly that the code given in my example should always be a fatal link time error. It can lead to a shared object that is corrupt when linked into one particular program but perfectly valid when linked into a different program. The case you presented I think can be relaxed although ideally we do it in a way where similar future codegen problems can't sneak in.

I am still hesitant to call any ABI non-conformance a bug. It is not uncommon that implementations differ a bit from ABI and ABI may describe something that is unnecessary. I agree that for the problem in question, the ABI is correct, and GCC<5.4 and GCC<6.4 had a problem with symbol preemption under RTLD_LOCAL.

Created D72183 to add --lax-call-lacks-nop. As you observed (b Nop_Needed), ppc64-error-toc-tail-call.s does not break when I make the suppresion opt-in (via --lax-call-lacks-nop). I'm investigating the assembler bug.

lld/ELF/InputSection.cpp

case R_PPC64_CALL:
       // If this is a call to __tls_get_addr, it may be part of a TLS
       // sequence that has been relaxed and turned into a nop. In this
       // case, we don't want to handle it as a call.
       if (read32(bufLoc) == 0x60000000) // nop
         break;
 
       // Patch a nop (0x60000000) to a ld.
       if (rel.sym->needsTocRestore) {
-        if (bufLoc + 8 > bufEnd || read32(bufLoc + 4) != 0x60000000) {
-          error(getErrorLocation(bufLoc) + "call lacks nop, can't restore toc");
+        // gcc/gfortran 5.4, 6.3 and earlier versions do not add nop for
+        // recursive calls even if the function is preemptible. This is not
+        // wrong in the common case where the function is not preempted at
+        // runtime. Just ignore.
+        if ((bufLoc + 8 > bufEnd || read32(bufLoc + 4) != 0x60000000) &&
+            rel.sym->file != file) {
+          // Use substr(6) to remove the "__plt_" prefix.
+          errorOrWarn(getErrorLocation(bufLoc) + "call to " +
+                      lld::toString(*rel.sym).substr(6) +
+                      " lacks nop, can't restore toc");
           break;
         }
         write32(bufLoc + 4, 0xe8410018); // ld %r2, 24(%r1)
       }
       target->relocateOne(bufLoc, type, targetVA);
       break;
     default:
       target->relocateOne(bufLoc, type, targetVA);

lld/ELF/Thunks.cpp

void PPC64PltCallStub::writeTo(uint8_t *buf) {
   write32(buf + 0, 0xf8410018); // std     r2,24(r1)
   writePPC64LoadAndBranch(buf + 4, offset);
 }
 
 void PPC64PltCallStub::addSymbols(ThunkSection &isec) {
   Defined *s = addSymbol(saver.save("__plt_" + destination.getName()), STT_FUNC,
                          0, isec);
   s->needsTocRestore = true;
+  s->file = destination.file;
 }
 
 void PPC64LongBranchThunk::writeTo(uint8_t *buf) {
   int64_t offset = in.ppc64LongBranchTarget->getEntryVA(&destination, addend) -
                    getPPC64TocBase();
   writePPC64LoadAndBranch(buf, offset);
 }
 

lld/test/ELF/ppc64-bsymbolic-toc-restore.s

 # REQUIRES: ppc
 
 # RUN: llvm-mc -filetype=obj -triple=powerpc64le-unknown-linux %s -o %t1.o
 # RUN: llvm-mc -filetype=obj -triple=powerpc64le-unknown-linux %p/Inputs/ppc64-bsymbolic-local-def.s  -o %t2.o
 # RUN: ld.lld -Bsymbolic -shared %t1.o %t2.o -o %t
 # RUN: llvm-objdump -d -r %t | FileCheck %s
 # RUN: not ld.lld -shared %t1.o %t2.o -o %t 2>&1 | FileCheck --check-prefix=FAIL %s
 
 # RUN: llvm-mc -filetype=obj -triple=powerpc64-unknown-linux %s -o %t1.o
 # RUN: llvm-mc -filetype=obj -triple=powerpc64-unknown-linux %p/Inputs/ppc64-bsymbolic-local-def.s  -o %t2.o
 # RUN: ld.lld -Bsymbolic -shared %t1.o %t2.o -o %t
 # RUN: llvm-objdump -d -r %t | FileCheck %s
 # RUN: not ld.lld -shared %t1.o %t2.o -o %t 2>&1 | FileCheck --check-prefix=FAIL %s
 
-# FAIL: call lacks nop, can't restore toc
+# FAIL: call to def lacks nop, can't restore toc
 
 # Test to document the toc-restore behavior with -Bsymbolic option. Since
 # -Bsymbolic causes the call to bind to the internal defintion we know the
 # caller and callee share the same TOC base. This means branching to the
 # local entry point of the callee, and no need for a nop to follow the call
 # (since there is no need to restore the TOC-pointer after the call).
 
         .abiversion 2

lld/test/ELF/ppc64-error-toc-restore.s

 // REQUIRES: ppc
 
 // RUN: llvm-mc -filetype=obj -triple=powerpc64le-unknown-linux %s -o %t.o
 // RUN: llvm-mc -filetype=obj -triple=powerpc64le-unknown-linux %p/Inputs/shared-ppc64.s -o %t2.o
 // RUN: ld.lld -shared %t2.o -o %t2.so
 // RUN: not ld.lld %t.o %t2.so -o %t 2>&1 | FileCheck %s
 
 // RUN: llvm-mc -filetype=obj -triple=powerpc64-unknown-linux %s -o %t.o
 // RUN: llvm-mc -filetype=obj -triple=powerpc64-unknown-linux %p/Inputs/shared-ppc64.s -o %t2.o
 // RUN: ld.lld -shared %t2.o -o %t2.so
 // RUN: not ld.lld %t.o %t2.so -o %t 2>&1 | FileCheck %s
 
 # Calling external function bar needs a nop
-// CHECK: call lacks nop, can't restore toc
+// CHECK: call to foo lacks nop, can't restore toc
     .text
     .abiversion 2
 
 .global _start
 _start:
   bl foo

lld/test/ELF/ppc64-error-toc-tail-call.s

 // REQUIRES: ppc
 
 // RUN: llvm-mc -filetype=obj -triple=powerpc64le-unknown-linux %s -o %t.o
 // RUN: llvm-mc -filetype=obj -triple=powerpc64le-unknown-linux %p/Inputs/shared-ppc64.s -o %t2.o
 // RUN: ld.lld -shared %t2.o -o %t2.so
 // RUN: not ld.lld %t.o %t2.so -o %t 2>&1 | FileCheck %s
 
 // RUN: llvm-mc -filetype=obj -triple=powerpc64-unknown-linux %s -o %t.o
 // RUN: llvm-mc -filetype=obj -triple=powerpc64-unknown-linux %p/Inputs/shared-ppc64.s -o %t2.o
 // RUN: ld.lld -shared %t2.o -o %t2.so
 // RUN: not ld.lld %t.o %t2.so -o %t 2>&1 | FileCheck %s
 
 # A tail call to an external function without a nop should issue an error.
-// CHECK: call lacks nop, can't restore toc
+// CHECK: call to foo lacks nop, can't restore toc
+// CHECK-NOT: lacks nop
     .text
     .abiversion 2
 
 .global _start
 _start:
   b foo
+
+  // gcc/gfortran 5.4, 6.3 and earlier versions do not add nop for recursive
+  // calls.
+  b _start
+  b _start