This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by grimar on Jun 30 2016, 4:36 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar updated this revision to Diff 62350.Jun 30 2016, 4:36 AM
grimar retitled this revision from to [ELF] - Do not error out when version declaration not found when building executable..
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: emaste, grimar, llvm-commits.
grimar updated this object.Jun 30 2016, 4:40 AM

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.)

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 added inline comments.Jul 1 2016, 12:19 AM
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 added inline comments.Jul 1 2016, 2:30 AM
ELF/SymbolTable.cpp
190 ↗(On Diff #62350)

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

ruiu edited edge metadata.Jul 1 2016, 2:52 AM

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

grimar added a comment.Jul 1 2016, 2:59 AM
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 added a comment.Jul 1 2016, 3:34 AM

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 added a comment.Jul 1 2016, 3:41 AM
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 added a comment.Jul 1 2016, 3:58 AM

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 added a comment.Jul 1 2016, 4:02 AM
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 added a comment.Jul 1 2016, 4:04 AM

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 updated this revision to Diff 62486.Jul 1 2016, 5:30 AM
grimar edited edge metadata.
  • 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 added inline comments.Jul 1 2016, 11:39 PM
ELF/SymbolTable.cpp
194 ↗(On Diff #62486)

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

grimar added inline comments.Jul 4 2016, 1:22 AM
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.

davide added a subscriber: davide.Jul 4 2016, 3:06 PM
ruiu added inline comments.Jul 6 2016, 1:57 PM
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 added inline comments.Jul 6 2016, 11:36 PM
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 accepted this revision.Jul 7 2016, 10:23 AM
ruiu edited edge metadata.

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

This revision is now accepted and ready to land.Jul 7 2016, 10:23 AM
This revision was automatically updated to reflect the committed changes.