[ELF] - Resolve references properly when using .symver directive

[grimar]

Summary

This is PR28414.
Previously LLD was unable to link following:
(failed with undefined symbol bar)

Version script:
SOME_VERSION { global: *; };

.global _start
.global bar
.symver _start, bar@@SOME_VERSION
_start:
  jmp bar

Manual has next description:

.symver name, name2@@nodename
In this case, the symbol name must exist and be defined within the file being assembled. It is similar to name2@nodename.
The difference is name2@@nodename will also be used to resolve references to name2 by the linker
https://sourceware.org/binutils/docs/as/Symver.html

Patch implements that. If we have name@@ver symbol and name is undefined, name@@ver is used to resolve references to name.
If name is defined then multiple definition error is emited, that is consistent with what bfd do.

[grimar]

• Addressed review comments.

[ruiu]

I don't think we want to add a new function resolve for this feature. That function seems a bit too ad-hoc. You probably should resolve any symbol whose name is "foo@@bar" as "foo" from the beginning, by stripping "@@" and any characters that follows double-atsign before inserting symbols to the symbol table.

[grimar]

In D33680#772682, @ruiu wrote:

I don't think we want to add a new function resolve for this feature. That function seems a bit too ad-hoc. You probably should resolve any symbol whose name is "foo@@bar" as "foo" from the beginning, by stripping "@@" and any characters that follows double-atsign before inserting symbols to the symbol table.

I afraid I do not know how to implement your idea right now then. We add symbols at SymbolTable<ELFT>::insert(StringRef Name). There we are unable to assign symbol
versions, because we do not know if this symbol undefined or not shared may be (that means it is not a definition). At this place we have only name. And still need to parse it somehow
to assign proper version definition. This patch allows to do that in easy and clear way.

I spent not much time on this and may be missing something. Going to revisit your suggestion later.

[grimar]

• Addressed comments.

[ruiu]

What this patch does is probably correct, but it doesn't look clean. The code to do one thing scattered across SymbolTable.cpp and Symbols.cpp for no reason. Let me clean this up.

[grimar]

Going to do an update I mentioned earlier for this one tomorrow, I am sorry for delay.

[grimar]

• Updated.

I played with few variations of this patch and found combination that looks fine for me finally.
I think this way of solving initial issue is simple and short and that is why I prefer it in general.

isDefaultVersion() looks a code duplication at first look, but it is just 3 lines and
IMO makes logic be very straightforward and allows to simplify other things, what makes this patch be very short.

[grimar]

• Fixing silly last minute mistype.

[ruiu]

ELF/SymbolTable.cpp

719–721 ↗

(On Diff #102604)

You can do

return B->isInCurrentDSO() && B->getName().find("@@") != StringRef::npos;

733 ↗

(On Diff #102604)

You can do

for (size_t I = 0; I < SymVector.size(); ++I)

to remove DefaultV, no?

734 ↗

(On Diff #102604)

What is the cost of doing this for each symbol?

ELF/Symbols.cpp

259 ↗

(On Diff #102604)

Why did you add this?

[grimar]

• Addressed review comments.

ELF/SymbolTable.cpp

719–721 ↗

(On Diff #102604)

Done. FWIW at first I wanted to suggest to inline it since it is short now, but I tried to do that and found it is a bit more readable for me when it is wrapped in a method just like now.

733 ↗

(On Diff #102604)

Yes, I tried something like that too when wrote it. I just found that its looks close to
mutating arguments. When you debug a loop it is probably easier to understand logic when
it's end is static. Having something like temp vector it is not ideal, but I would prefer to have temp vector than
mutate loop bound probably.

734 ↗

(On Diff #102604)

I tried to link LLC binary with/wo this patch. Numbers were 5,790791071 seconds time elapsed (+- 0,23% ) vs 5,736353430 seconds time elapsed ( +- 0,32% ), so it looks like difference is zero or minimal because only computation error is visible.

ELF/Symbols.cpp

259 ↗

(On Diff #102604)

Explanation is below (from my earlier answer about the same change to Rafael in llvm-mails)
Looks I should have add comment here, I did that.

Why do you needed to add "if (Config->Shared)"? The patch looks good
otherwise, but I would like to understand this part.

Cheers,
Rafael

Testcase does not specify --version-script and would fail otherwise with "has undefined version error",
after doing this change we are consistent with gnu linkers which do not require version script file for such case when
building executable.

And reason for that I believe is next. Spec (https://sourceware.org/binutils/docs/as/Symver.html) says:
"Use the .symver directive to bind symbols to specific version nodes within a source file. This is only supported on ELF platforms,
and is typically used when assembling files to be linked into a shared library. There are cases where it may make sense to use this
in objects to be bound into an application itself so as to override a versioned symbol from a shared library."
(last sentence).

Example. Imagine dso.s:

.globl foo1
foo1:
  ret
.globl foo2
foo2:
  ret

And version script ver.txt: VERSION_1.0 { global : foo1; foo2; local : *; };

Build DSO:
llvm-mc -filetype=obj -triple=x86_64-pc-linux dso.s -o dso.o
ld.bfd dso.o -shared -o dso.so --version-script ver.txt

Now we want to override one of foo() in executable. Usually version script is not provided when linking executable,
so we probably should be able just to do:

(app.s)

.global zed
.symver zed, foo1@@VERSION_1.0
zed:
  .quad 0

.global _start
_start:
 call foo1
 call foo2

llvm-mc -filetype=obj -triple=x86_64-pc-linux app.s -o app.o
ld.bfd app.o dso.so -o out

And end up with overriden foo1:

17: 00000000006003b0     0 NOTYPE  GLOBAL DEFAULT   10 foo2@@VERSION_1.0
18: 00000000006003b0     0 NOTYPE  GLOBAL DEFAULT   10 _edata
19: 00000000006003b0     0 NOTYPE  GLOBAL DEFAULT   10 _end
20: 0000000000400278     0 NOTYPE  GLOBAL DEFAULT    8 foo1@@VERSION_1.0

Without "if (Config->Shared)" we would error out here.

[grimar]

• Removed temp vector according to review suggestions. I am sorry for delay.

[ruiu]

ELF/SymbolTable.cpp

728 ↗

(On Diff #102957)

Now that you can use a for-each loop, no?

ELF/Symbols.cpp

259 ↗

(On Diff #102604)

Why do you want to support that behavior?

[grimar]

• Addressed review comments.

ELF/SymbolTable.cpp

728 ↗

(On Diff #102957)

Ah yes. Honestly I found your comment very confusing at first. Because I used this loop assuming that SymVector may change in addRegular(). That can not happen because of find(Body->getName()) check below.

I found that even in diff where temp vector was introduced (102014), it was useless, that happened
because initially when I wrote patch I did not find() symbols, but added regulars always,
then found it is wrong (we should resolve only undefined ones) and added a check
later, before posting. I did not realize that check removes need of vector that time.

And all time after that I was still keeping in mind that code may modify SymVector in loop, but it was a mistake.

ELF/Symbols.cpp

259 ↗

(On Diff #102604)

I think it is consistent with spec (please see my comment from Thu, Jun 15, 2:49 PM),
and consistent with gnu linkers, so why not to do that ?

Otherwise for testcase we will need to specify --version-script. That still works for initial PR,
but also uncommon in general for building executables I think.

[ruiu]

ELF/SymbolTable.cpp

733–734 ↗

(On Diff #103334)

<name>@@<version> means the symbol is the default version. If that's the case,
the symbol is not used only to resolve <name> of version<version> but also unversioned
symbols with name <name>.

735 ↗

(On Diff #103334)

Don't you want to check if a symbol returned by find is Undefined?

ELF/Symbols.cpp

259 ↗

(On Diff #102604)

I don't know if this logic is correct. At least it looks odd. Shouldn't it be a check if a version script was given?

[grimar]

• Addressed review comments.

ELF/SymbolTable.cpp

735 ↗

(On Diff #103334)

No, I think we should do this check.

First testcase (version-script-symver-err.s) currently fails with "error: duplicate symbol: bar".
If I would resolve only undefined symbols, it would not fail,
though this behavior is consistent with GNU linkers and looks fine for me. I do not think spec says anything about we should try to resolve only undefined symbols.

ELF/Symbols.cpp

259 ↗

(On Diff #102604)

As I mentioned, looks spec allows that. I am not sure how much this case is common though.

Original PR I am trying to address here has version script in command line,
Because of that I suggest to focus on case revealed by PR. I updated testcases to have version scripts and removed this check. This fixes PR and if later we will have complains from somebody, we will be able to put this check back again.
What do you think ?

[grimar]

• Addressed review comments/suggestions.

[ruiu]

ELF/SymbolTable.cpp

742–743 ↗

(On Diff #104381)

Old doesn't sound like a good name as it is not an old symbol. Since the scope of this variable is very small, just S would be enough.

745–746 ↗

(On Diff #104381)

I added Symbol::copyBody, so please use that function.

[grimar]

• Addressed review comments.

ELF/SymbolTable.cpp

742–743 ↗

(On Diff #104381)

Done.

745–746 ↗

(On Diff #104381)

Done.

[ruiu]

LGTM

[grimar]

• Reopening reverted patch with fix for FreeBSD failture testcase provided by Ed Maste.

[ruiu]

LGTM

[orivej]

lld incorrectly resolves versioned symbols to zeros when they are in a static library. Here is an example that should loop, but it crashes: https://gist.github.com/orivej/8379fbd550022e966f77d1516e31702a

[grimar]

In D33680#801659, @orivej wrote:

lld incorrectly resolves versioned symbols to zeros when they are in a static library. Here is an example that should loop, but it crashes: https://gist.github.com/orivej/8379fbd550022e966f77d1516e31702a

D35059 fixes that too.

lld/trunk/ELF/SymbolTable.cpp

void SymbolTable<ELFT>::assignWildcardVersion(SymbolVersion Ver,
   // Exact matching takes precendence over fuzzy matching,
   // so we set a version to a symbol only if no version has been assigned
   // to the symbol. This behavior is compatible with GNU.
   for (SymbolBody *B : Syms)
     if (B->symbol()->VersionId == Config->DefaultSymbolVersion)
       B->symbol()->VersionId = VersionId;
 }
 
+static bool isDefaultVersion(SymbolBody *B) {
+  return B->isInCurrentDSO() && B->getName().find("@@") != StringRef::npos;
+}
+
 // This function processes version scripts by updating VersionId
 // member of symbols.
 template <class ELFT> void SymbolTable<ELFT>::scanVersionScript() {
   // Symbol themselves might know their versions because symbols
   // can contain versions in the form of <name>@<version>.
-  // Let them parse their names.
-  if (!Config->VersionDefinitions.empty())
-    for (Symbol *Sym : SymVector)
-      Sym->body()->parseSymbolVersion();
+  // Let them parse and update their names to exclude version suffix.
+  for (Symbol *Sym : SymVector) {
+    SymbolBody *Body = Sym->body();
+    bool IsDefault = isDefaultVersion(Body);
+    Body->parseSymbolVersion();
+
+    if (!IsDefault)
+      continue;
+
+    // <name>@@<version> means the symbol is the default version. If that's the
+    // case, the symbol is not used only to resolve <name> of version <version>
+    // but also undefined unversioned symbols with name <name>.
+    SymbolBody *S = find(Body->getName());
+    if (S && S->isUndefined())
+      S->copy(Body);
+  }
 
   // Handle edge cases first.
   handleAnonymousVersion();
 
   if (Config->VersionDefinitions.empty())
     return;
 
   // Now we have version definitions, so we need to set version ids to symbols.

lld/trunk/test/ELF/version-script-symver.s

+# REQUIRES: x86
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
+# RUN: echo "VERSION { global: *; };" > %t.map
+# RUN: ld.lld %t.o --version-script %t.map -o %t
+
+.global _start
+.global bar
+.symver _start, bar@@VERSION
+_start:
+  jmp bar