[ELF] - Add Id field to Version struct.

[grimar]

Summary

That helps to avoid expressions like I + 2 in code
that assigns version number to symbols.

Change was suggested by Rui Ueyama.

[grimar]

• Fixed comment.

[• rafael]

LGTM

[ruiu]

LGTM

ELF/Config.h

45 ↗

(On Diff #63047)

I'd move this after Name so that Name and Id (arguments passed by user) are next to each other.

ELF/SymbolListFile.cpp

88 ↗

(On Diff #63047)

s/by/for/

[grimar]

After rebasing I had to modify that patch. I had to introduce createSymbolVersion() method,
that creates new version entry in one place. So could this change be reviewed before I commit please ?

[• rafael]

Lgtm, but define it as

Lld::createSymbolVersion

Cheers,
Rafael

[ruiu]

LGTM

ELF/SymbolListFile.cpp

85 ↗

(On Diff #63484)

I'd name this defineSymbolVersion.

ELF/SymbolTable.cpp

33 ↗

(On Diff #63484)

This should be in a .h file.

[grimar]

In D22086#480283, @rafael wrote:

Lgtm, but define it as

Lld::createSymbolVersion

Cheers,
Rafael

Ok, after moving to .h it is under lld::elf namespace.

lld/trunk/ELF/Config.h

 
 enum class BuildIdKind { None, Fnv1, Md5, Sha1, Hexstring };
 
 enum class UnresolvedPolicy { NoUndef, Error, Warn, Ignore };
 
 // This struct contains symbols version definition that
 // can be found in version script if it is used for link.
 struct Version {
-  Version(llvm::StringRef Name) : Name(Name) {}
+  Version(llvm::StringRef Name, size_t Id) : Name(Name), Id(Id) {}
   llvm::StringRef Name;
+  size_t Id;
   std::vector<llvm::StringRef> Globals;
   size_t NameOff; // Offset in string table.
 };
 
 // This struct contains the global configuration for the linker.
 // Most fields are direct mapping from the command line options
 // and such fields have the same name as the corresponding options.
 // Most fields are initialized by the driver.

lld/trunk/ELF/SymbolListFile.h

 #define LLD_ELF_SYMBOL_LIST_FILE_H
 
 #include "lld/Core/LLVM.h"
 #include "llvm/Support/MemoryBuffer.h"
 
 namespace lld {
 namespace elf {
 
+size_t defineSymbolVersion(StringRef Version);
+
 void parseDynamicList(MemoryBufferRef MB);
 void parseVersionScript(MemoryBufferRef MB);
 
 } // namespace elf
 } // namespace lld
 
 #endif

lld/trunk/ELF/SymbolListFile.cpp

public:
   void run();
 
 private:
   void parseVersion(StringRef Version);
   void parseLocal();
   void parseVersionSymbols(StringRef Version);
 };
 
+size_t elf::defineSymbolVersion(StringRef Version) {
+  // Identifiers start at 2 because 0 and 1 are reserved
+  // for VER_NDX_LOCAL and VER_NDX_GLOBAL constants.
+  size_t VersionId = Config->SymbolVersions.size() + 2;
+  Config->SymbolVersions.push_back(elf::Version(Version, VersionId));
+  return VersionId;
+}
+
 void VersionScriptParser::parseVersion(StringRef Version) {
   expect("{");
-  Config->SymbolVersions.push_back(elf::Version(Version));
+  defineSymbolVersion(Version);
   if (peek() == "global:") {
     next();
     parseVersionSymbols(Version);
   }
   if (peek() == "local:")
     parseLocal();
   else if (peek() != "}")
     parseVersionSymbols(Version);

lld/trunk/ELF/SymbolTable.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "SymbolTable.h"
 #include "Config.h"
 #include "Error.h"
 #include "LinkerScript.h"
 #include "Strings.h"
+#include "SymbolListFile.h"
 #include "Symbols.h"
 #include "llvm/Bitcode/ReaderWriter.h"
 #include "llvm/Support/StringSaver.h"
 
 using namespace llvm;
 using namespace llvm::object;
 using namespace llvm::ELF;
 
static uint16_t getVersionId(Symbol *Sym, StringRef Name) {
   // If symbol name contains '@' or '@@' we can assign its version id right
   // here. '@@' means the default version. It is usually the most recent one.
   // VERSYM_HIDDEN flag should be set for all non-default versions.
   StringRef Version = Name.drop_front(VersionBegin + 1);
   bool Default = Version.startswith("@");
   if (Default)
     Version = Version.drop_front();
 
-  size_t I = 2;
-  for (elf::Version &V : Config->SymbolVersions) {
+  for (elf::Version &V : Config->SymbolVersions)
     if (V.Name == Version)
-      return Default ? I : (I | VERSYM_HIDDEN);
-    ++I;
-  }
+      return Default ? V.Id : (V.Id | VERSYM_HIDDEN);
+
 
   // If we are not building shared and version script
   // is not specified, then it is not a error, it is
   // in common not to use script for linking executables.
   // In this case we just create new version.
   if (!Config->Shared && !Config->HasVersionScript) {
-    Config->SymbolVersions.push_back(elf::Version(Version));
-    return Default ? I : (I | VERSYM_HIDDEN);
+    size_t Id = defineSymbolVersion(Version);
+    return Default ? Id : (Id | VERSYM_HIDDEN);
   }
 
   error("symbol " + Name + " has undefined version " + Version);
   return 0;
 }
 
 // Find an existing symbol or create and insert a new one.
 template <class ELFT>
for (StringRef Name : V.Globals) {
           error("version script assignment of " + V.Name + " to symbol " +
                 Name + " failed: symbol not defined");
         continue;
       }
 
       if (B->symbol()->VersionId != VER_NDX_GLOBAL &&
           B->symbol()->VersionId != VER_NDX_LOCAL)
         warning("duplicate symbol " + Name + " in version script");
-      B->symbol()->VersionId = I + 2;
+      B->symbol()->VersionId = V.Id;
     }
   }
 
   for (size_t I = Config->SymbolVersions.size() - 1; I != (size_t)-1; --I) {
     Version &V = Config->SymbolVersions[I];
     for (StringRef Name : V.Globals) {
       if (!hasWildcard(Name))
         continue;
 
       for (SymbolBody *B : findAll(Name))
         if (B->symbol()->VersionId == VER_NDX_GLOBAL ||
             B->symbol()->VersionId == VER_NDX_LOCAL)
-          B->symbol()->VersionId = I + 2;
+          B->symbol()->VersionId = V.Id;
     }
   }
 }
 
 // Print the module names which define the notified
 // symbols provided through -y or --trace-symbol option.
 template <class ELFT> void SymbolTable<ELFT>::traceDefined() {
   for (const auto &Symbol : Config->TraceSymbol)