[ELF] Move (copy relocation/canonical PLT) before error checking

[MaskRay]

Summary

In processRelocAux(), we handle errors before copy relocation/canonical PLT.
This makes error checking a bit complex because we have to check for
conditions that will be allowed by copy relocation/canonical PLT.

Instead, move copy relocation/canonical PLT before error checking. This
simplifies the previous clumsy error checking code

config->shared || (config->pie && expr == R_ABS && type != target->symbolicRel)

to the simple config->isPic. Some diagnostics can be reported in
different ways. The code motion changes diagnostics for some contrived
test cases:

• copy-rel-pie-error.s -> copy-rel-pie2.s: It was rejected before but accepted now. ld.bfd also accepts the case.
• copy-errors.s: "cannot preempt symbol" changes to "symbol 'bar' has no type"
• got32{,x}-i386.s: the suggestion changes from "-fPIC or -Wl,-z,notext" to "-fPIE"
• x86-64-dyn-rel-error5.s: one diagnostic changes for -pie case

[MaskRay]

simplify the code

[MaskRay]

further simplification

[peter.smith]

Not got a strong opinion here, happy to go with the consensus.

ELF/Relocations.cpp

1004 ↗

(On Diff #214356)

Could this trigger if !(sym.isFunc() || sym.isObject())? as this code path could exist in the previous version, but not with this one. This could be fixed by removing the (sym.isFunc() || sym.isObject()) from the initial if and reinstating if (sym.isFunc()) later.

1076 ↗

(On Diff #214356)

Maybe able to simplify as Config->isPic is true in both error messages.

if (config->isPic) {
  if (!canWrite && !isRel(expr)) {
    error("can't create dynamic relocation");
  } else {
    errorOrWarn("relocation" + toString(type) + " cannot be used against " + ...);
  }
  return;
}

[MaskRay]

Address review comments

[peter.smith]

Thanks for the update, I've no more comments.

[MaskRay]

% factor 66007 # ping
66007: 149 443

[peter.smith]

I'm happy with the changes. Looks good to me.

lld/trunk/ELF/Relocations.cpp

if (expr == R_GOT || (rel == target->symbolicRel && !sym.isPreemptible)) {
       // a dynamic relocation.
       // ftp://www.linux-mips.org/pub/linux/mips/doc/ABI/mipsabi.pdf p.4-19
       if (config->emachine == EM_MIPS)
         in.mipsGot->addEntry(*sec.file, sym, addend, expr);
       return;
     }
   }
 
-  if (!canWrite && (config->isPic && !isRelExpr(expr))) {
-    error(
-        "can't create dynamic relocation " + toString(type) + " against " +
-        (sym.getName().empty() ? "local symbol" : "symbol: " + toString(sym)) +
-        " in readonly segment; recompile object files with -fPIC "
-        "or pass '-Wl,-z,notext' to allow text relocations in the output" +
-        getLocation(sec, sym, offset));
-    return;
-  }
-
-  // Copy relocations (for STT_OBJECT) and canonical PLT (for STT_FUNC) are only
-  // possible in an executable.
-  //
-  // Among R_ABS relocatoin types, symbolicRel has the same size as the word
-  // size. Others have fewer bits and may cause runtime overflow in -pie/-shared
-  // mode. Disallow them.
-  if (config->shared ||
-      (config->pie && expr == R_ABS && type != target->symbolicRel)) {
-    errorOrWarn(
-        "relocation " + toString(type) + " cannot be used against " +
-        (sym.getName().empty() ? "local symbol" : "symbol " + toString(sym)) +
-        "; recompile with -fPIC" + getLocation(sec, sym, offset));
-    return;
-  }
-
-  // If the symbol is undefined we already reported any relevant errors.
-  if (sym.isUndefined())
-    return;
-
+  // When producing an executable, we can perform copy relocations (for
+  // STT_OBJECT) and canonical PLT (for STT_FUNC).
+  if (!config->shared) {
     if (!canDefineSymbolInExecutable(sym)) {
-    error("cannot preempt symbol: " + toString(sym) +
+      errorOrWarn("cannot preempt symbol: " + toString(sym) +
                   getLocation(sec, sym, offset));
       return;
     }
 
     if (sym.isObject()) {
       // Produce a copy relocation.
       if (auto *ss = dyn_cast<SharedSymbol>(&sym)) {
         if (!config->zCopyreloc)
           error("unresolvable relocation " + toString(type) +
                 " against symbol '" + toString(*ss) +
                 "'; recompile with -fPIC or remove '-z nocopyreloc'" +
                 getLocation(sec, sym, offset));
         addCopyRelSymbol<ELFT>(*ss);
       }
       sec.relocations.push_back({expr, type, offset, addend, &sym});
       return;
     }
 
-  if (sym.isFunc()) {
     // This handles a non PIC program call to function in a shared library. In
     // an ideal world, we could just report an error saying the relocation can
     // overflow at runtime. In the real world with glibc, crt1.o has a
     // R_X86_64_PC32 pointing to libc.so.
     //
     // The general idea on how to handle such cases is to create a PLT entry and
     // use that as the function value.
     //
if (!config->shared) {
     // R_386_JMP_SLOT, etc).
 
     // For position independent executable on i386, the plt entry requires ebx
     // to be set. This causes two problems:
     // * If some code has a direct reference to a function, it was probably
     //   compiled without -fPIE/-fPIC and doesn't maintain ebx.
     // * If a library definition gets preempted to the executable, it will have
     //   the wrong ebx value.
+    if (sym.isFunc()) {
       if (config->pie && config->emachine == EM_386)
         errorOrWarn("symbol '" + toString(sym) +
                     "' cannot be preempted; recompile with -fPIE" +
                     getLocation(sec, sym, offset));
       if (!sym.isInPlt())
         addPltEntry<ELFT>(in.plt, in.gotPlt, in.relaPlt, target->pltRel, sym);
       if (!sym.isDefined())
         replaceWithDefined(
             sym, in.plt,
             target->pltHeaderSize + target->pltEntrySize * sym.pltIndex, 0);
       sym.needsPltAddr = true;
       sec.relocations.push_back({expr, type, offset, addend, &sym});
       return;
     }
+  }
+
+  if (config->isPic) {
+    if (!canWrite && !isRelExpr(expr))
+      errorOrWarn(
+          "can't create dynamic relocation " + toString(type) + " against " +
+          (sym.getName().empty() ? "local symbol"
+                                 : "symbol: " + toString(sym)) +
+          " in readonly segment; recompile object files with -fPIC "
+          "or pass '-Wl,-z,notext' to allow text relocations in the output" +
+          getLocation(sec, sym, offset));
+    else
+      errorOrWarn(
+          "relocation " + toString(type) + " cannot be used against " +
+          (sym.getName().empty() ? "local symbol" : "symbol " + toString(sym)) +
+          "; recompile with -fPIC" + getLocation(sec, sym, offset));
+    return;
+  }
+
+  // If the symbol is undefined we already reported any relevant errors.
+  if (sym.isUndefined())
+    return;
 
   errorOrWarn("symbol '" + toString(sym) + "' has no type" +
               getLocation(sec, sym, offset));
 }
 
 template <class ELFT, class RelTy>
 static void scanReloc(InputSectionBase &sec, OffsetGetter &getOffset, RelTy *&i,
                       RelTy *end) {

lld/trunk/test/ELF/copy-errors.s

 // REQUIRES: x86
 // RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
 // RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %p/Inputs/protected-shared.s -o %t2.o
 // RUN: ld.lld %t2.o -o %t2.so -shared
 // RUN: not ld.lld %t.o %t2.so -o %t 2>&1 | FileCheck %s
 
-// CHECK: cannot preempt symbol: bar
+// CHECK: error: cannot preempt symbol: bar
 // CHECK: >>> defined in {{.*}}.so
 // CHECK: >>> referenced by {{.*}}.o:(.text+0x1)
 
 // CHECK: error: symbol 'zed' has no type
 // CHECK-NEXT: >>> defined in {{.*}}.so
 // CHECK-NEXT: >>> referenced by {{.*}}.o:(.text+0x6)
 
-// RUN: not ld.lld --noinhibit-exec %t.o %t2.so -o %t 2>&1 | FileCheck %s --check-prefix=NOINHIBIT
+// RUN: ld.lld --noinhibit-exec %t.o %t2.so -o %t 2>&1 | FileCheck %s --check-prefix=NOINHIBIT
+// NOINHIBIT: warning: cannot preempt symbol: bar
+// NOINHIBIT-NEXT: >>> defined in {{.*}}.so
+// NOINHIBIT-NEXT: >>> referenced by {{.*}}.o:(.text+0x1)
 // NOINHIBIT: warning: symbol 'zed' has no type
 // NOINHIBIT-NEXT: >>> defined in {{.*}}.so
 // NOINHIBIT-NEXT: >>> referenced by {{.*}}.o:(.text+0x6)
 
 .global _start
 _start:
 .byte 0xe8
 .long bar - .
 .byte 0xe8
 .long zed - .

lld/trunk/test/ELF/copy-rel-pie-error.s

-// REQUIRES: x86
-// RUN: llvm-mc %s -o %t.o -filetype=obj -triple=x86_64-pc-linux
-// RUN: llvm-mc %p/Inputs/copy-rel-pie.s -o %t2.o -filetype=obj -triple=x86_64-pc-linux
-// RUN: ld.lld %t2.o -o %t2.so -shared
-// RUN: not ld.lld %t.o %t2.so -o /dev/null -pie 2>&1 | FileCheck %s
-
-// CHECK: can't create dynamic relocation R_X86_64_64 against symbol: bar in readonly segment; recompile object files with -fPIC or pass '-Wl,-z,notext' to allow text relocations in the output
-// CHECK: >>> defined in {{.*}}.so
-// CHECK: >>> referenced by {{.*}}.o:(.text+0x0)
-
-// CHECK: can't create dynamic relocation R_X86_64_64 against symbol: foo in readonly segment; recompile object files with -fPIC or pass '-Wl,-z,notext' to allow text relocations in the output
-// CHECK: >>> defined in {{.*}}.so
-// CHECK: >>> referenced by {{.*}}.o:(.text+0x8)
-
-.global _start
-_start:
-        .quad bar
-        .quad foo

lld/trunk/test/ELF/copy-rel-pie2.s

+// REQUIRES: x86
+// RUN: llvm-mc %s -o %t.o -filetype=obj -triple=x86_64-pc-linux
+// RUN: llvm-mc %p/Inputs/copy-rel-pie.s -o %t2.o -filetype=obj -triple=x86_64-pc-linux
+// RUN: ld.lld %t2.o -o %t2.so -shared
+// RUN: ld.lld %t.o %t2.so -o %t -pie
+// RUN: llvm-readobj -r %t | FileCheck %s
+
+// CHECK: R_X86_64_COPY
+// CHECK: R_X86_64_JUMP_SLOT
+
+.rodata
+.quad bar
+.quad foo

lld/trunk/test/ELF/got32-i386.s

 ## 73728 == 0x12000 == ADDR(.got)
 # CHECK:       _start:
 # CHECK-NEXT:   401001: 8b 1d {{.*}}  movl 4202496, %ebx
 # CHECK: Sections:
 # CHECK:  Name Size     VMA
 # CHECK:  .got 00000004 0000000000402000
 
 # RUN: not ld.lld %t.o -o %t -pie 2>&1 | FileCheck %s --check-prefix=ERR
-# ERR: error: can't create dynamic relocation R_386_GOT32 against symbol: foo in readonly segment; recompile object files with -fPIC or pass '-Wl,-z,notext' to allow text relocations in the output
+# ERR: error: symbol 'foo' cannot be preempted; recompile with -fPIE

lld/trunk/test/ELF/got32x-i386.s

 # CHECK-NEXT:   40100d: 8b 80 {{.*}} movl -4096(%eax), %eax
 # CHECK-NEXT:   401013: 8b 83 {{.*}} movl -4096(%ebx), %eax
 # CHECK: Sections:
 # CHECK:  Name Size     VMA
 # CHECK:  .got.plt 0000000c 0000000000403000
 
 # RUN: not ld.lld %S/Inputs/i386-got32x-baseless.elf -o %t1 -pie 2>&1 | \
 # RUN:   FileCheck %s --check-prefix=ERR
-# ERR: error: can't create dynamic relocation R_386_GOT32X against symbol: foo in readonly segment; recompile object files with -fPIC or pass '-Wl,-z,notext' to allow text relocations in the output
-# ERR: error: can't create dynamic relocation R_386_GOT32X against symbol: foo in readonly segment; recompile object files with -fPIC or pass '-Wl,-z,notext' to allow text relocations in the output
+# ERR-COUNT-2: error: symbol 'foo' cannot be preempted; recompile with -fPIE

lld/trunk/test/ELF/x86-64-dyn-rel-error5.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
-# RUN: not ld.lld -pie %t.o -o /dev/null 2>&1 | FileCheck %s
-# RUN: not ld.lld -shared %t.o -o /dev/null 2>&1 | FileCheck %s
+# RUN: not ld.lld -pie %t.o -o /dev/null 2>&1 | FileCheck --check-prefixes=CHECK,PIE %s
+# RUN: not ld.lld -shared %t.o -o /dev/null 2>&1 | FileCheck --check-prefixes=CHECK,SHARED %s
 
 ## Check we don't create dynamic relocations in a writable section,
 ## if the number of bits is smaller than the wordsize.
 
 .globl hidden
 .hidden hidden
 local:
 hidden:
 
 # CHECK: error: relocation R_X86_64_8 cannot be used against local symbol; recompile with -fPIC
 # CHECK-NEXT: >>> defined in {{.*}}.o
 # CHECK-NEXT: >>> referenced by {{.*}}.o:(.data+0x0)
 # CHECK: error: relocation R_X86_64_16 cannot be used against local symbol; recompile with -fPIC
 # CHECK: error: relocation R_X86_64_32 cannot be used against local symbol; recompile with -fPIC
-# CHECK: error: relocation R_X86_64_32 cannot be used against symbol hidden; recompile with -fPIC
+
+# PIE: error: cannot preempt symbol: hidden
+# SHARED: error: relocation R_X86_64_32 cannot be used against symbol hidden; recompile with -fPIC
 
 .data
 .byte local     # R_X86_64_8
 .short local    # R_X86_64_16
 .long local     # R_X86_64_32
 
 .long hidden    # R_X86_64_32