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

George Rimar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jun 15 04:49:58 PDT 2017

grimar added inline comments.

Comment at: ELF/SymbolTable.cpp:719-721
+  if (!Body->isInCurrentDSO())
+    return false;
+  return Body->getName().find("@@") != StringRef::npos;
ruiu wrote:
> You can do
>   return B->isInCurrentDSO() && B->getName().find("@@") != StringRef::npos;
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.

Comment at: ELF/SymbolTable.cpp:733
+  std::vector<DefinedRegular *> DefaultV;
+  for (Symbol *Sym : SymVector) {
+    if (isDefaultVersion(Sym->body()))
ruiu wrote:
> You can do
>   for (size_t I = 0; I < SymVector.size(); ++I)
> to remove `DefaultV`, no?
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.

Comment at: ELF/SymbolTable.cpp:734
+  for (Symbol *Sym : SymVector) {
+    if (isDefaultVersion(Sym->body()))
+      DefaultV.push_back(cast<DefinedRegular>(Sym->body()));
ruiu wrote:
> What is the cost of doing this for each symbol?
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.

Comment at: ELF/Symbols.cpp:259
   // It is an error if the specified version is not defined.
-  error(toString(File) + ": symbol " + S + " has undefined version " + Verstr);
+  if (Config->Shared)
+    error(toString(File) + ": symbol " + S + " has undefined version " +
ruiu wrote:
> Why did you add this?
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.

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
.globl foo2

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:


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

.global _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.


More information about the llvm-commits mailing list