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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


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


================
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:
> > > 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?
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 ?


https://reviews.llvm.org/D33680