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

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

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

================
Comment at: ELF/OutputSections.cpp:566
@@ -565,1 +565,3 @@
 
+static unsigned getVerDefNum() {
+  // At least one version dependency must exist. Additional version dependencies
----------------
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."

================
Comment at: ELF/OutputSections.cpp:1430
@@ +1429,3 @@
+
+template <class ELFT> void VersionDefinitionSection<ELFT>::createDefs() {
+  FileDefNameOff = Out<ELFT>::DynStrTab->addString(Config->OutputFile);
----------------
Other sections finalize() functions add strings to .dymstr. Is there any reason you didn't choose to do this in finalize()?

================
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;
----------------
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.

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

================
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];
----------------
Use range-based for loop.

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


http://reviews.llvm.org/D21018