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

[Peter Collingbourne via llvm-commits]

pcc added a subscriber: pcc.

================
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++};
----------------
Can we rename our implementation of the ELF hash function to something like `hashElf`? I previously wasn't aware that we had one.

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

================
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:
----------------
Grammar nitpick: "to the number of definitions"

================
Comment at: ELF/OutputSections.cpp:1457
@@ +1456,3 @@
+
+  for (std::pair<Version *, VerDefEntry> &VD : VerDefs) {
+    VerDefEntry& Def = VD.second;
----------------
It isn't deterministic to iterate over a DenseMap like that.

================
Comment at: ELF/OutputSections.cpp:1532
@@ +1531,3 @@
+  // for VER_NDX_LOCAL and VER_NDX_GLOBAL.
+  // First identifiers are reserved by verdef section if it is exist.
+  NextIndex = 2;
----------------
Grammar nitpick: "if it exists"

================
Comment at: ELF/OutputSections.cpp:1535
@@ -1440,1 +1534,3 @@
+  if (!Config->SymbolVersions.empty())
+    NextIndex = std::max(getVerDefNum() + 1, NextIndex);
 }
----------------
The std::max seems redundant because getVerDefNum() cannot return a value less than 2 here.

================
Comment at: ELF/OutputSections.h:245-248
@@ +244,6 @@
+  struct VerDefEntry {
+    uint32_t Flags; // Version definition flags.
+    uint32_t Hash;  // Name hash.
+    size_t NameOff; // Offset in string table.
+    size_t Ndx;     // Version definition index.
+  };
----------------
I don't think you need either of the flags or hash fields here. The first one is never anything but zero in your implementation, and you can compute the second one as you produce the contents of the .gnu.version_d section.

================
Comment at: ELF/OutputSections.h:251
@@ +250,3 @@
+
+  llvm::DenseMap<Version *, VerDefEntry> VerDefs;
+
----------------
Instead of having this DenseMap, could you move VerDefEntry's fields to Version?

================
Comment at: ELF/OutputSections.h:262
@@ -233,2 +261,3 @@
+
 // The .gnu.version section specifies the required version of each symbol in the
 // dynamic symbol table. It contains one Elf_Versym for each dynamic symbol
----------------
Please update this comment to cover version definitions.

================
Comment at: ELF/Symbols.h:142
@@ +141,3 @@
+  // This is reference to version definition for symbol.
+  Version* VerDef = nullptr;
+
----------------
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?


http://reviews.llvm.org/D21018