[ELF] - Do not fail when set versions for linkerscript's symbol aliases

[grimar]

Summary

This is PR34121,

previously we would fail on following:

% cat sym.c
void f() {}

% cat sym.script
g = f;
VERSION { V1 { global: f; g; local: *; }; }

% clang -fPIC -c sym.c
% ld.lld -shared -o libsym.so sym.o sym.script --no-undefined-version
ld.lld: error: version script assignment of 'V1' to symbol 'g' failed: symbol not defined

That happened because at the point of proccessing symbol versions, symbol g was
not yet created. Patch changes logic to create undefined symbols referenced by script
earlier, so we are able to assing version correctly.

[ruiu]

It doesn't seem correct. Essentially, the fundamental issue is that we add versions to symbols too early, so moving it upwards in the driver doesn't feel right.

[grimar]

In D36579#838308, @ruiu wrote:

It doesn't seem correct. Essentially, the fundamental issue is that we add versions to symbols too early, so moving it upwards in the driver doesn't feel right.

I am not sure I got your point right. My patch does not move code that adds versions upwards.

What it do is moves proccessing of symbols referenced by linkerscript script, so that at the moment of scaning version script, symbols
touched in linkerscript become available for versioning.

So it works like if I would scan over script commands and adds undefined symbol for each symbol assigned by
linkerscript's assignment expression. That allows to version that undefined symbol, which is replaced by
defined regular when later script is proccessed.

[smeenai]

@grimar, I believe @ruiu's point was that the real issue is that LLD versions symbols too early, so moving other things upwards in the driver (in this case, the processing of symbols defined in a linker script) to work around the versioning being too early doesn't feel right.

[grimar]

Problem here is that we can not version symbols too late either, as we looks need to do that before
running LTO to allow internalization and versioning to work. In that sence versioning happens probably
at right place now. I'll update diff with a better version soon.

[grimar]

• Reimplemented in a different way.

[grimar]

Ping.

[ruiu]

It doesn't feel right to add a new member to Symbol just for this. Can you add a DefinedRegular symbol with some dummy value instead of an Undefined symbol, no?

[grimar]

In D36579#861514, @ruiu wrote:

It doesn't feel right to add a new member to Symbol just for this. Can you add a DefinedRegular symbol with some dummy value instead of an Undefined symbol, no?

Yes, you right. In update (going to post in a minute) I used addAbsolute because it has less arguments than addRegular and used weak binding for these symbols to
avoid "duplicate symbol: _gp" error in ELF/mips-gp-ext.s. Linkersctipt's addRegular replaces binding with STB_GLOBAL for these symbols anyways.

[grimar]

• Addressed comments.

[ruiu]

ELF/Driver.cpp

950

Inline this function.

956

Add symbols defined in linker scripts early because otherwise version
scripts referring script-synthesized symbols wouldn't work due to undefined
symbol error. (Version scripts are processed earlier than linker scripts.)

[grimar]

• Addressed review comments.

ELF/Driver.cpp

950

Done.

956

Added, thanks for comment !

[ruiu]

ELF/Driver.cpp

988

Add symbols -> Add dummy symbols

994

What is !Cmd->Provide for?

995

Is there any reason to create symbols as STB_WEAK?

[grimar]

• Updated comment.

ELF/Driver.cpp

988

Done.

994

symbols.s has next testcase:
(testcase was innaccurate before today, I fixed it in rL312779).

# The symbol is not referenced. Don't provide it.
# RUN: echo "SECTIONS { PROVIDE(newsym = 1);}" > %t.script
# RUN: ld.lld -o %t1 --script %t.script %t
# RUN: llvm-objdump -t %t1 | FileCheck --check-prefix=DONTPROVIDE %s
# DONTPROVIDE-NOT: newsym

If we have no !Cmd->Provide condition here it will fail,
because we should not provide newsym always, but only when it is referenced.

995

Yes, please refer to my comment above:
https://reviews.llvm.org/D36579#861990

[ruiu]

ELF/Driver.cpp

995

I don't think adding symbols as weak symbols is a right thing to do, if the problem is just for _gp. It implies that _gp's handling is not right.

[grimar]

• Addressed review comments.

ELF/Driver.cpp

995

I changed how _gp and other special symbols affected are created, added testcase.

[grimar]

I also posted alternative version of that here: D37731

[atanasyan]

MIPS part is LGTM.

It's interesting that _gp_disp and __gnu_local_gp have incorrect binding and visibility for so long time. I will fix that later.

[ruiu]

ELF/Writer.cpp

800 ↗

(On Diff #114771)

GP -> Gp

(You can see below that MipsGp is named MipsGp and not MipsGP.)

801–804 ↗

(On Diff #114771)

I don't get the meaning of this part. Gp doesn't seem to be guaranteed to be a DefinedRegular.

808 ↗

(On Diff #114771)

GPD -> GpDisp

819 ↗

(On Diff #114771)

LGP -> LocalGp

[grimar]

In D36579#868106, @atanasyan wrote:

MIPS part is LGTM.

It's interesting that _gp_disp and __gnu_local_gp have incorrect binding and visibility for so long time. I will fix that later.

Thanks for looking. It only happens for _gp_disp and __gnu_local_gp if them are assigned from script to something, I guess that is just a rare case.
Also I think that other special symbols are also affected. I did not yet check, but I think that for example __ehdr_start, __executable_start, __dso_handle
will remain hidden even if be assigned from script.

[grimar]

• Addressed review comments.

ELF/Writer.cpp

801–804 ↗

(On Diff #114771)

!GP->isInCurrentDSO() condition handled all other possible cases I believe except DefinedCommon, which looks
possible and I missed it.
Fixed.

[atanasyan]

In D36579#869328, @grimar wrote:

In D36579#868106, @atanasyan wrote:

MIPS part is LGTM.

It's interesting that _gp_disp and __gnu_local_gp have incorrect binding and visibility for so long time. I will fix that later.

Thanks for looking. It only happens for _gp_disp and __gnu_local_gp if them are assigned from script to something, I guess that is just a rare case.
Also I think that other special symbols are also affected. I did not yet check, but I think that for example __ehdr_start, __executable_start, __dso_handle
will remain hidden even if be assigned from script.

It looks like we do not have a strict requirement to put _gp_disp and __gnu_local_gp into the linked file at all. Both these symbols are "magic" symbols. The _gp_disp designates offset between start of a function and '_gp' value. I.e. its value used for relocations calculation differs from function to function. The __gnu_local_gp can be considered as just a synonym for the _gp. GNU ld/gold write both symbols into an output and LLD mimics this behavior, but as far as I know no tool uses these symbols in linked binaries.

Both _gp_disp and __gnu_local_gp are undefined symbols in input object files. So if we do nothing, we get the "undefined symbol" error. Do you know any method to suppress this error for particular symbols? We would recognize these special symbols when calculate relocations, but do not define them and do not write them into the output symbol table.

[grimar]

In D36579#869407, @atanasyan wrote:

In D36579#869328, @grimar wrote:

In D36579#868106, @atanasyan wrote:

MIPS part is LGTM.

It's interesting that _gp_disp and __gnu_local_gp have incorrect binding and visibility for so long time. I will fix that later.

Thanks for looking. It only happens for _gp_disp and __gnu_local_gp if them are assigned from script to something, I guess that is just a rare case.
Also I think that other special symbols are also affected. I did not yet check, but I think that for example __ehdr_start, __executable_start, __dso_handle
will remain hidden even if be assigned from script.

It looks like we do not have a strict requirement to put _gp_disp and __gnu_local_gp into the linked file at all. Both these symbols are "magic" symbols. The _gp_disp designates offset between start of a function and '_gp' value. I.e. its value used for relocations calculation differs from function to function. The __gnu_local_gp can be considered as just a synonym for the _gp. GNU ld/gold write both symbols into an output and LLD mimics this behavior, but as far as I know no tool uses these symbols in linked binaries.

Both _gp_disp and __gnu_local_gp are undefined symbols in input object files. So if we do nothing, we get the "undefined symbol" error. Do you know any method to suppress this error for particular symbols? We would recognize these special symbols when calculate relocations, but do not define them and do not write them into the output symbol table.

Probably it should be possible to set IsUsedInRegularObj = false for them and define as weak. That way they should not be in symtab and should not be reported as undefined.
I am not sure that is a good solution though.

[grimar]

In favor of D38239.

ELF/Driver.cpp

static void excludeLibs(opt::InputArgList &Args, ArrayRef<InputFile *> Files) {
   for (InputFile *File : Files)
     if (auto *F = dyn_cast<ArchiveFile>(File))
       if (All || Libs.count(path::filename(F->getName())))
         for (SymbolBody *Sym : F->getSymbols())
           Sym->symbol()->VersionId = VER_NDX_LOCAL;
 }
 
 // Do actual linking. Note that when this function is called,
 // all linker scripts have already been parsed.
 template <class ELFT> void LinkerDriver::link(opt::InputArgList &Args) {
   Target = getTarget();
 
   Config->MaxPageSize = getMaxPageSize(Args);
   Config->ImageBase = getImageBase(Args);
 
   // Default output filename is "a.out" by the Unix tradition.
   if (Config->OutputFile.empty())
     Config->OutputFile = "a.out";
 
   // Fail early if the output file or map file is not writable. If a user has a
   // long link, e.g. due to a large LTO link, they do not wish to run it and
   // find that it failed because there was a mistake in their command-line.
   if (auto E = tryCreateFile(Config->OutputFile))
template <class ELFT> void LinkerDriver::link(opt::InputArgList &Args) {
   for (auto *Arg : Args.filtered(OPT_trace_symbol))
     Symtab->trace(Arg->getValue());
 
   // Add all files to the symbol table. This will add almost all
   // symbols that we need to the symbol table.
   for (InputFile *F : Files)
     Symtab->addFile<ELFT>(F);
 
   // If an entry symbol is in a static archive, pull out that file now
   // to complete the symbol table. After this, no new names except a
   // few linker-synthesized ones will be added to the symbol table.
   if (Symtab->find(Config->Entry))
     Symtab->addUndefined<ELFT>(Config->Entry);
 
   // Return if there were name resolution errors.
   if (ErrorCount)
     return;
 
   // Handle the `--undefined <sym>` options.
   Symtab->scanUndefinedFlags<ELFT>();
 
   // Handle undefined symbols in DSOs.
   Symtab->scanShlibUndefined<ELFT>();
 
   // Handle the -exclude-libs option.
   if (Args.hasArg(OPT_exclude_libs))
     excludeLibs(Args, Files);
 
+  // Some symbols (such as __ehdr_start) are defined lazily only when there
+  // are undefined symbols for them, so we add these to trigger that logic.
+  // We also want to assign versions to symbols assigned by linkerscript,
+  // so have to create them early.
+  for (StringRef Sym : Script->Opt.ReferencedSymbols)
+    Symtab->addUndefined<ELFT>(Sym);
+
   // Apply version scripts.
   Symtab->scanVersionScript();
 
   // Create wrapped symbols for -wrap option.
   for (auto *Arg : Args.filtered(OPT_wrap))
     Symtab->addSymbolWrap<ELFT>(Arg->getValue());
 
   // Create alias symbols for -defsym option.
   for (std::pair<StringRef, StringRef> &Def : getDefsym(Args))
     Symtab->addSymbolAlias<ELFT>(Def.first, Def.second);
 
   Symtab->addCombinedLTOObject<ELFT>();
   if (ErrorCount)
     return;
 
-  // Some symbols (such as __ehdr_start) are defined lazily only when there
-  // are undefined symbols for them, so we add these to trigger that logic.
-  for (StringRef Sym : Script->Opt.ReferencedSymbols)
-    Symtab->addUndefined<ELFT>(Sym);
-
   // Apply symbol renames for -wrap and -defsym
   Symtab->applySymbolRenames();
 
   // Now that we have a complete list of input files.
   // Beyond this point, no new files are added.
   // Aggregate all input sections into one place.
   for (ObjFile<ELFT> *F : ObjFile<ELFT>::Instances)
     for (InputSectionBase *S : F->getSections())

ELF/ScriptParser.cpp

private:
   std::vector<StringRef> readOutputSectionPhdrs();
   InputSectionDescription *readInputSectionDescription(StringRef Tok);
   StringMatcher readFilePatterns();
   std::vector<SectionPattern> readInputSectionsList();
   InputSectionDescription *readInputSectionRules(StringRef FilePattern);
   unsigned readPhdrType();
   SortSectionPolicy readSortKind();
   SymbolAssignment *readProvideHidden(bool Provide, bool Hidden);
-  SymbolAssignment *readProvideOrAssignment(StringRef Tok);
+  SymbolAssignment *readProvideOrAssignment(StringRef Tok, bool InSec);
   void readSort();
   AssertCommand *readAssert();
   Expr readAssertExpr();
   Expr readConstant();
   Expr getPageSize();
 
   uint64_t readMemoryAssignment(StringRef, StringRef, StringRef);
   std::pair<uint32_t, uint32_t> readMemoryAttributes();
while (!atEOF()) {
     } else if (Tok == "PHDRS") {
       readPhdrs();
     } else if (Tok == "SEARCH_DIR") {
       readSearchDir();
     } else if (Tok == "SECTIONS") {
       readSections();
     } else if (Tok == "VERSION") {
       readVersion();
-    } else if (SymbolAssignment *Cmd = readProvideOrAssignment(Tok)) {
+    } else if (SymbolAssignment *Cmd = readProvideOrAssignment(Tok, false)) {
       Script->Opt.Commands.push_back(Cmd);
     } else {
       setError("unknown directive: " + Tok);
     }
   }
 }
 
 void ScriptParser::addFile(StringRef S) {
void ScriptParser::readSections() {
   // -no-rosegment is used to avoid placing read only non-executable sections in
   // their own segment. We do the same if SECTIONS command is present in linker
   // script. See comment for computeFlags().
   Config->SingleRoRx = true;
 
   expect("{");
   while (!ErrorCount && !consume("}")) {
     StringRef Tok = next();
-    BaseCommand *Cmd = readProvideOrAssignment(Tok);
+    BaseCommand *Cmd = readProvideOrAssignment(Tok, false);
     if (!Cmd) {
       if (Tok == "ASSERT")
         Cmd = readAssert();
       else
         Cmd = readOutputSectionDescription(Tok);
     }
     Script->Opt.Commands.push_back(Cmd);
   }
OutputSection *ScriptParser::readOutputSectionDescription(StringRef OutSec) {
   if (consume("ONLY_IF_RW"))
     Cmd->Constraint = ConstraintKind::ReadWrite;
   expect("{");
 
   while (!ErrorCount && !consume("}")) {
     StringRef Tok = next();
     if (Tok == ";") {
       // Empty commands are allowed. Do nothing here.
-    } else if (SymbolAssignment *Assign = readProvideOrAssignment(Tok)) {
+    } else if (SymbolAssignment *Assign = readProvideOrAssignment(Tok, true)) {
       Cmd->Commands.push_back(Assign);
     } else if (BytesDataCommand *Data = readBytesDataCommand(Tok)) {
       Cmd->Commands.push_back(Data);
     } else if (Tok == "ASSERT") {
       Cmd->Commands.push_back(readAssert());
       expect(";");
     } else if (Tok == "CONSTRUCTORS") {
       // CONSTRUCTORS is a keyword to make the linker recognize C++ ctors/dtors
SymbolAssignment *ScriptParser::readProvideHidden(bool Provide, bool Hidden) {
   SymbolAssignment *Cmd = readAssignment(next());
   Cmd->Provide = Provide;
   Cmd->Hidden = Hidden;
   expect(")");
   expect(";");
   return Cmd;
 }
 
-SymbolAssignment *ScriptParser::readProvideOrAssignment(StringRef Tok) {
+SymbolAssignment *ScriptParser::readProvideOrAssignment(StringRef Tok,
+                                                        bool InSec) {
   SymbolAssignment *Cmd = nullptr;
   if (peek() == "=" || peek() == "+=") {
+    if (!InSec && Tok != ".")
+      Script->Opt.ReferencedSymbols.push_back(Tok);
     Cmd = readAssignment(Tok);
     expect(";");
   } else if (Tok == "PROVIDE") {
     Cmd = readProvideHidden(true, false);
   } else if (Tok == "HIDDEN") {
     Cmd = readProvideHidden(false, true);
   } else if (Tok == "PROVIDE_HIDDEN") {
     Cmd = readProvideHidden(true, true);

ELF/SymbolTable.cpp

if (!DemangledSyms) {
     }
   }
   return *DemangledSyms;
 }
 
 std::vector<SymbolBody *> SymbolTable::findByVersion(SymbolVersion Ver) {
   if (Ver.IsExternCpp)
     return getDemangledSyms().lookup(Ver.Name);
-  if (SymbolBody *B = find(Ver.Name))
+  if (SymbolBody *B = find(Ver.Name)) {
     if (!B->isUndefined())
       return {B};
+    // Is symbol is undefined it can be possible it was created for
+    // handling linkerscript assign command. Such symbol will be processed
+    // later, but we still want to assign version for them here.
+    auto It = llvm::find(Script->Opt.ReferencedSymbols, Ver.Name);
+    if (It != Script->Opt.ReferencedSymbols.end())
+      return {B};
+    return {};
+  }
   return {};
 }
 
 std::vector<SymbolBody *> SymbolTable::findAllByVersion(SymbolVersion Ver) {
   std::vector<SymbolBody *> Res;
   StringMatcher M(Ver.Name);
 
   if (Ver.IsExternCpp) {

test/ELF/linkerscript/align.s

 # RUN: }" > %t3.script
 # RUN: ld.lld -o %t3 --script %t3.script %t
 # RUN: llvm-objdump -section-headers %t3 | FileCheck %s
 # RUN: llvm-objdump -t %t3 | FileCheck -check-prefix SYMBOLS %s
 
 # SYMBOLS-LABEL: SYMBOL TABLE:
 # SYMBOLS-NEXT: 0000000000000000         *UND*           00000000
 # SYMBOLS-NEXT: 0000000000014008         .text           00000000 _start
-# SYMBOLS-NEXT: 0000000000010000         *ABS*           00000000 __code_base__
 # SYMBOLS-NEXT: 0000000000001000         *ABS*           00000000 VAR
+# SYMBOLS-NEXT: 0000000000010000         *ABS*           00000000 __code_base__
 # SYMBOLS-NEXT: 0000000000011000         .bbb            00000000 __start_bbb
 # SYMBOLS-NEXT: 0000000000012000         .bbb            00000000 __end_bbb
 
 ## Check that ALIGN zero do nothing and does not crash #1.
 # RUN: echo "SECTIONS { . = ALIGN(0x123, 0); .aaa : { *(.aaa) } }" > %t.script
 # RUN: ld.lld -o %t4 --script %t.script %t
 # RUN: llvm-objdump -section-headers %t4 | FileCheck %s -check-prefix=ZERO
 

test/ELF/linkerscript/symbols-synthetic.s

 # RUN:        PROVIDE_HIDDEN(_end_sec = ADDR(.text) + SIZEOF(.text)); }" > %t.script
 # RUN: ld.lld -o %t1 --eh-frame-hdr --script %t.script %t
 # RUN: llvm-objdump -t %t1 | FileCheck --check-prefix=IN-SEC %s
 
 # SIMPLE:      0000000000000128         .foo    00000000 .hidden _end_sec
 # SIMPLE-NEXT: 0000000000000120         .foo    00000000 _begin_sec
 # SIMPLE-NEXT: 0000000000000128         *ABS*   00000000 _end_sec_abs
 # SIMPLE-NEXT: 0000000000001048         .text   00000000 _start
+# SIMPLE-NEXT: 0000000000000ee4         *ABS*   00000000 size_foo_3
 # SIMPLE-NEXT: 0000000000000120         .foo    00000000 begin_foo
 # SIMPLE-NEXT: 0000000000000128         .foo    00000000 end_foo
 # SIMPLE-NEXT: 0000000000000008         *ABS*   00000000 size_foo_1
 # SIMPLE-NEXT: 0000000000000008         *ABS*   00000000 size_foo_1_abs
 # SIMPLE-NEXT: 0000000000001000         .foo    00000000 begin_bar
 # SIMPLE-NEXT: 0000000000001004         .foo    00000000 end_bar
 # SIMPLE-NEXT: 0000000000000ee4         *ABS*   00000000 size_foo_2
-# SIMPLE-NEXT: 0000000000000ee4         *ABS*   00000000 size_foo_3
 # SIMPLE-NEXT: 0000000000001004         .eh_frame_hdr     00000000 __eh_frame_hdr_start
 # SIMPLE-NEXT: 0000000000001010         *ABS*             00000000 __eh_frame_hdr_start2
 # SIMPLE-NEXT: 0000000000001018         .eh_frame_hdr     00000000 __eh_frame_hdr_end
 # SIMPLE-NEXT: 0000000000001020         *ABS*             00000000 __eh_frame_hdr_end2
 
 # NO-SEC:       0000000000201000         .text     00000000 .hidden _begin_sec
 # NO-SEC-NEXT:  0000000000201001         .text     00000000 .hidden _end_sec
 

test/ELF/linkerscript/version-script.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
+
+# RUN: echo "bar = foo; VERSION { V { global: foo; bar; local: *; }; }" > %t.script
+# RUN: ld.lld -T %t.script -shared --no-undefined-version %t.o -o %t.so
+# RUN: llvm-readobj -V %t.so | FileCheck %s
+
+# CHECK:      Symbols [
+# CHECK-NEXT:   Symbol {
+# CHECK-NEXT:     Version: 0
+# CHECK-NEXT:     Name: @
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Symbol {
+# CHECK-NEXT:     Version: 2
+# CHECK-NEXT:     Name: foo@@V
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Symbol {
+# CHECK-NEXT:     Version: 0
+# CHECK-NEXT:     Name: und@
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Symbol {
+# CHECK-NEXT:     Version: 2
+# CHECK-NEXT:     Name: bar@@V
+# CHECK-NEXT:   }
+# CHECK-NEXT: ]
+
+# RUN: echo "bar = und; VERSION { V { global: foo; bar; local: *; }; }" > %t.script
+# RUN: not ld.lld -T %t.script -shared --no-undefined-version %t.o -o %t.so \
+# RUN:   2>&1 | FileCheck --check-prefix=ERR %s
+# ERR: symbol not found: und
+
+.global und
+
+.text
+.globl foo
+.type foo,@function
+foo: