[ELF] Don't emit dynamic relocations with weak undef in writable sections

[MaskRay]

Summary

In processRelocAux(), our handling of 1) link-time constant and 2) weak
undef is the same, so put them together to simplify the logic.

This moves the weak undef code around. The result is that:

For a writable section, we will not emit dynamic relocations for weak undefined
symbols.

Since this simplifies the logic, improves consistency with the readonly
case, and also seems to match what GNU linkers do, so this is probably
good to do.

The condition !Config->Shared was there probably because it is common
for a -shared link not to specify full dependencies. Keep it now but we
may revisit the decision in the future.

gABI says:

The behavior of weak symbols in areas not specified by this document is
implementation defined. Weak symbols are intended primarily for use in
system software. Applications using weak symbols are unreliable since
changes in the runtime environment might cause the execution to fail.

With this change, we can simplify another place
Symbol::includeInDynsym() (see D59549) a bit.

[grimar]

test/ELF/weak-undef-rw.s

10 ↗

(On Diff #203528)

What is GNU linkers behavior in this case?

[MaskRay]

Emphasize the benefits

[MaskRay]

test/ELF/weak-undef-rw.s

10 ↗

(On Diff #203528)

GNU linkers don't emit dynamic relocations for executables, so this change seems to improves compatibility.

[ruiu]

Does this mean that we previously emit a dynamic relocation for a weak undefined symbol that couldn't have been resolved at link time, and this patch changes the behavior so that weak undefined symbols must be resolved to some symbol at link time. Am I correct?

If so, that change seems a little risky, as it will silently change a program behavior. If the following three conditions are met, a program will behave differently.

1. An object file contains a weak undefined symbol
2. No DSOs define the weak undefined symbol at link time
3. But some DSO defines the weak undefined symbol at run time

I'd think that the above conditions are minor, and you generally shouldn't rely on that kind of edge cases, though.

Ultimately, it seems like it is ultimately a programming language and ELF's failure that they underrepresents the difference between global variables defined in other modules and global variables defined in the same module. The ELF's model tries to hide the difference between static linking and dynamic linking, but the difference leaks at various places like this. We have only one type of "weak" undefined type, but in dynamic linking, a symbol is given two chances to be resolved (static link time and run time), so it is hard to say what is a correct behavior.

[ruiu]

test/ELF/weak-undef-rw.s

10 ↗

(On Diff #203528)

Hm, if this is the same as GNU linker, I believe it's a safe change in practice.

[MaskRay]

In weak-undef-rw.s, mention this matches ld.bfd and gold

[MaskRay]

I've tested this internally. This is a safe change.

[ruiu]

Was my previous comment correct? (I'm not suggesting any change at the moment but I'd like to make sure that we are on the same page.)

[MaskRay]

Your previous comment is correct.

1. An object file contains a weak undefined symbol

2. No DSOs define the weak undefined symbol at link time

3. But some DSO defines the weak undefined symbol at run time

Currently:

• If !CanWrite, no dynamic relocation is created (cannot be preempted by a runtime DSO).
• If CanWrite, a dynamic relocation is created (can be preempted by a runtime DSO).

This patch makes it:

• If !CanWrite, no dynamic relocation is created
• If CanWrite, no dynamic relocation is created

This also appears to be what ld.bfd and gold do.

[MaskRay]

Shall we give it a try?

[ruiu]

Yes, let's land this. LGTM

lld/trunk/ELF/Relocations.cpp

 // is needed after applying relocations. If it is, it will go after rw and rx
 // sections. Given that it is ro, we will need an extra PT_LOAD. This
 // complicates things for the dynamic linker and means we would have to reserve
 // space for the extra PT_LOAD even if we end up not using it.
 template <class ELFT, class RelTy>
 static void processRelocAux(InputSectionBase &Sec, RelExpr Expr, RelType Type,
                             uint64_t Offset, Symbol &Sym, const RelTy &Rel,
                             int64_t Addend) {
-  if (isStaticLinkTimeConstant(Expr, Type, Sym, Sec, Offset)) {
+  // If the relocation is known to be a link-time constant, we know no dynamic
+  // relocation will be created, pass the control to relocateAlloc() or
+  // relocateNonAlloc() to resolve it.
+  //
+  // The behavior of an undefined weak reference is implementation defined. If
+  // the relocation is to a weak undef, and we are producing an executable, let
+  // relocate{,Non}Alloc() resolve it.
+  if (isStaticLinkTimeConstant(Expr, Type, Sym, Sec, Offset) ||
+      (!Config->Shared && Sym.isUndefWeak())) {
     Sec.Relocations.push_back({Expr, Type, Offset, Addend, &Sym});
     return;
   }
+
   bool CanWrite = (Sec.Flags & SHF_WRITE) || !Config->ZText;
   if (CanWrite) {
     // FIXME Improve the way we handle absolute relocation types that will
     // change to relative relocations. ARM has a relocation type R_ARM_TARGET1
     // that is similar to SymbolicRel. PPC64 may have similar relocation types.
     if ((!Sym.IsPreemptible &&
          (Config->EMachine == EM_ARM || Config->EMachine == EM_PPC64 ||
           Type == Target->SymbolicRel)) ||
if ((!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 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()) {
-    Sec.Relocations.push_back({Expr, Type, Offset, Addend, &Sym});
-    return;
-  }
-
   if (!CanWrite && (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 "
         "or pass '-Wl,-z,notext' to allow text relocations in the output" +
         getLocation(Sec, Sym, Offset));
     return;

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

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
 # RUN: ld.lld %t.o -o %t --export-dynamic
-# RUN: llvm-readobj -r %t | FileCheck %s
+# RUN: llvm-readelf -r %t | FileCheck %s
 
-# CHECK: R_X86_64_64 foobar 0x0
+## gABI leaves the behavior of weak undefined references implementation defined.
+## We choose to resolve it statically and not create a dynamic relocation for
+## implementation simplicity. This also matches ld.bfd and gold.
+
+# CHECK: no relocations
 
         .global _start
 _start:
         .data
         .weak foobar
         .quad foobar