[ELF] --fortran-common: prefer STB_WEAK to COMMON

[MaskRay]

Summary

The ELF specification says "The link editor honors the common definition and
ignores the weak ones." GNU ld and our Symbol::compare follow this, but the
--fortran-common code (D86142) made a mistake on the precedence.

[peter.smith]

LGTM. I don't think that the original intention of D86142 was to differ in this regard. Will be worth giving sfertile some time to object if there is a specific reason.

[sfertile]

LGTM, thanks for finding and fixing this MaskRay.

lld/ELF/InputFiles.cpp

 // 1) Consider the tentative definition a 'real' definition (ie promotion from
 //    tentative to real definition has already happened) and not inspect
 //    archive members for Global/Weak definitions to replace the tentative
 //    definition. An archive member would only be included if it satisfies some
 //    other undefined symbol. This is the behavior Gold uses.
 //
 // 2) Consider the tentative definition as still undefined (ie the promotion to
 //    a real definition happens only after all symbol resolution is done).
-//    The linker searches archive members for global or weak definitions to
+//    The linker searches archive members for STB_GLOBAL definitions to
 //    replace the tentative definition with. This is the behavior used by
 //    GNU ld.
 //
 //  The second behavior is inherited from SysVR4, which based it on the FORTRAN
 //  COMMON BLOCK model. This behavior is needed for proper initialization in old
 //  (pre F90) FORTRAN code that is packaged into an archive.
 //
 //  The following functions search archive members for definitions to replace
 //  tentative definitions (implementing behavior 2).
 static bool isBitcodeNonCommonDef(MemoryBufferRef mb, StringRef symName,
                                   StringRef archiveName) {
   IRSymtabFile symtabFile = check(readIRSymtab(mb));
   for (const irsymtab::Reader::SymbolRef &sym :
        symtabFile.TheReader.symbols()) {
     if (sym.isGlobal() && sym.getName() == symName)
-      return !sym.isUndefined() && !sym.isCommon();
+      return !sym.isUndefined() && !sym.isWeak() && !sym.isCommon();
   }
   return false;
 }
 
 template <class ELFT>
 static bool isNonCommonDef(MemoryBufferRef mb, StringRef symName,
                            StringRef archiveName) {
   ObjFile<ELFT> *obj = make<ObjFile<ELFT>>(mb, archiveName);
   StringRef stringtable = obj->getStringTable();
 
   for (auto sym : obj->template getGlobalELFSyms<ELFT>()) {
     Expected<StringRef> name = sym.getName(stringtable);
     if (name && name.get() == symName)
-      return sym.isDefined() && !sym.isCommon();
+      return sym.isDefined() && sym.getBinding() == STB_GLOBAL &&
+             !sym.isCommon();
   }
   return false;
 }
 
 static bool isNonCommonDef(MemoryBufferRef mb, StringRef symName,
                            StringRef archiveName) {
   switch (getELFKind(mb, archiveName)) {
   case ELF32LEKind:

lld/test/ELF/common-archive-lookup.s

 ## Object file archives.
 # RUN: llvm-ar crs 1.a 1.o strong_data_only.o
 # RUN: llvm-ar crs 2.a 1.o weak_data_only.o
 # RUN: llvm-ar crs 3.a 2.o strong_data_only.o
 
 ## Bitcode files.
 # RUN: llvm-as -o 1.bc commonblock.ll
 # RUN: llvm-as -o 2.bc blockdata.ll
+# RUN: llvm-as -o 3.bc weak.ll
 
 ## Bitcode archive.
 # RUN: llvm-ar crs 4.a 1.bc 2.bc
 
 # RUN: ld.lld -o 1 main.o 1.a
 # RUN: llvm-objdump -D -j .data 1 | FileCheck --check-prefix=TEST1 %s
 
 # RUN: ld.lld -o 2 main.o --start-lib 1.o strong_data_only.o --end-lib
 # RUN: llvm-objdump -D -j .data 2 | FileCheck --check-prefix=TEST1 %s
 
 # RUN: ld.lld -o 3 main.o 2.a
-# RUN: llvm-objdump -D -j .data 3 | FileCheck --check-prefix=TEST1 %s
+# RUN: llvm-objdump -t 3 | FileCheck --check-prefix=BSS %s
 
 # RUN: ld.lld -o 4 main.o --start-lib 1.o weak_data_only.o --end-lib
-# RUN: llvm-objdump -D -j .data 4 | FileCheck --check-prefix=TEST1 %s
+# RUN: llvm-objdump -t 4 | FileCheck --check-prefix=BSS %s
 
 # RUN: ld.lld -o 5 main.o 3.a --print-map | FileCheck --check-prefix=MAP %s
 
 # RUN: ld.lld -o 6 main.o 2.o 1.a
 # RUN: llvm-objdump -D -j .data 6 | FileCheck --check-prefix=TEST2 %s
 
 # RUN: ld.lld -o 7 main.o 2.o --start-lib 1.o strong_data_only.o --end-lib
 # RUN: llvm-objdump -D -j .data 7 | FileCheck --check-prefix=TEST2 %s

 # RUN: ld.lld --no-fortran-common -o 11 main.o --start-lib 1.o strong_data_only.o --end-lib
 # RUN: llvm-readobj --syms 11 | FileCheck --check-prefix=NFC %s
 
 # RUN: ld.lld -o - main.o 4.a --lto-emit-asm | FileCheck --check-prefix=ASM %s
 
 # RUN: ld.lld -o - main.o  --start-lib 1.bc 2.bc --end-lib --lto-emit-asm | \
 # RUN:   FileCheck --check-prefix=ASM %s
 
+## COMMON overrides weak. Don't extract 3.bc which provides a weak definition.
+# RUN: ld.lld -o /dev/null main.o --start-lib 1.bc 3.bc --end-lib -y block | FileCheck --check-prefix=LTO_WEAK %s
+
 ## Old FORTRAN that mixes use of COMMON blocks and BLOCK DATA requires that we
 ## search through archives for non-tentative definitions (from the BLOCK DATA)
 ## to replace the tentative definitions (from the COMMON block(s)).
 
 ## Ensure we have used the initialized definition of 'block' instead of a
 ## common definition.
 # TEST1-LABEL:  Disassembly of section .data:
 # TEST1:          <block>:
 # TEST1-NEXT:       ea 2e 44 54
 # TEST1-NEXT:       fb 21 09 40
 # TEST1-NEXT:       ...
 
+# BSS:       [[#%x,]] g     O .bss   0000000000000028 block
 
 # NFC:       Name: block
 # NFC-NEXT:  Value:
 # NFC-NEXT:  Size: 40
 # NFC-NEXT:  Binding: Global (0x1)
 # NFC-NEXT:  Type: Object (0x1)
 # NFC-NEXT:  Other: 0
 # NFC-NEXT:  Section: .bss

 # MAP:       28 8 3.a(2.o):(.data)
 # MAP-NEXT:  28 1 block
 
 # ASM:         .type   block,@object
 # ASM:       block:
 # ASM-NEXT:    .long 5
 # ASM:         .size   block, 20
 
+# LTO_WEAK:     1.bc: common definition of block
+# LTO_WEAK:     <internal>: reference to block
+# LTO_WEAK-NOT: {{.}}
+
 #--- ref.s
   .text
   .abiversion 2
   .global bar
   .type bar,@function
 bar:
   addis 4, 2, block@toc@ha
   addi  4, 4, block@toc@l

 
 
 #--- blockdata.ll
 target datalayout = "e-m:e-i64:64-n32:64-S128-v256:256:256-v512:512:512"
 target triple = "powerpc64le-unknown-linux-gnu"
 
 @block = dso_local local_unnamed_addr global [5 x i32] [i32 5, i32 0, i32 0, i32 0, i32 0], align 4
 
+#--- weak.ll
+target datalayout = "e-m:e-i64:64-n32:64-S128-v256:256:256-v512:512:512"
+target triple = "powerpc64le-unknown-linux-gnu"
+
+@block = weak dso_local global [5 x i32] [i32 5, i32 0, i32 0, i32 0, i32 0], align 4
+
 #--- commonblock.ll
 target datalayout = "e-m:e-i64:64-n32:64-S128-v256:256:256-v512:512:512"
 target triple = "powerpc64le-unknown-linux-gnu"
 
 @block =  common dso_local local_unnamed_addr global [5 x i32] zeroinitializer, align 4
 
 define dso_local i32 @bar(i32 signext %i) local_unnamed_addr {
 entry:
   %idxprom = sext i32 %i to i64
   %arrayidx = getelementptr inbounds [5 x i32], [5 x i32]* @block, i64 0, i64 %idxprom
   %0 = load i32, i32* %arrayidx, align 8
   ret i32 %0
 }