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

Rui Ueyama via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jun 22 15:08:33 PDT 2017


ruiu added inline comments.


================
Comment at: ELF/SymbolTable.cpp:733-734
+
+    // <name>@@<version> means symbol has 'default' version. We should use it to
+    // resolve references to <name>.
+    if (IsDefault && find(Body->getName())) {
----------------
<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>.


================
Comment at: ELF/SymbolTable.cpp:735
+    // resolve references to <name>.
+    if (IsDefault && find(Body->getName())) {
+      auto *D = cast<DefinedRegular>(Body);
----------------
Don't you want to check if a symbol returned by `find` is Undefined?


================
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 " +
----------------
grimar wrote:
> 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.
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?


https://reviews.llvm.org/D33680





More information about the llvm-commits mailing list