Page MenuHomePhabricator

Use symbols with the default version to resolve unversioned symbols.
Needs ReviewPublic

Authored by ruiu on Jun 9 2017, 3:07 PM.

Details

Reviewers
grimar
Summary

This patch is made based on https://reviews.llvm.org/D33680.

Symbols can contain symbol versions in themselves. If a symbol name
is in the form of "foo@bar", foo's version is "bar".

There's one special version -- the default version. If a symbol is in
the form of "foo@@bar", it is used not only to resolve "foo" in
version "bar" but also any unversioned "foo". Thus, it is called the
default.

This patch implements the behavior.

Fixes https://bugs.llvm.org/show_bug.cgi?id=33383.

Event Timeline

ruiu created this revision.Jun 9 2017, 3:07 PM
silvas added a subscriber: silvas.Jun 9 2017, 7:18 PM

If it is based on D33680 I presume it fixes https://bugs.llvm.org/show_bug.cgi?id=28414 also; would be good to mention that in the description too.

grimar edited edge metadata.Jun 9 2017, 11:31 PM

Thanks for taking a look on that ! Really.

Honestly I think D33680 is more clear for my eye. Let me do one more update for it,
I have one change in mind I did not include ealier, we can discuss which way fits better after that.

My conserns about this patch are inline.

lld/ELF/SymbolTable.cpp
32

I would inline it.

755

So you are mutating arguments here. Generally you are asking to avoid that in reviews.
And I think I agree with that strategy.
Mutating arguments really made me be confusing, please see next comment.

780

So Version can have valid 0 value it seems ?

But below you do check for 0:

if (Ver != 0)
  Sym->VersionId = IsDefault ? Ver : (Ver | VERSYM_HIDDEN);

That become visible and confusing for me mostly because of mutating arguments.
(I was a bit lost in what this method do in general).

ruiu added inline comments.Jun 12 2017, 10:18 AM
lld/ELF/SymbolTable.cpp
32

I wouldn't -- I tried both and it seems this is slightly clearner.

755

Well, I tried to use a tuple or a struct to return a value, but turned out this seems the best option I have.

780

I could fix it.

grimar added inline comments.Jun 12 2017, 10:39 AM
lld/ELF/SymbolTable.cpp
755

I have one idea in mind for D33680, I would like to show you, sorry again for delay here. I'll polish and update it tomorrow to demonstrate and we can choose which approach fits better then..

ruiu added inline comments.Jun 12 2017, 10:41 AM
lld/ELF/SymbolTable.cpp
780

Take your time. No reason to rush.