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

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/OutputSections.cpp:567
@@ +566,3 @@
+static unsigned getVerDefNum() {
+  // Returns the number of version definition entries. Because the first entry
+  // is for the version definition itself, it is the number of versioned symbols
----------------
Make this a function comment.

================
Comment at: ELF/OutputSections.cpp:1453
@@ +1452,3 @@
+                            uint32_t Flags, uint32_t Index, StringRef Name,
+                            unsigned StrTabOffset) {
+  Verdef->vd_version = 1;
----------------
unsigned -> size_t.

================
Comment at: ELF/OutputSections.cpp:1477
@@ +1476,3 @@
+  uint32_t I = 1;
+  writeDefinition(Verdef, Verdaux, VER_FLG_BASE, I++, getFileDefName(),
+                  FileDefNameOff);
----------------
Just pass 1 instead of I.

================
Comment at: ELF/SymbolListFile.cpp:119-121
@@ -114,1 +118,5 @@
+
+    Config->SymbolVersions.push_back(elf::Version(Version));
+    elf::Version &V = Config->SymbolVersions.back();
+    V.Globals.push_back(Cur);
     expect(";");
----------------
So you are constructing a Version object in two steps -- first by passing only `Version` parameter to the constructor and then add `Cur` to its Globals array, right? That's a bit hard to read. Is there any way to do this in one step?

================
Comment at: ELF/SymbolTable.cpp:531
@@ +530,3 @@
+  // assign version references for each symbol.
+  unsigned I = 1;
+  for (Version &V : Config->SymbolVersions) {
----------------
size_t should be more accurate type (in practice they don't differ though.)

================
Comment at: ELF/SymbolTable.cpp:533
@@ +532,3 @@
+  for (Version &V : Config->SymbolVersions) {
+    ++I;
+    for (StringRef Name : V.Globals)
----------------
Increment this at end of the loop -- that's easier to read.

================
Comment at: ELF/Symbols.h:420
@@ -427,1 +419,3 @@
+  // Version definition index.
+  uint16_t VersionId;
 
----------------
Does `unsigned VersionId : 16` save space?

================
Comment at: ELF/Writer.cpp:121-123
@@ -120,4 +120,5 @@
   SymbolTableSection<ELFT> DynSymTab(DynStrTab);
+  VersionDefinitionSection<ELFT> VerDef;
   VersionTableSection<ELFT> VerSym;
   VersionNeedSection<ELFT> VerNeed;
 
----------------
So, instead of instantiating them unconditionally, can you instantiate them only when needed?


http://reviews.llvm.org/D21018