[ELF] - Do not error out when version declaration not found when building executable.

[grimar]

Summary

When building executable usually version script is absent.
Before this patch error was shown in the case when
symbol name contained version and there was no script to match it.
That is PR28359. Instead of error out patch allows
to create new version declaration in this case and use it.
It looks like gnu linkers do the same and this
behavior seems logical and correct to me.

[emaste]

With this change and a workaround for PR 28357 I can link the FreeBSD base system without disabling symver.

(For the PR 28357 workaround I removed most of the extern "C++" entries from the version script and replaced those that are actually used by other components in the base system with their mangled names.)

[grimar]

In D21890#471347, @emaste wrote:

With this change and a workaround for PR 28357 I can link the FreeBSD base system without disabling symver.

(For the PR 28357 workaround I removed most of the extern "C++" entries from the version script and replaced those that are actually used by other components in the base system with their mangled names.)

Cool ! :) I started working on PR28357, hope to have something soon, probably tomorrow.

[ruiu]

ELF/SymbolTable.cpp

190 ↗

(On Diff #62350)

I don't see a reason to handle shared libraries in a different way than executables. It feels to me that the correct behavior is printing out the error message if a symbol contains '@' and a version script is given.

[grimar]

ELF/SymbolTable.cpp

190 ↗

(On Diff #62350)

Problem of PR28359 is that executable is linked and version script is not given.

[ruiu]

So, if a symbol contains '@' and version scripts are given, then it is an error, right?

[grimar]

In D21890#472197, @ruiu wrote:

So, if a symbol contains '@' and version scripts are given, then it is an error, right?

If we are building DSO and we have version script and we have symbol that has @,
which version name is not in the script, that is an error. The same probably logically
apply for executable case.
But if we dont have script and build executable, that is in common, so that is not error I think.

So I can expand the checks like that (pseudocode):

if (!Config->Shared && HaveNoVersionScript) {
  Config->SymbolVersions.push_back(elf::Version(Version));
  return Default ? I : (I | VERSYM_HIDDEN);
}

[ruiu]

That logic makes sense, but this is also satisfies our need, no?

if (HaveNoVersionScript) {
  Config->SymbolVersions.push_back(elf::Version(Version));
  return Default ? I : (I | VERSYM_HIDDEN);
}

[grimar]

In D21890#472210, @ruiu wrote:

That logic makes sense, but this is also satisfies our need, no?

if (HaveNoVersionScript) {
  Config->SymbolVersions.push_back(elf::Version(Version));
  return Default ? I : (I | VERSYM_HIDDEN);
}

Technically yes. I just think than when linking DSO with versioned
symbols it is assumed that you have the script. If you do not have it then
it is more probably a mistake, like you forget to specify one. This code
can hide such possible error I think.
I would be more strict here and error out such cases.

[ruiu]

I'm sorry but I'm confused with PR28359. The PR looks to me that when (1) you are creating an DSO and (2) if a version string after '@' is not defined in a linker script, then the GNU linker doesn't handle that as an error. It looks it is different from what this patch is trying to address. What am I missing here?

[grimar]

In D21890#472230, @ruiu wrote:

I'm sorry but I'm confused with PR28359. The PR looks to me that when (1) you are creating an DSO and (2) if a version string after '@' is not defined in a linker script, then the GNU linker doesn't handle that as an error. It looks it is different from what this patch is trying to address. What am I missing here?

1. PR does not create DSO, just executable.
2. Not defined in version script, because there is no script specified.

Gnu linker does not handle as error when not shared is created in this case.

[grimar]

responce.txt is:

--sysroot=/tank/emaste/obj/tank/emaste/src/freebsd-xlld/tmp
-Bstatic
-o as.full
tank/emaste/obj/tank/emaste/src/freebsd-xlld/tmp/usr/lib/crt1.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/tmp/usr/lib/crti.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/tmp/usr/lib/crtbeginT.o
-L tank/emaste/obj/tank/emaste/src/freebsd-xlld/tmp/usr/lib
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/as/app.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/as/as.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/as/atof-generic.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/as/atof-ieee.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/as/cond.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/as/depend.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/as/dw2gencfi.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/as/dwarf2dbg.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/as/ecoff.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/as/ehopt.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/as/expr.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/as/flonum-copy.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/as/flonum-konst.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/as/flonum-mult.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/as/frags.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/as/hash.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/as/input-file.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/as/input-scrub.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/as/listing.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/as/literal.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/as/macro.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/as/messages.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/as/obj-elf.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/as/output-file.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/as/read.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/as/sb.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/as/stabs.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/as/subsegs.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/as/symbols.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/as/write.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/as/tc-i386.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/libbfd/libbfd.a
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/libiberty/libiberty.a
tank/emaste/obj/tank/emaste/src/freebsd-xlld/gnu/usr.bin/binutils/libopcodes/libopcodes.a
-l gcc
-l gcc_eh
-l c
-l gcc
-l gcc_eh
tank/emaste/obj/tank/emaste/src/freebsd-xlld/tmp/usr/lib/crtend.o
tank/emaste/obj/tank/emaste/src/freebsd-xlld/tmp/usr/lib/crtn.o

[grimar]

• Added logic to error out if linking executable and met symbol with version

that was not specified in script. (If there is no script - no error for executable is produced.)

• Updated testcase.

[ruiu]

ELF/SymbolTable.cpp

194 ↗

(On Diff #62486)

Is !Config->VersionScript the same as Config->SymbolVersions.empty()?

[grimar]

ELF/SymbolTable.cpp

194 ↗

(On Diff #62486)

Not anymore. Because of next line:

Config->SymbolVersions.push_back(elf::Version(Version));

So since we can create a new version now during SymbolTable<ELFT>::insert(), it is not the same.

[ruiu]

ELF/Config.h

104 ↗

(On Diff #62486)

Rename HasVersionScript

ELF/SymbolTable.cpp

194 ↗

(On Diff #62486)

I'm not still convinced why this condition is not if (!Config->HasVersionScript). I do understand that you are just implementing the PR, but I think you need to justify the PR.

[grimar]

ELF/SymbolTable.cpp

194 ↗

(On Diff #62486)

I'm not still convinced why this condition is not if (!Config->HasVersionScript).

Because if we are linking DSO and have a symbol which contains version in name and do not have HasVersionScript,
that most probably an error, since it looks that version script was not specified by mistake.
Because linking DSO with versioned symbols without script is uncommon. It is so much uncommon that gnu linker restricts that,
so in real world we should not met that.
In opposite, it is common case for executables, that is what PR shows I believe. That is why I believe additional check of
!Config->Shared is a must here. Since we do not want and do not need to be less strict here than we should.

[ruiu]

LGTM. Please rename Config->VersionScript Config->HasVersionScript.

lld/trunk/ELF/Config.h

struct Configuration {
   bool DiscardLocals;
   bool DiscardNone;
   bool EhFrameHdr;
   bool EnableNewDtags;
   bool ExportDynamic;
   bool FatalWarnings;
   bool GcSections;
   bool GnuHash = false;
+  bool HasVersionScript = false;
   bool ICF;
   bool Mips64EL = false;
   bool NoGnuUnique;
   bool NoUndefinedVersion;
   bool Pic;
   bool Pie;
   bool PrintGcSections;
   bool Rela;

lld/trunk/ELF/Driver.cpp

void LinkerDriver::readConfigs(opt::InputArgList &Args) {
   if (auto *Arg = Args.getLastArg(OPT_dynamic_list))
     if (Optional<MemoryBufferRef> Buffer = readFile(Arg->getValue()))
       parseDynamicList(*Buffer);
 
   for (auto *Arg : Args.filtered(OPT_export_dynamic_symbol))
     Config->DynamicList.push_back(Arg->getValue());
 
   if (auto *Arg = Args.getLastArg(OPT_version_script)) {
+    Config->HasVersionScript = true;
     if (Optional<MemoryBufferRef> Buffer = readFile(Arg->getValue()))
       parseVersionScript(*Buffer);
   }
 
   for (auto *Arg : Args.filtered(OPT_trace_symbol))
     Config->TraceSymbol.insert(Arg->getValue());
 }
 

lld/trunk/ELF/SymbolTable.cpp

if (Default)
     Version = Version.drop_front();
 
   size_t I = 2;
   for (elf::Version &V : Config->SymbolVersions) {
     if (V.Name == Version)
       return Default ? I : (I | VERSYM_HIDDEN);
     ++I;
   }
+
+  // If we are not building shared and version script
+  // is not specified, then it is not a error, it is
+  // in common not to use script for linking executables.
+  // In this case we just create new version.
+  if (!Config->Shared && !Config->HasVersionScript) {
+    Config->SymbolVersions.push_back(elf::Version(Version));
+    return Default ? I : (I | VERSYM_HIDDEN);
+  }
+
   error("symbol " + Name + " has undefined version " + Version);
   return 0;
 }
 
 // Find an existing symbol or create and insert a new one.
 template <class ELFT>
 std::pair<Symbol *, bool> SymbolTable<ELFT>::insert(StringRef Name) {
   unsigned NumSyms = SymVector.size();

lld/trunk/test/ELF/verdef-executable.s

+# REQUIRES: x86
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t1
+# RUN: ld.lld %t1 -o %t
+# RUN: llvm-readobj -t -V -dyn-symbols %t | FileCheck --check-prefix=EXE %s
+
+# EXE:      Symbols [
+# EXE-NEXT:    Symbol {
+# EXE-NEXT:      Name:
+# EXE-NEXT:      Value: 0x0
+# EXE-NEXT:      Size: 0
+# EXE-NEXT:      Binding: Local
+# EXE-NEXT:      Type: None (0x0)
+# EXE-NEXT:      Other: 0
+# EXE-NEXT:      Section: Undefined (0x0)
+# EXE-NEXT:    }
+# EXE-NEXT:    Symbol {
+# EXE-NEXT:      Name: _start
+# EXE-NEXT:      Value: 0x11004
+# EXE-NEXT:      Size: 0
+# EXE-NEXT:      Binding: Global
+# EXE-NEXT:      Type: None
+# EXE-NEXT:      Other: 0
+# EXE-NEXT:      Section: .text
+# EXE-NEXT:    }
+# EXE-NEXT:    Symbol {
+# EXE-NEXT:      Name: a
+# EXE-NEXT:      Value: 0x11000
+# EXE-NEXT:      Size: 0
+# EXE-NEXT:      Binding: Global
+# EXE-NEXT:      Type: Function
+# EXE-NEXT:      Other: 0
+# EXE-NEXT:      Section: .text
+# EXE-NEXT:    }
+# EXE-NEXT:    Symbol {
+# EXE-NEXT:      Name: b
+# EXE-NEXT:      Value: 0x11002
+# EXE-NEXT:      Size: 0
+# EXE-NEXT:      Binding: Global
+# EXE-NEXT:      Type: Function
+# EXE-NEXT:      Other: 0
+# EXE-NEXT:      Section: .text
+# EXE-NEXT:    }
+# EXE-NEXT:    Symbol {
+# EXE-NEXT:      Name: b
+# EXE-NEXT:      Value: 0x11001
+# EXE-NEXT:      Size: 0
+# EXE-NEXT:      Binding: Global
+# EXE-NEXT:      Type: Function
+# EXE-NEXT:      Other: 0
+# EXE-NEXT:      Section: .text
+# EXE-NEXT:    }
+# EXE-NEXT:    Symbol {
+# EXE-NEXT:      Name: b_1
+# EXE-NEXT:      Value: 0x11001
+# EXE-NEXT:      Size: 0
+# EXE-NEXT:      Binding: Global
+# EXE-NEXT:      Type: Function
+# EXE-NEXT:      Other: 0
+# EXE-NEXT:      Section: .text
+# EXE-NEXT:    }
+# EXE-NEXT:    Symbol {
+# EXE-NEXT:      Name: b_2
+# EXE-NEXT:      Value: 0x11002
+# EXE-NEXT:      Size: 0
+# EXE-NEXT:      Binding: Global
+# EXE-NEXT:      Type: Function
+# EXE-NEXT:      Other: 0
+# EXE-NEXT:      Section: .text
+# EXE-NEXT:    }
+# EXE-NEXT:    Symbol {
+# EXE-NEXT:      Name: c
+# EXE-NEXT:      Value: 0x11003
+# EXE-NEXT:      Size: 0
+# EXE-NEXT:      Binding: Global
+# EXE-NEXT:      Type: Function
+# EXE-NEXT:      Other: 0
+# EXE-NEXT:      Section: .text
+# EXE-NEXT:    }
+# EXE-NEXT:  ]
+# EXE-NEXT: DynamicSymbols [
+# EXE-NEXT: ]
+# EXE-NEXT: Version symbols {
+# EXE-NEXT: }
+# EXE-NEXT: SHT_GNU_verdef {
+# EXE-NEXT: }
+# EXE-NEXT: SHT_GNU_verneed {
+# EXE-NEXT: }
+
+# RUN: ld.lld -pie --export-dynamic %t1 -o %t2
+# RUN: llvm-readobj -t -V -dyn-symbols %t2 | \
+# RUN:   FileCheck --check-prefix=EXEDYN %s
+
+# EXEDYN:      DynamicSymbols [
+# EXEDYN-NEXT:    Symbol {
+# EXEDYN-NEXT:      Name: @
+# EXEDYN-NEXT:      Value: 0x0
+# EXEDYN-NEXT:      Size: 0
+# EXEDYN-NEXT:      Binding: Local
+# EXEDYN-NEXT:      Type: None
+# EXEDYN-NEXT:      Other: 0
+# EXEDYN-NEXT:      Section: Undefined
+# EXEDYN-NEXT:    }
+# EXEDYN-NEXT:    Symbol {
+# EXEDYN-NEXT:      Name: _start@
+# EXEDYN-NEXT:      Value: 0x1004
+# EXEDYN-NEXT:      Size: 0
+# EXEDYN-NEXT:      Binding: Global
+# EXEDYN-NEXT:      Type: None
+# EXEDYN-NEXT:      Other: 0
+# EXEDYN-NEXT:      Section: .text
+# EXEDYN-NEXT:    }
+# EXEDYN-NEXT:    Symbol {
+# EXEDYN-NEXT:      Name: a@
+# EXEDYN-NEXT:      Value: 0x1000
+# EXEDYN-NEXT:      Size: 0
+# EXEDYN-NEXT:      Binding: Global
+# EXEDYN-NEXT:      Type: Function
+# EXEDYN-NEXT:      Other: 0
+# EXEDYN-NEXT:      Section: .text
+# EXEDYN-NEXT:    }
+# EXEDYN-NEXT:    Symbol {
+# EXEDYN-NEXT:      Name: b@@LIBSAMPLE_2.0
+# EXEDYN-NEXT:      Value: 0x1002
+# EXEDYN-NEXT:      Size: 0
+# EXEDYN-NEXT:      Binding: Global
+# EXEDYN-NEXT:      Type: Function
+# EXEDYN-NEXT:      Other: 0
+# EXEDYN-NEXT:      Section: .text
+# EXEDYN-NEXT:    }
+# EXEDYN-NEXT:    Symbol {
+# EXEDYN-NEXT:      Name: b@LIBSAMPLE_1.0
+# EXEDYN-NEXT:      Value: 0x1001
+# EXEDYN-NEXT:      Size: 0
+# EXEDYN-NEXT:      Binding: Global
+# EXEDYN-NEXT:      Type: Function
+# EXEDYN-NEXT:      Other: 0
+# EXEDYN-NEXT:      Section: .text
+# EXEDYN-NEXT:    }
+# EXEDYN-NEXT:    Symbol {
+# EXEDYN-NEXT:      Name: b_1@
+# EXEDYN-NEXT:      Value: 0x1001
+# EXEDYN-NEXT:      Size: 0
+# EXEDYN-NEXT:      Binding: Global
+# EXEDYN-NEXT:      Type: Function
+# EXEDYN-NEXT:      Other: 0
+# EXEDYN-NEXT:      Section: .text
+# EXEDYN-NEXT:    }
+# EXEDYN-NEXT:    Symbol {
+# EXEDYN-NEXT:      Name: b_2@
+# EXEDYN-NEXT:      Value: 0x1002
+# EXEDYN-NEXT:      Size: 0
+# EXEDYN-NEXT:      Binding: Global
+# EXEDYN-NEXT:      Type: Function
+# EXEDYN-NEXT:      Other: 0
+# EXEDYN-NEXT:      Section: .text
+# EXEDYN-NEXT:    }
+# EXEDYN-NEXT:    Symbol {
+# EXEDYN-NEXT:      Name: c@
+# EXEDYN-NEXT:      Value: 0x1003
+# EXEDYN-NEXT:      Size: 0
+# EXEDYN-NEXT:      Binding: Global
+# EXEDYN-NEXT:      Type: Function
+# EXEDYN-NEXT:      Other: 0
+# EXEDYN-NEXT:      Section: .text
+# EXEDYN-NEXT:    }
+# EXEDYN-NEXT:  ]
+# EXEDYN-NEXT:  Version symbols {
+# EXEDYN-NEXT:    Section Name: .gnu.version
+# EXEDYN-NEXT:    Address: 0x288
+# EXEDYN-NEXT:    Offset: 0x288
+# EXEDYN-NEXT:    Link: 1
+# EXEDYN-NEXT:    Symbols [
+# EXEDYN-NEXT:      Symbol {
+# EXEDYN-NEXT:        Version: 0
+# EXEDYN-NEXT:        Name: @
+# EXEDYN-NEXT:      }
+# EXEDYN-NEXT:      Symbol {
+# EXEDYN-NEXT:        Version: 1
+# EXEDYN-NEXT:        Name: _start@
+# EXEDYN-NEXT:      }
+# EXEDYN-NEXT:      Symbol {
+# EXEDYN-NEXT:        Version: 1
+# EXEDYN-NEXT:        Name: a@
+# EXEDYN-NEXT:      }
+# EXEDYN-NEXT:      Symbol {
+# EXEDYN-NEXT:        Version: 2
+# EXEDYN-NEXT:        Name: b@@LIBSAMPLE_2.0
+# EXEDYN-NEXT:      }
+# EXEDYN-NEXT:      Symbol {
+# EXEDYN-NEXT:        Version: 3
+# EXEDYN-NEXT:        Name: b@LIBSAMPLE_1.0
+# EXEDYN-NEXT:      }
+# EXEDYN-NEXT:      Symbol {
+# EXEDYN-NEXT:        Version: 1
+# EXEDYN-NEXT:        Name: b_1@
+# EXEDYN-NEXT:      }
+# EXEDYN-NEXT:      Symbol {
+# EXEDYN-NEXT:        Version: 1
+# EXEDYN-NEXT:        Name: b_2@
+# EXEDYN-NEXT:      }
+# EXEDYN-NEXT:      Symbol {
+# EXEDYN-NEXT:        Version: 1
+# EXEDYN-NEXT:        Name: c@
+# EXEDYN-NEXT:      }
+# EXEDYN-NEXT:    ]
+# EXEDYN-NEXT:  }
+# EXEDYN-NEXT:  SHT_GNU_verdef {
+# EXEDYN-NEXT:    Definition {
+# EXEDYN-NEXT:      Version: 1
+# EXEDYN-NEXT:      Flags: Base
+# EXEDYN-NEXT:      Index: 1
+# EXEDYN-NEXT:      Hash:
+# EXEDYN-NEXT:      Name:
+# EXEDYN-NEXT:    }
+# EXEDYN-NEXT:    Definition {
+# EXEDYN-NEXT:      Version: 1
+# EXEDYN-NEXT:      Flags: 0x0
+# EXEDYN-NEXT:      Index: 2
+# EXEDYN-NEXT:      Hash: 98456416
+# EXEDYN-NEXT:      Name: LIBSAMPLE_2.0
+# EXEDYN-NEXT:    }
+# EXEDYN-NEXT:    Definition {
+# EXEDYN-NEXT:      Version: 1
+# EXEDYN-NEXT:      Flags: 0x0
+# EXEDYN-NEXT:      Index: 3
+# EXEDYN-NEXT:      Hash: 98457184
+# EXEDYN-NEXT:      Name: LIBSAMPLE_1.0
+# EXEDYN-NEXT:    }
+# EXEDYN-NEXT:  }
+# EXEDYN-NEXT:  SHT_GNU_verneed {
+# EXEDYN-NEXT:  }
+
+## Check when linking executable that error produced if version script
+## was specified and there are symbols with version in name that
+## is absent in script.
+# RUN: echo    "VERSION { \
+# RUN:          global: foo;   \
+# RUN:          local: *; }; " > %t.script
+# RUN: not ld.lld --version-script %t.script %t1 -o %t3 2>&1 | \
+# RUN:   FileCheck -check-prefix=ERR %s
+# ERR: symbol b@LIBSAMPLE_1.0 has undefined version LIBSAMPLE_1.0
+
+b@LIBSAMPLE_1.0 = b_1
+b@@LIBSAMPLE_2.0 = b_2
+
+.globl a
+.type  a,@function
+a:
+retq
+
+.globl b_1
+.type  b_1,@function
+b_1:
+retq
+
+.globl b_2
+.type  b_2,@function
+b_2:
+retq
+
+.globl c
+.type  c,@function
+c:
+retq
+
+.globl _start
+_start:
+ nop