From fc0aa8424ca98da29a9c7aa15b4427d47504ba87 Mon Sep 17 00:00:00 2001
From: Fangrui Song <i@maskray.me>
Date: Wed, 23 Feb 2022 10:15:42 -0800
Subject: [PATCH] [ELF] Check COMMON symbols for PROVIDE and don't redefine
 COMMON symbols edata/end/etext

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.

Reviewed By: peter.smith

Differential Revision: https://reviews.llvm.org/D120389
---
 lld/ELF/LinkerScript.cpp   |  2 +-
 lld/ELF/Writer.cpp         |  2 +-
 lld/test/ELF/edata-etext.s | 54 +++++++++++++++++++++++++++++++++++---
 3 files changed, 53 insertions(+), 5 deletions(-)

diff --git a/lld/ELF/LinkerScript.cpp b/lld/ELF/LinkerScript.cpp
index 402fa2f4ffbf..4b80d6af6e26 100644
--- a/lld/ELF/LinkerScript.cpp
+++ b/lld/ELF/LinkerScript.cpp
@@ -203,7 +203,7 @@ static bool shouldDefineSym(SymbolAssignment *cmd) {
   // 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;
 }
diff --git a/lld/ELF/Writer.cpp b/lld/ELF/Writer.cpp
index cd43e79b8276..bf9e315ec0d2 100644
--- a/lld/ELF/Writer.cpp
+++ b/lld/ELF/Writer.cpp
@@ -165,7 +165,7 @@ void elf::combineEhSections() {
 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,
diff --git a/lld/test/ELF/edata-etext.s b/lld/test/ELF/edata-etext.s
index 673475e3e7ee..19cf2e5eb67d 100644
--- a/lld/test/ELF/edata-etext.s
+++ b/lld/test/ELF/edata-etext.s
@@ -1,7 +1,10 @@
 # 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.
@@ -32,6 +35,23 @@
 # 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:
@@ -41,3 +61,31 @@ _start:
 .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 = .);
+}
-- 
2.35.1.616.g0bdcbb4464-goog