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

George Rimar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 21 03:13:51 PDT 2017


grimar added inline comments.


================
Comment at: ELF/SymbolTable.cpp:728
+  // Let them parse and update their names to exclude version suffix.
+  for (size_t I = 0, N = SymVector.size(); I < N; ++I) {
+    SymbolBody *Body = SymVector[I]->body();
----------------
ruiu wrote:
> Now that you can use a for-each loop, no?
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.


================
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:
> grimar wrote:
> > 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.
> > >
> > >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.
> Why do you want to support that behavior?
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.


https://reviews.llvm.org/D33680





More information about the llvm-commits mailing list