[ELF] - Allow relocation to a weak undefined symbol when -z notext is given.

[grimar]

Summary

Previously we failed to resolve them:
"relocation R_X86_64_32 cannot be used against shared object; recompile with -fPIC"

This finishes fixing PR35720.

[emaste]

This LGTM, I had a similar change in my repo while trying to fix PR35720.

ELF/Relocations.cpp

607 ↗

(On Diff #128076)

I think this comment was previously a logical continuation of the "is simply writable" comment, and the "Or," and ", too." don't quite really follow with the reordering, maybe just "If we are allowed ... a dynamic relocation as we want."

[grimar]

• Updated comment.

ELF/Relocations.cpp

607 ↗

(On Diff #128076)

Thanks, Ed !

[emaste]

LGTM.

[emaste]

Conflicts after rL321430. I tried a straightforward rebase (moving the if ((S.Flags & SHF_WRITE) || !Config->ZText) block below if (Sym.isUndefWeak()) but test/ELF/retain-und.s (from rL293093) fails.

[grimar]

In D41551#963986, @emaste wrote:

Conflicts after rL321430. I tried a straightforward rebase (moving the if ((S.Flags & SHF_WRITE) || !Config->ZText) block below if (Sym.isUndefWeak()) but test/ELF/retain-und.s (from rL293093) fails.

That testcase looks close to what this patch trying to fix. It contains weak undef.
Both gold and bfd of v2.29.51.20171006 does not produce relocation for that testcase.

It seems to me we should fix that testcase as well. I'll rebase this patch now.

[grimar]

• Rebased.

[grimar]

Ah no. Actually gold and bfd of v2.29.51.20171006 produce relocation when -shared is given (I tested without). And do not produce it for executable.
Looking again.

[grimar]

Ok, I think GNU linkers do correct thing. We also should produce the relocation for week undefined when building shared library.
Imagine next code:

dso.c:

extern __attribute__((visibility("default"), weak)) unsigned long long foo();
unsigned long long func() {
  return &foo;
}
unsigned long long run() {
 return func();
}

main.c:

unsigned long long run();

unsigned long long foo() {
 return 1;
}

int main() {
 return run() > 0 ? 1 : 0;
}

If we do not produce the relocation for week undefined foo when linkind DSO, code would work incorrectly:

~/LLVM/build/bin/clang -o dso dso.c -shared -fuse-ld=lld -O0 -z notext
~/LLVM/build/bin/clang -o main main.c dso -fuse-ld=lld -O0
./main 
echo $?
0

Output should be 1.

I'll update the patch and testcase.

[grimar]

• Updated in according to previous comments.

[emaste]

The original FreeBSD test case (a 'hello world' linked with lld -znotext) works with this change applied.

[emaste]

ELF/Relocations.cpp

593 ↗

(On Diff #128158)

minor nit, "and we are producing an executable"

[emaste]

LGTM

lld/trunk/ELF/Relocations.cpp

static RelExpr getPltExpr(Symbol &Sym, RelExpr Expr, bool &IsConstant) {
   IsConstant = true;
   return toPlt(Expr);
 }
 
 template <class ELFT>
 static RelExpr adjustExpr(Symbol &Sym, RelExpr Expr, RelType Type,
                           InputSectionBase &S, uint64_t RelOff,
                           bool &IsConstant) {
+  // If a relocation can be applied at link-time, we don't need to
+  // create a dynamic relocation in the first place.
+  if (IsConstant)
+    return Expr;
+
+  // If the relocation is to a weak undef, and we are producing
+  // executable, give up on it and produce a non preemptible 0.
+  if (!Config->Shared && Sym.isUndefWeak()) {
+    Sym.IsPreemptible = false;
+    IsConstant = true;
+    return Expr;
+  }
+
   // If a section writable or if we are allowed to create dynamic relocations
   // against read-only sections (i.e. when "-z notext" is given), we can create
   // any dynamic relocation the dynamic linker knows how to handle.
   if ((S.Flags & SHF_WRITE) || !Config->ZText) {
     // We use PLT for relocations that may overflow in runtime,
     // see comment for getPltExpr().
     if (Sym.isFunc() && !Target->isPicRel(Type))
       return getPltExpr(Sym, Expr, IsConstant);
     return Expr;
   }
 
-  // If a relocation can be applied at link-time, we don't need to
-  // create a dynamic relocation in the first place.
-  if (IsConstant)
-    return Expr;
-
   // If we got here we know that this relocation would require the dynamic
   // linker to write a value to read only memory.
 
-  // If the relocation is to a weak undef, give up on it and produce a
-  // non preemptible 0.
-  if (Sym.isUndefWeak()) {
-    Sym.IsPreemptible = false;
-    IsConstant = true;
-    return Expr;
-  }
-
   // We can hack around it if we are producing an executable and
   // the refered symbol can be preemepted to refer to the executable.
   if (Config->Shared || (Config->Pic && !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" +
         getLocation(S, Sym, RelOff));

lld/trunk/test/ELF/znotext-weak-undef.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+# RUN: not ld.lld -z notext -shared %t.o -o %t 2>&1 | FileCheck %s
+# CHECK: relocation R_X86_64_32 cannot be used against shared object; recompile with -fPIC
+
+# RUN: ld.lld -z notext %t.o -o %t
+# RUN: llvm-readobj -r %t | FileCheck %s --check-prefix=EXE
+# EXE:      Relocations [
+# EXE-NEXT: ]
+
+.text
+.global foo
+.weak foo
+
+_start:
+mov $foo,%eax