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

[George Rimar via llvm-commits]

grimar 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
----------------
ruiu wrote:
> Make this a function comment.
Done.

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

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

================
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(";");
----------------
ruiu wrote:
> 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?
Oh. There was a bug here. It created new elf::Version for each global. Though that did not affect whole functionality, it is defenitely not what I wanted.
Fixed.

================
Comment at: ELF/SymbolTable.cpp:531
@@ +530,3 @@
+  // assign version references for each symbol.
+  unsigned I = 1;
+  for (Version &V : Config->SymbolVersions) {
----------------
ruiu wrote:
> size_t should be more accurate type (in practice they don't differ though.)
Probably yes, but we also have VersionId of type uint16_t, so I am not sure how much it can be called accurate in total. Changed to size_t.

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

================
Comment at: ELF/Symbols.h:420
@@ -427,1 +419,3 @@
+  // Version definition index.
+  uint16_t VersionId;
 
----------------
ruiu wrote:
> Does `unsigned VersionId : 16` save space?
72 vs 80, yes.
Moved it under uint8_t Binding; The same result but looks a bit nicer I think ?

================
Comment at: ELF/Writer.cpp:121-123
@@ -120,4 +120,5 @@
   SymbolTableSection<ELFT> DynSymTab(DynStrTab);
+  VersionDefinitionSection<ELFT> VerDef;
   VersionTableSection<ELFT> VerSym;
   VersionNeedSection<ELFT> VerNeed;
 
----------------
ruiu wrote:
> So, instead of instantiating them unconditionally, can you instantiate them only when needed?
Ah, sorry, I misunderstood at first. Fixed for Verdef (I think because of VerNeed it is not possible to perform that change for other 2, as Out<ELFT>::VerNeed->addSymbol(SS) called after creation of sections).


http://reviews.llvm.org/D21018