[LLD][ELF] Error if _GLOBAL_OFFSET_TABLE_ is defined in input objects

[peter.smith]

Summary

The _GLOBAL_OFFSET_TABLE_ is a linker defined symbol that is placed at some location relative to the .got, .got.plt or .toc section. On some targets such as Arm the correctness of some code sequences using a relocation to _GLOBAL_OFFSET_TABLE_ depend on the value of the symbol being in the linker defined place. Follow the ld.gold example and give a multiple symbol definition error. The ld.bfd behaviour is to ignore the definition in the input object and redefine it, which seems like it could be more surprising.

I've restricted the effect of the change to _GLOBAL_OFFSET_TABLE_ rather than making it universal for all linker defined symbols as there are some existing tests that explicitly redefine some linker defined symbols in input objects citing some previous problem. My justification for special casing _GLOBAL_OFFSET_TABLE_ is that it is the only symbol that the linker uses the value of internally, the use of other linker defined symbols is defined by the input objects only.

A test program to check the behaviour of ld.gold and ld.bfd using the symbol names in addReservedSymbols() showed that ld.bfd would just redefine the symbols and ignore the input definitions. ld.gold would error if there was a non-common definition of the symbol.

fixes pr39587 (turning incorrect output into an error message)

[grimar]

Change looks reasonable. Have a suggestion about the code though.

ELF/Writer.cpp

222 ↗

(On Diff #174348)

Maybe just inline the code here?

StringRef GotTableSymName =
    (Config->EMachine == EM_PPC64) ? ".TOC." : "_GLOBAL_OFFSET_TABLE_";
if (Symtab->find(GotTableSymName))
  ElfSym::GlobalOffsetTable = cast<Defined>(
      Symtab->addDefined(GotTableSymName, Out::ElfHeader,
                         Target->GotBaseSymOff, STV_HIDDEN, STB_GLOBAL));

[grimar]

ELF/Writer.cpp

222 ↗

(On Diff #174348)

I think we can avoid doing cast<Defined> too. Posted D54627 for that.

[peter.smith]

I've updated the patch to inline the call for _GLOBAL_OFFSET_TABLE_ so that addOptionalRegular remains unchanged. The cast to defined can be removed if/when D54627 lands.

[grimar]

LGTM.

[peter.smith]

Updated to remove now unnecessary cast and to make sure REQUIRES has a colon at the end. Reposting in case Rui has any comments.

[grimar]

ELF/Writer.cpp

220 ↗

(On Diff #175439)

You do not need the bracers around the right side of the expression it seems:

ElfSym::GlobalOffsetTable = Symtab->addDefined(
     GotTableSymName, STV_HIDDEN, STT_NOTYPE, Target->GotBaseSymOff,
     /*Size=*/0, STB_GLOBAL, Out::ElfHeader,
     /*File=*/nullptr);

[grimar]

ELF/Writer.cpp

220 ↗

(On Diff #175439)

bracers -> parenthesis

[peter.smith]

Thanks, superfluous parentheses removed.

[ruiu]

ELF/Writer.cpp

219–220 ↗

(On Diff #175441)

Calling find before addDefined seems a bit odd to me. Doesn't this print out a "duplicate symbol" error that may be confusing to users? Perhaps it is better to print out a more concrete warning message like "you cannot redefine linker-reserved symbol: _GLOBAL_OFFSET_TABLE_" if Symtab->find() returns a defined symbol?

[peter.smith]

Thanks for the comments. I've added a specific error message for when _GLOBAL_OFFSET_TABLE_ is already defined.

[grimar]

ELF/Writer.cpp

222 ↗

(On Diff #175648)

I think you should use the GotTableSymName here? (for EM_PPC64 case)

Also, we often print the location first,
so I would suggest printing something like:

toString(S->File) + ": cannot redefine linker defined symbol '" + GotTableSymName + "'";

[peter.smith]

Thanks for the comment. I've made the suggested change.

[grimar]

I have no more comments, thanks!

[ruiu]

LGTM

lld/trunk/ELF/Writer.cpp

void elf::addReservedSymbols() {
   }
 
   // The Power Architecture 64-bit v2 ABI defines a TableOfContents (TOC) which
   // combines the typical ELF GOT with the small data sections. It commonly
   // includes .got .toc .sdata .sbss. The .TOC. symbol replaces both
   // _GLOBAL_OFFSET_TABLE_ and _SDA_BASE_ from the 32-bit ABI. It is used to
   // represent the TOC base which is offset by 0x8000 bytes from the start of
   // the .got section.
-  ElfSym::GlobalOffsetTable = addOptionalRegular(
-      (Config->EMachine == EM_PPC64) ? ".TOC." : "_GLOBAL_OFFSET_TABLE_",
-      Out::ElfHeader, Target->GotBaseSymOff);
+  // We do not allow _GLOBAL_OFFSET_TABLE_ to be defined by input objects as the
+  // correctness of some relocations depends on its value.
+  StringRef GotTableSymName =
+      (Config->EMachine == EM_PPC64) ? ".TOC." : "_GLOBAL_OFFSET_TABLE_";
+  if (Symbol *S = Symtab->find(GotTableSymName)) {
+    if (S->isDefined())
+      error(toString(S->File) + " cannot redefine linker defined symbol '" +
+            GotTableSymName + "'");
+    else
+      ElfSym::GlobalOffsetTable = Symtab->addDefined(
+          GotTableSymName, STV_HIDDEN, STT_NOTYPE, Target->GotBaseSymOff,
+          /*Size=*/0, STB_GLOBAL, Out::ElfHeader,
+          /*File=*/nullptr);
+  }
 
   // __ehdr_start is the location of ELF file headers. Note that we define
   // this symbol unconditionally even when using a linker script, which
   // differs from the behavior implemented by GNU linker which only define
   // this symbol if ELF headers are in the memory mapped segment.
   addOptionalRegular("__ehdr_start", Out::ElfHeader, 0, STV_HIDDEN);
 
   // __executable_start is not documented, but the expectation of at

lld/trunk/test/ELF/global-offset-table-position-redef-err.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+# RUN: not ld.lld -shared %t.o -o %t.so 2>&1 | FileCheck %s
+
+# On some targets the location of the _GLOBAL_OFFSET_TABLE_ symbol table can
+# matter for the correctness of some relocations. Follow the example of ld.gold
+# and give a multiple definition error if input objects attempt to redefine it.
+
+# CHECK: ld.lld: error: {{.*o}} cannot redefine linker defined symbol '_GLOBAL_OFFSET_TABLE_'
+
+.data
+.global _GLOBAL_OFFSET_TABLE_
+_GLOBAL_OFFSET_TABLE_:
+.word 0