[ELF] Don't special case weak symbols for pie with no shared objects

[MaskRay]

Summary

D59275 added the following clause to Symbol::includeInDynsym()

if (isUndefWeak() && Config->Pie && SharedFiles.empty())
  return false;

D59549 explored the possibility to generalize it for -no-pie.

GNU ld's rules are architecture dependent and partly controlled by -z
{,no-}dynamic-undefined-weak. Our attempts to mimic its rules are
actually half-baked and don't provide perceivable benefits (it can save
a few more weak undefined symbols in .dynsym in a -static-pie
executable). Let's just delete the rule for simplicity.

This makes D71795 NFC.

[MaskRay]

Add reviewers

[MaskRay]

lld/test/ELF/ppc32-weak-undef-call.s

15

This behavior change does not affect anything. I added this file just to check our behavior regarding the glibc weak undefined symbol __gmon_start__. The instruction will not be called when it is undefined, so we don't care the target of the branch.

[MaskRay]

Ping :)

[peter.smith]

Apologies for the delay in responding, the original came in when I was on vacation.

• Am I correct in thinking that the extra dynamic symbols that will result with -pie and no shared libraries are just wasted space, but won't cause anything to happen at run time as there are no dynamic relocations generated against them?
• The main argument for removing is that this our ld.bfd is ad-hoc and this part doesn't help much, removing it would simplify here and permit a simplification in D71795. Is there a plan beyond D71795 to more closely follow ld.bfd behaviour, or is this just an area where we expect cosmetic inconsistencies?

At the moment I'm happy to go ahead with this if we've got a good idea of what our desired end-goal is.

[MaskRay]

In D71794#1807562, @peter.smith wrote:

Apologies for the delay in responding, the original came in when I was on vacation.

• Am I correct in thinking that the extra dynamic symbols that will result with -pie and no shared libraries are just wasted space, but won't cause anything to happen at run time as there are no dynamic relocations generated against them?

Yes, it just wastes some space on -static-pie programs, like ~5 symbols. It doesn't cause anything at run time. A statically linked executable does not use shared objects, so what is left in .dynsym does not matter. Well, glibc supports dlopen from a statically linked executable, and the loaded shared object may use symbols from the executable. In that case, the link should use --export-dynamic or --dynamic-list, instead of relying on the ~5 weak symbols in .dynsym.

• The main argument for removing is that this our ld.bfd is ad-hoc and this part doesn't help much, removing it would simplify here and permit a simplification in D71795. Is there a plan beyond D71795 to more closely follow ld.bfd behaviour, or is this just an area where we expect cosmetic inconsistencies?

At the moment I'm happy to go ahead with this if we've got a good idea of what our desired end-goal is.

It does not help. It can get in the away in certain places due to the different behaviors between -no-pie and -pie. Yes, it permits a simplification in D71795. We will expect cosmetic inconsistencies with ld.bfd in certain undefined weak scenarios. ld.bfd treats undefined weak symbols in an architecture dependent manner, and not many people understand the differences well. lld should not follow its rules. We may need to abide by psABI in some cases, but make our reasonable decisions in others.

[peter.smith]

Ok LGTM. May be worth waiting a day to see if there are any final objections.

lld/ELF/Symbols.cpp

 }
 
 bool Symbol::includeInDynsym() const {
   if (!config->hasDynSymTab)
     return false;
   if (computeBinding() == STB_LOCAL)
     return false;
 
-  // If a PIE binary was not linked against any shared libraries, then we can
-  // safely drop weak undef symbols from .dynsym.
-  if (isUndefWeak() && config->pie && sharedFiles.empty())
-    return false;
-
   return isUndefined() || isShared() || exportDynamic || inDynamicList;
 }
 
 // Print out a log message for --trace-symbol.
 void printTraceSymbol(const Symbol *sym) {
   std::string s;
   if (sym->isUndefined())
     s = ": reference to ";

lld/test/ELF/ppc32-weak-undef-call.s

 # REQUIRES: ppc
 # RUN: llvm-mc -filetype=obj -triple=powerpc %s -o %t.o
 # RUN: ld.lld %t.o -o %t
-# RUN: llvm-objdump -d --no-show-raw-insn %t | FileCheck --check-prefix=EXE %s
+# RUN: llvm-objdump -d --no-show-raw-insn %t | FileCheck --check-prefix=PDE %s
 # RUN: ld.lld -pie %t.o -o %t
-# RUN: llvm-objdump -d --no-show-raw-insn %t | FileCheck --check-prefix=EXE %s
+# RUN: llvm-objdump -d --no-show-raw-insn %t | FileCheck --check-prefix=PIC %s
 # RUN: ld.lld -shared %t.o -o %t
-# RUN: llvm-objdump -d --no-show-raw-insn %t | FileCheck --check-prefix=SHARED %s
+# RUN: llvm-objdump -d --no-show-raw-insn %t | FileCheck --check-prefix=PIC %s
 
 ## It does not really matter how we fixup it, but we cannot overflow and
 ## should not generate a call stub (this would waste space).
-# EXE: bl .+0
+# PDE: bl .+0
 
-## With -shared, create a call stub. ld.bfd produces bl .+0
-# SHARED: bl .+4
-# SHARED: 00000000.plt_pic32.foo:
+## With -pie or -shared, create a call stub. ld.bfd produces bl .+0
+# PIC: bl .+4
+# PIC: 00000000.plt_pic32.foo:
 
 .weak foo
 bl foo

lld/test/ELF/weak-undef-no-shared-libs.s

-// REQUIRES: x86
-// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
-// RUN: ld.lld -pie %t.o -o %t
-// RUN: llvm-readobj -V --dyn-syms %t | FileCheck %s
-
-        .globl _start
-_start:
-        .type foo,@function
-        .weak foo
-        .long foo@gotpcrel
-
-// Test that an entry for weak undefined symbols is NOT emitted in .dynsym as
-// the PIE was not linked with any shared libraries. There are other tests which
-// ensure that the weak undefined symbols do get emitted in .dynsym for PIEs
-// linked against dynamic libraries.
-
-
-// CHECK:      DynamicSymbols [
-// CHECK-NEXT:   Symbol {
-// CHECK-NEXT:     Name:
-// CHECK-NEXT:     Value: 0x0
-// CHECK-NEXT:     Size: 0
-// CHECK-NEXT:     Binding: Local (0x0)
-// CHECK-NEXT:     Type: None (0x0)
-// CHECK-NEXT:     Other: 0
-// CHECK-NEXT:     Section: Undefined (0x0)
-// CHECK-NEXT:   }
-// CHECK-NEXT: ]