This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Depricate version references.
ClosedPublic

Authored by grimar on Jun 30 2016, 1:09 AM.

Details

Summary

This is PR28358

According to
https://www.akkadia.org/drepper/dsohowto.pdf

"The fourth point, the VERS 1.0 version being referred to in the VERS 2.0 definition, is not really important in symbol versioning. It marks the predecessor relationship of the two versions and it is done to maintain the similar- ities with Solaris’ internal versioning. It does not cause any problem it might in fact be useful to a human reader so predecessors should always be mentioned."

Patch partially reverts 273423 "[ELF] - Implemented version script hierarchies.",
version references are just ignored now.

Diff Detail

Event Timeline

grimar updated this revision to Diff 62341.Jun 30 2016, 1:09 AM
grimar retitled this revision from to [ELF] - Depricate version references..
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar.
ruiu edited edge metadata.Jun 30 2016, 1:39 AM

Is this really what we want? Let's say we have two version definitions as shown below.

VER1 { global: foo; local: *; };
VER2 { global: bar; } VER1;

In this case, I think VER1's catch-all local: * hides all symbols but foo and bar from VER2. Your patch seems to change this behavior.

In D21888#471119, @ruiu wrote:

Is this really what we want? Let's say we have two version definitions as shown below.

VER1 { global: foo; local: *; };
VER2 { global: bar; } VER1;

In this case, I think VER1's catch-all local: * hides all symbols but foo and bar from VER2. Your patch seems to change this behavior.

There is no difference. Just re-checked with gold.

VER1 { global: foo; local: *; };
VER2 { global: bar; } VER1;

and

VER1 { global: foo; local: *; };
VER2 { global: bar; };

has the same output. Actually I noticed that much earlier when worked on all of that, but I was sure that hierarchies are required
for correctnes to comform specification.
But if Drepper writes that there is no point to have it then why not remove it.

ruiu added a comment.Jun 30 2016, 1:56 AM

Really? At least, gold seems to set vda_next field in a DSO if a predecessor is given. Do you mean that vda_next is ignored at runtime?

In D21888#471126, @ruiu wrote:

Really? At least, gold seems to set vda_next field in a DSO if a predecessor is given. Do you mean that vda_next is ignored at runtime?

Yes, sure, vda_next is set and that is what D21556 implemented. I mean that there is no affect on local tag, symbols versions and so on.
So on everything that can do some effect. So yes, vda_next looks to be just ignored in runtime.

ruiu added a comment.Jun 30 2016, 2:17 AM

What about non-GNU systems such as BSDs?

In D21888#471132, @ruiu wrote:

What about non-GNU systems such as BSDs?

I did not check. Generally this change is bit in opposite with documenation I saw before,
as everywhere (ex: https://docs.oracle.com/cd/E19120-01/open.solaris/819-0690/chapter6-80869/index.html) version
dependency or "predecessor" is mentioned as a part of verdef section.

So I guess we can only check to find how much is important to have for other systems.

ruiu accepted this revision.Jul 1 2016, 12:28 AM
ruiu edited edge metadata.

I still half-believe that the va_next is completely ignored, but if so, this patch makes sense. Let's land this to see if this will work.

ELF/SymbolListFile.cpp
98–99

Add a blank line before the comment. Also I'd mention that parent name is a hint for human that the runtime completely ignores.

// Each version may have a parent version. For example, "Ver2" defined as
// "Ver2 { global: foo; local: *; } Ver1;" has "Ver1" as a parent. This version
// hierarchy is, probably against your instinct, purely for human; the runtime
// doesn't care about them at all. In LLD, we simply skip the token.
This revision is now accepted and ready to land.Jul 1 2016, 12:28 AM
grimar added a comment.Jul 1 2016, 2:41 AM
In D21888#472146, @ruiu wrote:

I still half-believe that the va_next is completely ignored

I feel wierd about this. Looks like format of verdef section is half-baked.

This revision was automatically updated to reflect the committed changes.