This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Add support for locals list in version script.
ClosedPublic

Authored by grimar on Nov 8 2016, 6:16 AM.

Details

Summary

Found this when tried to build FreeBSD port jpeg-turbo-1.5.1 which fails to link because of next script:

LIBJPEGTURBO_8.0 {
  
  local:
    jsimd_*;
    jconst_*;
};

LIBJPEG_8.0 {
  global:
    *;
};

Previously we did not support anything except "local: *", patch changes that.

Actually GNU rules of proccessing wildcards are more complex than that (http://www.airs.com/blog/archives/300):
There are 2 iteration for wildcards, at first iteration "*" wildcards are ignored and handled at second iteration.

Since we previously decided not to implement such complex rules,
I suggest solution that is implemented in this patch. So for "local: *" case nothing changes, but if we have wildcarded locals,
they are processed before globals. That adds a chance for such scripts to work.

Diff Detail

Event Timeline

grimar updated this revision to Diff 77184.Nov 8 2016, 6:16 AM
grimar retitled this revision from to [ELF] - Add support for locals list in version script..
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar, davide, evgeny777.
ruiu added inline comments.Nov 8 2016, 1:23 PM
ELF/LinkerScript.cpp
1807

Use setError.

1810

Please take a look at other places. We usually write a loop this way to consume tokens until "}".

while (!Error && !consume("}")) {
  ...
}
ELF/SymbolTable.cpp
580–581

Do you actually need this guard?

697

You want to care about the case in which IsExternCpp is true.

grimar updated this revision to Diff 77313.Nov 9 2016, 12:59 AM
  • Addressed review comments.
ELF/LinkerScript.cpp
1807

Done.

1810

I did :) readLocal was written similar to readExtern.
Changed code to use while + peek, consume will not work here.

ELF/SymbolTable.cpp
580–581

I think so. We want to show this warning if assigning version to symbol that is already has one,
(and switch to error one day https://llvm.org/bugs/show_bug.cgi?id=28342)
also we want to show warning if symbol is both in local and global list as it is obvious error.
Actually I even thing we want to error out in latter case, that is how gold do afaik, planned to suggest that in a separate patch to keep this bit simpler for review.

697

Yes, but currently IsExternCpp is never set to true for locals.
Scripts with externs in locals will fail to parse, added testcase to demonstrate.
I suggest to implement them in a separate patch.
(as well as support for locals in anonymous versions declarations).

ruiu added inline comments.Nov 9 2016, 5:00 PM
ELF/SymbolTable.cpp
580–581

I mean, why the previous condition Sym->VersionId != Config->DefaultSymbolVersion wouldn't work?

705

Why symbol version number is of size_t?

grimar updated this revision to Diff 77458.Nov 10 2016, 2:00 AM
  • Addressed review comments.
ELF/SymbolTable.cpp
580–581

Ah, right. It is enough, no need in additional set at all.

705

Because we pass VersionDefinition::Id, which is size_t,
because we use next code:

size_t VersionId = Config->VersionDefinitions.size() + 2;
Config->VersionDefinitions.push_back({VerStr, VersionId});

Few numbers: this patch should fix several FreeBSD ports linkage fails, one of them is graphics/jpeg-turbo which blocks 5014 packages.

ruiu accepted this revision.Nov 11 2016, 11:57 AM
ruiu edited edge metadata.

LGTM

This revision is now accepted and ready to land.Nov 11 2016, 11:57 AM
This revision was automatically updated to reflect the committed changes.