[ELF] Check COMMON symbols for PROVIDE and don't redefine COMMON symbols edata/end/etext

[MaskRay]

Summary

In GNU ld, the definition precedence is: regular symbol assignment > relocatable object definition > PROVIDE symbol assignment.

GNU ld's internal linker scripts define the non-reserved (by C and C++)
edata/end/etext with PROVIDE so the relocatable object definition takes
precedence. This makes sense because int end; is valid.

We currently redefine such symbols if they are COMMON, but not if they are
regular definitions, so int end; with -fcommon is essentially a UB in ld.lld.
Fix this (also improve consistency and match GNU ld) by using the
isDefined code path for isCommon. In GNU ld, reserved identifiers like
__ehdr_start do not use PROVIDE, while we treat them all as PROVIDE, this
seems fine.

[peter.smith]

LGTM, makes sense to me.

[abrachet]

We're seeing test failures for edata-etext.s https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8821409046322626033/overview

[MaskRay]

In D120389#3341362, @abrachet wrote:

We're seeing test failures for edata-etext.s https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8821409046322626033/overview

Sorry, should be fixed by 53c5bd9da285be20325f6c63b3685865ae267ef6

I'll recommend that you don't set --dump-input=never. The link does not provide any information to act on the failure.

********************
Failed Tests (1):
  lld :: ELF/edata-etext.s


Testing Time: 8.11s
  Unsupported:  434
  Passed     : 2166
  Failed     :    1

1 warning(s) in tests

lld/ELF/LinkerScript.cpp

if (cmd->name == ".")
     return false;
 
   if (!cmd->provide)
     return true;
 
   // If a symbol was in PROVIDE(), we need to define it only
   // when it is a referenced undefined symbol.
   Symbol *b = symtab->find(cmd->name);
-  if (b && !b->isDefined())
+  if (b && !b->isDefined() && !b->isCommon())
     return true;
   return false;
 }
 
 // Called by processSymbolAssignments() to assign definitions to
 // linker-script-defined symbols.
 void LinkerScript::addSymbol(SymbolAssignment *cmd) {
   if (!shouldDefineSym(cmd))

lld/ELF/Writer.cpp

void elf::combineEhSections() {
   }
 
   llvm::erase_value(inputSections, nullptr);
 }
 
 static Defined *addOptionalRegular(StringRef name, SectionBase *sec,
                                    uint64_t val, uint8_t stOther = STV_HIDDEN) {
   Symbol *s = symtab->find(name);
-  if (!s || s->isDefined())
+  if (!s || s->isDefined() || s->isCommon())
     return nullptr;
 
   s->resolve(Defined{nullptr, StringRef(), STB_GLOBAL, stOther, STT_NOTYPE, val,
                      /*size=*/0, sec});
   return cast<Defined>(s);
 }
 
 static Defined *addAbsolute(StringRef name) {

lld/test/ELF/edata-etext.s

 # REQUIRES: x86
-# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
-# RUN: ld.lld %t.o -o %t
-# RUN: llvm-objdump -t --section-headers %t | FileCheck %s
+# RUN: rm -rf %t && split-file %s %t
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %t/a.s -o %t/a.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %t/b.s -o %t/b.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %t/c.s -o %t/c.o
+# RUN: ld.lld %t/a.o -o %t/a
+# RUN: llvm-objdump -t --section-headers %t/a | FileCheck %s
 
 ## This checks that:
 ## 1) Address of _etext is the first location after the last read-only loadable segment.
 ## 2) Address of _edata points to the end of the last non SHT_NOBITS section.
 ##    That is how gold/bfd do. At the same time specs says: "If the address of _edata is
 ##    greater than the address of _etext, the address of _end is same as the address
 ##    of _edata." (https://docs.oracle.com/cd/E53394_01/html/E54766/u-etext-3c.html).
 ## 3) Address of _end is different from _edata because of 2.

 # CHECK-NEXT:  0000000000201159 g       .text 0000000000000000 etext
 
 # RUN: ld.lld -r %t.o -o %t2
 # RUN: llvm-objdump -t %t2 | FileCheck %s --check-prefix=RELOCATABLE
 # RELOCATABLE:       0000000000000000 *UND* 0000000000000000 _edata
 # RELOCATABLE-NEXT:  0000000000000000 *UND* 0000000000000000 _end
 # RELOCATABLE-NEXT:  0000000000000000 *UND* 0000000000000000 _etext
 
+## If a relocatable object file defines non-reserved identifiers (by C and C++)
+## edata/end/etext, don't redefine them. Note: GNU ld redefines the reserved
+## _edata while we don't for simplicty.
+# RUN: ld.lld %t/b.o -o %t/b
+# RUN: llvm-objdump -t %t/b | FileCheck %s --check-prefix=CHECK2
+# RUN: ld.lld %t/c.o -o %t/c
+# RUN: llvm-objdump -t %t/c | FileCheck %s --check-prefix=CHECK2
+## PROVIDE does not redefine defined symbols, even if COMMON.
+# RUN: ld.lld %t/c.o %t/lds -o %t/c
+# RUN: llvm-objdump -t %t/c | FileCheck %s --check-prefix=CHECK2
+
+# CHECK2:       [[#%x,]] g     O .bss   0000000000000001 _edata
+# CHECK2-NEXT:  [[#%x,]] g     O .bss   0000000000000001 edata
+# CHECK2-NEXT:  [[#%x,]] g     O .bss   0000000000000001 end
+# CHECK2-NEXT:  [[#%x,]] g     O .bss   0000000000000001 etext
+
+#--- a.s
 .global _edata,_end,_etext,_start,edata,end,etext
 .text
 _start:
   nop
 .data
   .word 1
 .bss
   .align 4
   .space 6
+
+#--- b.s
+.bss
+.macro def x
+  .globl \x
+  .type \x, @object
+  \x: .byte 0
+  .size \x, 1
+.endm
+def _edata
+def edata
+def end
+def etext
+
+#--- c.s
+.comm _edata,1,1
+.comm edata,1,1
+.comm end,1,1
+.comm etext,1,1
+
+#--- lds
+SECTIONS {
+  .text : { *(.text) }
+
+  PROVIDE(etext = .);
+  PROVIDE(edata = .);
+  PROVIDE(end = .);
+}