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

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/Config.h:38
@@ +37,3 @@
+struct Version {
+  Version(llvm::StringRef Name) { this->Name = Name; }
+  llvm::StringRef Name;
----------------
ruiu wrote:
>   Version(llvm::StringRef Name) : Name(Name) {}
Done.

================
Comment at: ELF/OutputSections.cpp:566
@@ -565,1 +565,3 @@
 
+static unsigned getVerDefNum() {
+  // At least one version dependency must exist. Additional version dependencies
----------------
ruiu wrote:
> Maybe the comment is too detailed and didn't actually say what it is doing? I'd say "Returns the number of version definition entries. Because the first entry is for the version definition itself, it is the number of versioned symbols plus one. Note that we don't support multiple versions yet."
Changed.

================
Comment at: ELF/OutputSections.cpp:1430
@@ +1429,3 @@
+
+template <class ELFT> void VersionDefinitionSection<ELFT>::createDefs() {
+  FileDefNameOff = Out<ELFT>::DynStrTab->addString(Config->OutputFile);
----------------
ruiu wrote:
> Other sections finalize() functions add strings to .dymstr. Is there any reason you didn't choose to do this in finalize()?
Moved to finalize(). I think there was some problem, not really remember what exactly in very first iteration of this patch (createDefs did more work that time). Now it is redundant.

================
Comment at: ELF/OutputSections.cpp:1431
@@ +1430,3 @@
+template <class ELFT> void VersionDefinitionSection<ELFT>::createDefs() {
+  FileDefNameOff = Out<ELFT>::DynStrTab->addString(Config->OutputFile);
+  for (Version &V : Config->SymbolVersions)
----------------
pcc wrote:
> I think this should be the soname, not the output file name.
Checked that, both ld/gold use soname, and if it is not set then output file name is taken. Corrected the logic to match.

================
Comment at: ELF/OutputSections.cpp:1455
@@ +1454,3 @@
+  size_t Cnt = Config->SymbolVersions.size() + 1;
+  for (size_t I = 0; I < Cnt; ++I) {
+    bool IsBase = I == 0;
----------------
ruiu wrote:
> You are writing two different types of records in this loop -- one for the first entry and the other for the remaining entries. It is better to separate the first entry from the others. Then you can remove all the IsBase dispatches from this loop.
Done.

================
Comment at: ELF/OutputSections.cpp:1462
@@ +1461,3 @@
+    Verdef->vd_cnt = 1;
+    Verdef->vd_hash = IsBase ? hashSysv(Config->OutputFile)
+                             : hashSysv(Config->SymbolVersions[I - 1].Name);
----------------
pcc wrote:
> Same here
Fixed.

================
Comment at: ELF/OutputSections.cpp:1499
@@ -1427,5 +1498,3 @@
        Out<ELFT>::DynSymTab->getSymbols()) {
-    if (auto *SS = dyn_cast<SharedSymbol<ELFT>>(P.first))
-      OutVersym->vs_index = SS->VersionId;
-    else
-      OutVersym->vs_index = VER_NDX_GLOBAL;
+    SymbolBody *Body = P.first;
+    OutVersym->vs_index = Body->symbol()->VersionId;
----------------
pcc wrote:
> Fold this variable into its use.
Done.

================
Comment at: ELF/OutputSections.h:256
@@ -235,2 +255,3 @@
 // table entry. An Elf_Versym is just a 16-bit integer that refers to a version
-// identifier defined in the .gnu.version_r section.
+// identifier defined in the .gnu.version_d section. The values 0 and 1 are
+// reserved. All other values are used for versions in the own object or in any
----------------
pcc wrote:
> Either .gnu.version_r or .gnu.version_d.
Done.

================
Comment at: ELF/SymbolListFile.cpp:77
@@ -76,1 +76,3 @@
 
+  ~VersionScriptParser() {
+    if (!hasLocal)
----------------
ruiu wrote:
> Please rebase.
Done.

================
Comment at: ELF/SymbolTable.cpp:528
@@ +527,3 @@
+  // assign version references for each symbol.
+  for (size_t I = 0, E = Config->SymbolVersions.size(); I < E; ++I) {
+    lld::elf::Version &V = Config->SymbolVersions[I];
----------------
ruiu wrote:
> Use range-based for loop.
Done.

================
Comment at: ELF/Symbols.cpp:269
@@ -268,3 +268,3 @@
     return false;
-  return (ExportDynamic && VersionScriptGlobal) || body()->isShared() ||
-         (body()->isUndefined() && Config->Shared);
+  return (ExportDynamic && (VersionScriptGlobal || body()->symbol()->VerDef)) ||
+         body()->isShared() || (body()->isUndefined() && Config->Shared);
----------------
pcc wrote:
> This is the only place where we are using the `VerDef` and `VersionScriptGlobal` fields. Can you replace `(VersionScriptGlobal || body()->symbol()->VerDef)`  here with `VersionId != VER_NDX_LOCAL` and remove both of those fields?
> 
> (Also, `body()->symbol()->` is unnecessary here, as it just takes you back to the `Symbol` you started with.)
Done.

================
Comment at: ELF/Writer.cpp:913
@@ -908,1 +912,3 @@
+
+    if (Out<ELFT>::VerSym->isRequired())
       Add(Out<ELFT>::VerSym);
----------------
ruiu wrote:
> Instead of adding isRequired() method, don't instantiate Out<ELFT>::VerSym if not needed. It's what we usually do for other sections.
Done.


http://reviews.llvm.org/D21018