[PATCH] D21018: [ELF] - Basic versioned symbols support implemented.

[George Rimar via llvm-commits]

grimar marked 9 inline comments as done.
grimar added a comment.

Hi Peter ! Thanks for usefull review. I hope to update this one soon. As soon as solve unifying problem you're mentioned.


================
Comment at: ELF/OutputSections.cpp:1432
@@ +1431,3 @@
+  size_t I = 1;
+  VerDefs[nullptr] = {VER_FLG_BASE, hashSysv(Config->OutputFile),
+                      Out<ELFT>::DynStrTab->addString(Config->OutputFile), I++};
----------------
pcc wrote:
> Can we rename our implementation of the ELF hash function to something like `hashElf`? I previously wasn't aware that we had one.
I have nothing against that if Rui and Rafael also support the new naming. That can be committed separatelly.

================
Comment at: ELF/OutputSections.cpp:1437
@@ +1436,3 @@
+                   I++};
+  } // Without this and opening bracket it does not compile under VS2015.
+}
----------------
pcc wrote:
> This is a well-known problem, we probably don't need to call out every instance of it.
Ok, I did not know about that.

================
Comment at: ELF/OutputSections.cpp:1445
@@ +1444,3 @@
+
+  // sh_info should be set to amount of definitions. This fact is missed in
+  // documentation, but confirmed by binutils community:
----------------
pcc wrote:
> Grammar nitpick: "to the number of definitions"
Thanks !

================
Comment at: ELF/Symbols.h:142
@@ +141,3 @@
+  // This is reference to version definition for symbol.
+  Version* VerDef = nullptr;
+
----------------
pcc wrote:
> This field belongs on Symbol, as it is a property of a symbol name (and this won't work correctly if a bitcode file defines a versioned symbol). Can you please move this to Symbol and add a test showing that LTO works correctly with this feature?
> 
> On that point, I'm not sure that we want all 3 of SharedSymbol::VersionId, Symbol::VersionScriptGlobal and this new field. Did you think about unifying them somehow?
Probably yes. I am working on unifying.


http://reviews.llvm.org/D21018