[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

974

Inline this function.

980

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

974

Done.

980

Added, thanks for comment !

[ruiu]

ELF/Driver.cpp

1017

Add symbols -> Add dummy symbols

1023

What is !Cmd->Provide for?

1024

Is there any reason to create symbols as STB_WEAK?

[grimar]

• Updated comment.

ELF/Driver.cpp

1017

Done.

1023

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.

1024

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

[ruiu]

ELF/Driver.cpp

1024

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

1024

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

794–798

GP -> Gp

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

795–798

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

802–803

GPD -> GpDisp

813–814

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

795–798

!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 (InputFile *F : Files)
     Symtab->addFile<ELFT>(F);
 
   // 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);
 
+  // Add dummy 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.)
+  for (BaseCommand *Base : Script->Opt.Commands)
+    if (SymbolAssignment *Cmd = dyn_cast<SymbolAssignment>(Base))
+      if (Cmd->Name != "." && !Symtab->find(Cmd->Name) && !Cmd->Provide)
+        Symtab->addAbsolute<ELFT>(Cmd->Name, STV_DEFAULT);
+
   // 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)

ELF/Writer.cpp

 // the linking result. This function defines such symbols.
 template <class ELFT> void Writer<ELFT>::addReservedSymbols() {
   if (Config->EMachine == EM_MIPS) {
     // Define _gp for MIPS. st_value of _gp symbol will be updated by Writer
     // so that it points to an absolute address which by default is relative
     // to GOT. Default offset is 0x7ff0.
     // See "Global Data Symbols" in Chapter 6 in the following document:
     // ftp://www.linux-mips.org/pub/linux/mips/doc/ABI/mipsabi.pdf
+    SymbolBody *Gp = Symtab->find("_gp");
+    if (!Gp || !isa<DefinedRegular>(Gp))
       ElfSym::MipsGp = Symtab->addAbsolute<ELFT>("_gp", STV_HIDDEN, STB_LOCAL);
+    else
+      ElfSym::MipsGp = dyn_cast<DefinedRegular>(Gp);
 
     // On MIPS O32 ABI, _gp_disp is a magic symbol designates offset between
     // start of function and 'gp' pointer into GOT.
-    if (Symtab->find("_gp_disp"))
+    SymbolBody *GpDisp = Symtab->find("_gp_disp");
+    if (GpDisp && !isa<DefinedRegular>(GpDisp))
       ElfSym::MipsGpDisp =
           Symtab->addAbsolute<ELFT>("_gp_disp", STV_HIDDEN, STB_LOCAL);
+    else
+      ElfSym::MipsGpDisp = dyn_cast_or_null<DefinedRegular>(GpDisp);
 
     // The __gnu_local_gp is a magic symbol equal to the current value of 'gp'
     // pointer. This symbol is used in the code generated by .cpload pseudo-op
     // in case of using -mno-shared option.
     // https://sourceware.org/ml/binutils/2004-12/msg00094.html
-    if (Symtab->find("__gnu_local_gp"))
+    SymbolBody *LocalGp = Symtab->find("__gnu_local_gp");
+    if (LocalGp && !isa<DefinedRegular>(LocalGp))
       ElfSym::MipsLocalGp =
           Symtab->addAbsolute<ELFT>("__gnu_local_gp", STV_HIDDEN, STB_LOCAL);
+    else
+      ElfSym::MipsLocalGp = dyn_cast_or_null<DefinedRegular>(LocalGp);
   }
 
   // The _GLOBAL_OFFSET_TABLE_ symbol is defined by target convention to
   // be at some offset from the base of the .got section, usually 0 or the end
   // of the .got
   InputSection *GotSection = InX::MipsGot ? cast<InputSection>(InX::MipsGot)
                                           : cast<InputSection>(InX::Got);
   ElfSym::GlobalOffsetTable = addOptionalRegular<ELFT>(

test/ELF/linkerscript/mips-symbols-assign.s

+# REQUIRES: mips
+# RUN: llvm-mc -filetype=obj -triple=mips-unknown-linux %s -o %t.o
+# RUN: echo "SECTIONS {                              \
+# RUN:         _gp = ABSOLUTE(.) + 0x100;            \
+# RUN:         __gnu_local_gp = ABSOLUTE(.) + 0x200; \
+# RUN:         _gp_disp = ABSOLUTE(.) + 0x300; }"> %t.script
+# RUN: ld.lld -shared -o %t.so --script %t.script %t.o
+# RUN: llvm-readobj -symbols %t.so | FileCheck %s
+
+## FIXME: _gp_disp and __gnu_local_gp should be global
+## and have default visibility in case below.
+# CHECK:      Symbol {
+# CHECK:        Name: __gnu_local_gp
+# CHECK-NEXT:   Value: 0x200
+# CHECK-NEXT:   Size: 0
+# CHECK-NEXT:   Binding: Local
+# CHECK-NEXT:   Type: None
+# CHECK-NEXT:   Other [
+# CHECK-NEXT:     STV_HIDDEN
+# CHECK-NEXT:   ]
+# CHECK-NEXT:   Section: Absolute
+# CHECK-NEXT: }
+# CHECK:      Symbol {
+# CHECK:        Name: _gp_disp
+# CHECK-NEXT:   Value: 0x300
+# CHECK-NEXT:   Size: 0
+# CHECK-NEXT:   Binding: Local
+# CHECK-NEXT:   Type: None
+# CHECK-NEXT:   Other [
+# CHECK-NEXT:     STV_HIDDEN
+# CHECK-NEXT:   ]
+# CHECK-NEXT:   Section: Absolute
+# CHECK-NEXT: }
+# CHECK:      Symbol {
+# CHECK:        Name: _gp
+# CHECK-NEXT:   Value: 0x100
+# CHECK-NEXT:   Size: 0
+# CHECK-NEXT:   Binding: Global
+# CHECK-NEXT:   Type: None
+# CHECK-NEXT:   Other: 0
+# CHECK-NEXT:   Section: Absolute
+# CHECK-NEXT: }
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux -o %t.o < /dev/null
+# RUN: ld.lld -shared -o %t.so --script %t.script %t.o
+# RUN: llvm-readobj -symbols %t.so | FileCheck %s -check-prefix=SCRIPT
+
+# SCRIPT:      Symbol {
+# SCRIPT:        Name: _gp
+# SCRIPT-NEXT:   Value: 0x100
+# SCRIPT-NEXT:   Size: 0
+# SCRIPT-NEXT:   Binding: Global
+# SCRIPT-NEXT:   Type: None
+# SCRIPT-NEXT:   Other: 0
+# SCRIPT-NEXT:   Section: Absolute
+# SCRIPT-NEXT: }
+# SCRIPT:      Symbol {
+# SCRIPT:        Name: __gnu_local_gp
+# SCRIPT-NEXT:   Value: 0x200
+# SCRIPT-NEXT:   Size: 0
+# SCRIPT-NEXT:   Binding: Global
+# SCRIPT-NEXT:   Type: None
+# SCRIPT-NEXT:   Other: 0
+# SCRIPT-NEXT:   Section: Absolute
+# SCRIPT-NEXT: }
+# SCRIPT:      Symbol {
+# SCRIPT:        Name: _gp_disp
+# SCRIPT-NEXT:   Value: 0x300
+# SCRIPT-NEXT:   Size: 0
+# SCRIPT-NEXT:   Binding: Global
+# SCRIPT-NEXT:   Type: None
+# SCRIPT-NEXT:   Other: 0
+# SCRIPT-NEXT:   Section: Absolute
+# SCRIPT-NEXT: }
+
+.text
+.globl  __start
+__start:
+  lui    $t0,%hi(__gnu_local_gp)
+  addi   $t0,$t0,%lo(__gnu_local_gp)
+  lui    $t0,%hi(_gp_disp)
+  addi   $t0,$t0,%lo(_gp_disp)

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:

test/ELF/mips-gp-ext.s

 # RUN:          _gp = 0x200; \
 # RUN:          .got  : { *(.got) } }" > %t.abs.script
 # RUN: ld.lld -shared -o %t.abs.so --script %t.abs.script %t.o
 # RUN: llvm-objdump -s -t %t.abs.so | FileCheck --check-prefix=ABS %s
 
 # REQUIRES: mips
 
 # REL:      Contents of section .text:
-# REL-NEXT:  0000 3c080000 2108010c 8f82fffc
+# REL-NEXT:  0000 3c080000 2108010c 8f82001c
 #                 ^-- %hi(_gp_disp)
 #                          ^-- %lo(_gp_disp)
-#                                   ^-- 8 - (0x10c - 0x100)
+#                                   ^-- 8 - (0x10c - 0x120)
 #                                       G - (GP - .got)
 
 # REL:      Contents of section .reginfo:
 # REL-NEXT:  0028 10000104 00000000 00000000 00000000
 # REL-NEXT:  0038 00000000 0000010c
 #                          ^-- _gp
 
 # REL:      Contents of section .data:
-# REL-NEXT:  00f0 fffffef4
+# REL-NEXT:  0110 fffffef4
 #                 ^-- 0-0x10c
 
 # REL: 00000000         .text           00000000 foo
 # REL: 00000000         *ABS*           00000000 .hidden _gp_disp
-# REL: 0000010c         *ABS*           00000000 .hidden _gp
+# REL: 0000010c         *ABS*           00000000  _gp
 
 # ABS:      Contents of section .text:
-# ABS-NEXT:  0000 3c080000 21080200 8f82ff08
+# ABS-NEXT:  0000 3c080000 21080200 8f82ff28
 #                 ^-- %hi(_gp_disp)
 #                          ^-- %lo(_gp_disp)
-#                                   ^-- 8 - (0x200 - 0x100)
+#                                   ^-- 8 - (0x200 - 0x120)
 #                                       G - (GP - .got)
 
 # ABS:      Contents of section .reginfo:
 # ABS-NEXT:  0028 10000104 00000000 00000000 00000000
 # ABS-NEXT:  0038 00000000 00000200
 #                          ^-- _gp
 
 # ABS:      Contents of section .data:
-# ABS-NEXT:  00f0 fffffe00
+# ABS-NEXT:  0110 fffffe00
 #                 ^-- 0-0x200
 
 # ABS: 00000000         .text           00000000 foo
 # ABS: 00000000         *ABS*           00000000 .hidden _gp_disp
-# ABS: 00000200         *ABS*           00000000 .hidden _gp
+# ABS: 00000200         *ABS*           00000000  _gp
 
   .text
 foo:
   lui    $t0, %hi(_gp_disp)
   addi   $t0, $t0, %lo(_gp_disp)
   lw     $v0, %call16(bar)($gp)
 
   .data
   .gpword foo