This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by peter.smith on Nov 16 2018, 3:12 AM.

Details

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)

Diff Detail

Repository
rL LLVM

Event Timeline

peter.smith created this revision.Nov 16 2018, 3:12 AM

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 added inline comments.Nov 16 2018, 3:58 AM
ELF/Writer.cpp
222 ↗(On Diff #174348)

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

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 accepted this revision.Nov 16 2018, 7:44 AM

LGTM.

This revision is now accepted and ready to land.Nov 16 2018, 7:44 AM

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 added inline comments.Nov 27 2018, 3:07 AM
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 added inline comments.Nov 27 2018, 3:08 AM
ELF/Writer.cpp
220 ↗(On Diff #175439)

bracers -> parenthesis

Thanks, superfluous parentheses removed.

ruiu added inline comments.Nov 27 2018, 9:16 AM
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?

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

grimar added inline comments.Nov 28 2018, 4:04 AM
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 + "'";

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

grimar accepted this revision.Nov 28 2018, 6:16 AM

I have no more comments, thanks!

ruiu accepted this revision.Nov 28 2018, 8:27 AM

LGTM

This revision was automatically updated to reflect the committed changes.