[ELF] Consistently prioritize non-* wildcards overs "*" in version scripts

We prioritize non-* wildcards overs VER_NDX_LOCAL/VER_NDX_GLOBAL "*".
This patch generalizes the rule to "*" of other versions and thus fixes PR40176.
I don't feel strongly about this GNU linkers' behavior but the
generalization simplifies code.

Delete `config->defaultSymbolVersion` which was used to special case
VER_NDX_LOCAL/VER_NDX_GLOBAL "*".

In `SymbolTable::scanVersionScript`, custom versions are handled the same
way as VER_NDX_LOCAL/VER_NDX_GLOBAL. So merge
`config->versionScript{Locals,Globals}` into `config->versionDefinitions`.
Overall this seems to simplify the code.

In `SymbolTable::assign{Exact,Wildcard}Versions`,
`sym->verdefIndex == config->defaultSymbolVersion` is changed to
`verdefIndex == UINT32_C(-1)`.
This allows us to give duplicate assignment diagnostics for
`{ global: foo; };` `V1 { global: foo; };`

In test/linkerscript/version-script.s:
  vs_index of an undefined symbol changes from 0 to 1. This doesn't matter (arguably 1 is better because the binding is STB_GLOBAL) because vs_index of an undefined symbol is ignored.

Reviewed By: ruiu

Differential Revision: https://reviews.llvm.org/D65716

llvm-svn: 367869

10 files changed, 91 insertions, 70 deletions

diff --git a/lld/ELF/Config.h b/lld/ELF/Config.h
index 02124c04c63..c038b50da81 100644
--- a/lld/ELF/Config.h
+++ b/lld/ELF/Config.h
@@ -71,8 +71,8 @@ struct SymbolVersion {
 // can be found in version script if it is used for link.
 struct VersionDefinition {
   llvm::StringRef name;
-  uint16_t id = 0;
-  std::vector<SymbolVersion> globals;
+  uint16_t id;
+  std::vector<SymbolVersion> patterns;
 };
 
 // This struct contains the global configuration for the linker.
@@ -117,8 +117,6 @@ struct Configuration {
   std::vector<llvm::StringRef> symbolOrderingFile;
   std::vector<llvm::StringRef> undefined;
   std::vector<SymbolVersion> dynamicList;
-  std::vector<SymbolVersion> versionScriptGlobals;
-  std::vector<SymbolVersion> versionScriptLocals;
   std::vector<uint8_t> buildIdVector;
   llvm::MapVector<std::pair<const InputSectionBase *, const InputSectionBase *>,
                   uint64_t>
@@ -224,7 +222,6 @@ struct Configuration {
   ARMVFPArgKind armVFPArgs = ARMVFPArgKind::Default;
   BuildIdKind buildId = BuildIdKind::None;
   ELFKind ekind = ELFNoneKind;
-  uint16_t defaultSymbolVersion = llvm::ELF::VER_NDX_GLOBAL;
   uint16_t emachine = llvm::ELF::EM_NONE;
   llvm::Optional<uint64_t> imageBase;
   uint64_t commonPageSize;
@@ -310,6 +307,12 @@ struct Configuration {
 // The only instance of Configuration struct.
 extern Configuration *config;
 
+// The first two elements of versionDefinitions represent VER_NDX_LOCAL and
+// VER_NDX_GLOBAL. This helper returns other elements.
+static inline ArrayRef<VersionDefinition> namedVersionDefs() {
+  return llvm::makeArrayRef(config->versionDefinitions).slice(2);
+}
+
 static inline void errorOrWarn(const Twine &msg) {
   if (!config->noinhibitExec)
     error(msg);
diff --git a/lld/ELF/Driver.cpp b/lld/ELF/Driver.cpp
index 7ee75756a93..0f0b567a7c2 100644
--- a/lld/ELF/Driver.cpp
+++ b/lld/ELF/Driver.cpp
@@ -1011,14 +1011,20 @@ static void readConfigs(opt::InputArgList &args) {
     }
   }
 
+  assert(config->versionDefinitions.empty());
+  config->versionDefinitions.push_back({"local", (uint16_t)VER_NDX_LOCAL, {}});
+  config->versionDefinitions.push_back(
+      {"global", (uint16_t)VER_NDX_GLOBAL, {}});
+
   // If --retain-symbol-file is used, we'll keep only the symbols listed in
   // the file and discard all others.
   if (auto *arg = args.getLastArg(OPT_retain_symbols_file)) {
-    config->defaultSymbolVersion = VER_NDX_LOCAL;
+    config->versionDefinitions[VER_NDX_LOCAL].patterns.push_back(
+        {"*", /*isExternCpp=*/false, /*hasWildcard=*/true});
     if (Optional<MemoryBufferRef> buffer = readFile(arg->getValue()))
       for (StringRef s : args::getLines(*buffer))
-        config->versionScriptGlobals.push_back(
-            {s, /*IsExternCpp*/ false, /*HasWildcard*/ false});
+        config->versionDefinitions[VER_NDX_GLOBAL].patterns.push_back(
+            {s, /*isExternCpp=*/false, /*hasWildcard=*/false});
   }
 
   bool hasExportDynamic =
diff --git a/lld/ELF/ScriptParser.cpp b/lld/ELF/ScriptParser.cpp
index 8f0aa660145..40c993df550 100644
--- a/lld/ELF/ScriptParser.cpp
+++ b/lld/ELF/ScriptParser.cpp
@@ -1344,16 +1344,10 @@ void ScriptParser::readAnonymousDeclaration() {
   std::vector<SymbolVersion> locals;
   std::vector<SymbolVersion> globals;
   std::tie(locals, globals) = readSymbols();
-
-  for (SymbolVersion v : locals) {
-    if (v.name == "*")
-      config->defaultSymbolVersion = VER_NDX_LOCAL;
-    else
-      config->versionScriptLocals.push_back(v);
-  }
-
-  for (SymbolVersion v : globals)
-    config->versionScriptGlobals.push_back(v);
+  for (const SymbolVersion &pat : locals)
+    config->versionDefinitions[VER_NDX_LOCAL].patterns.push_back(pat);
+  for (const SymbolVersion &pat : globals)
+    config->versionDefinitions[VER_NDX_GLOBAL].patterns.push_back(pat);
 
   expect(";");
 }
@@ -1365,22 +1359,14 @@ void ScriptParser::readVersionDeclaration(StringRef verStr) {
   std::vector<SymbolVersion> locals;
   std::vector<SymbolVersion> globals;
   std::tie(locals, globals) = readSymbols();
-
-  for (SymbolVersion v : locals) {
-    if (v.name == "*")
-      config->defaultSymbolVersion = VER_NDX_LOCAL;
-    else
-      config->versionScriptLocals.push_back(v);
-  }
+  for (const SymbolVersion &pat : locals)
+    config->versionDefinitions[VER_NDX_LOCAL].patterns.push_back(pat);
 
   // Create a new version definition and add that to the global symbols.
   VersionDefinition ver;
   ver.name = verStr;
-  ver.globals = globals;
-
-  // User-defined version number starts from 2 because 0 and 1 are
-  // reserved for VER_NDX_LOCAL and VER_NDX_GLOBAL, respectively.
-  ver.id = config->versionDefinitions.size() + 2;
+  ver.patterns = globals;
+  ver.id = config->versionDefinitions.size();
   config->versionDefinitions.push_back(ver);
 
   // Each version may have a parent version. For example, "Ver2"
diff --git a/lld/ELF/SymbolTable.cpp b/lld/ELF/SymbolTable.cpp
index 3faeed8c2bd..9463aeeba99 100644
--- a/lld/ELF/SymbolTable.cpp
+++ b/lld/ELF/SymbolTable.cpp
@@ -73,7 +73,7 @@ Symbol *SymbolTable::insert(StringRef name) {
 
   sym->setName(name);
   sym->symbolKind = Symbol::PlaceholderKind;
-  sym->versionId = config->defaultSymbolVersion;
+  sym->versionId = VER_NDX_GLOBAL;
   sym->visibility = STV_DEFAULT;
   sym->isUsedInRegularObj = false;
   sym->exportDynamic = false;
@@ -192,7 +192,7 @@ void SymbolTable::assignExactVersion(SymbolVersion ver, uint16_t versionId,
       return "VER_NDX_LOCAL";
     if (ver == VER_NDX_GLOBAL)
       return "VER_NDX_GLOBAL";
-    return ("version '" + config->versionDefinitions[ver - 2].name + "'").str();
+    return ("version '" + config->versionDefinitions[ver].name + "'").str();
   };
 
   // Assign the version.
@@ -203,8 +203,12 @@ void SymbolTable::assignExactVersion(SymbolVersion ver, uint16_t versionId,
     if (sym->getName().contains('@'))
       continue;
 
-    if (sym->versionId == config->defaultSymbolVersion)
+    // If the version has not been assigned, verdefIndex is -1. Use an arbitrary
+    // number (0) to indicate the version has been assigned.
+    if (sym->verdefIndex == UINT32_C(-1)) {
+      sym->verdefIndex = 0;
       sym->versionId = versionId;
+    }
     if (sym->versionId == versionId)
       continue;
 
@@ -214,15 +218,14 @@ void SymbolTable::assignExactVersion(SymbolVersion ver, uint16_t versionId,
 }
 
 void SymbolTable::assignWildcardVersion(SymbolVersion ver, uint16_t versionId) {
-  if (!ver.hasWildcard)
-    return;
-
   // Exact matching takes precendence over fuzzy matching,
   // so we set a version to a symbol only if no version has been assigned
   // to the symbol. This behavior is compatible with GNU.
-  for (Symbol *b : findAllByVersion(ver))
-    if (b->versionId == config->defaultSymbolVersion)
-      b->versionId = versionId;
+  for (Symbol *sym : findAllByVersion(ver))
+    if (sym->verdefIndex == UINT32_C(-1)) {
+      sym->verdefIndex = 0;
+      sym->versionId = versionId;
+    }
 }
 
 // This function processes version scripts by updating the versionId
@@ -233,26 +236,24 @@ void SymbolTable::assignWildcardVersion(SymbolVersion ver, uint16_t versionId) {
 void SymbolTable::scanVersionScript() {
   // First, we assign versions to exact matching symbols,
   // i.e. version definitions not containing any glob meta-characters.
-  for (SymbolVersion &ver : config->versionScriptGlobals)
-    assignExactVersion(ver, VER_NDX_GLOBAL, "global");
-  for (SymbolVersion &ver : config->versionScriptLocals)
-    assignExactVersion(ver, VER_NDX_LOCAL, "local");
   for (VersionDefinition &v : config->versionDefinitions)
-    for (SymbolVersion &ver : v.globals)
-      assignExactVersion(ver, v.id, v.name);
-
-  // Next, we assign versions to fuzzy matching symbols,
-  // i.e. version definitions containing glob meta-characters.
-  for (SymbolVersion &ver : config->versionScriptGlobals)
-    assignWildcardVersion(ver, VER_NDX_GLOBAL);
-  for (SymbolVersion &ver : config->versionScriptLocals)
-    assignWildcardVersion(ver, VER_NDX_LOCAL);
-
-  // Note that because the last match takes precedence over previous matches,
-  // we iterate over the definitions in the reverse order.
+    for (SymbolVersion &pat : v.patterns)
+      assignExactVersion(pat, v.id, v.name);
+
+  // Next, assign versions to wildcards that are not "*". Note that because the
+  // last match takes precedence over previous matches, we iterate over the
+  // definitions in the reverse order.
   for (VersionDefinition &v : llvm::reverse(config->versionDefinitions))
-    for (SymbolVersion &ver : v.globals)
-      assignWildcardVersion(ver, v.id);
+    for (SymbolVersion &pat : v.patterns)
+      if (pat.hasWildcard && pat.name != "*")
+        assignWildcardVersion(pat, v.id);
+
+  // Then, assign versions to "*". In GNU linkers they have lower priority than
+  // other wildcards.
+  for (VersionDefinition &v : config->versionDefinitions)
+    for (SymbolVersion &pat : v.patterns)
+      if (pat.hasWildcard && pat.name == "*")
+        assignWildcardVersion(pat, v.id);
 
   // Symbol themselves might know their versions because symbols
   // can contain versions in the form of <name>@<version>.
@@ -262,7 +263,7 @@ void SymbolTable::scanVersionScript() {
 
   // isPreemptible is false at this point. To correctly compute the binding of a
   // Defined (which is used by includeInDynsym()), we need to know if it is
-  // VER_NDX_LOCAL or not. If defaultSymbolVersion is VER_NDX_LOCAL, we should
-  // compute symbol versions before handling --dynamic-list.
+  // VER_NDX_LOCAL or not. Compute symbol versions before handling
+  // --dynamic-list.
   handleDynamicList();
 }
diff --git a/lld/ELF/Symbols.cpp b/lld/ELF/Symbols.cpp
index a9bc613b9a8..092eb32247e 100644
--- a/lld/ELF/Symbols.cpp
+++ b/lld/ELF/Symbols.cpp
@@ -227,7 +227,7 @@ void Symbol::parseSymbolVersion() {
   if (isDefault)
     verstr = verstr.substr(1);
 
-  for (VersionDefinition &ver : config->versionDefinitions) {
+  for (const VersionDefinition &ver : namedVersionDefs()) {
     if (ver.name != verstr)
       continue;
 
diff --git a/lld/ELF/SyntheticSections.cpp b/lld/ELF/SyntheticSections.cpp
index 84c498f00f4..eef9d8d8df2 100644
--- a/lld/ELF/SyntheticSections.cpp
+++ b/lld/ELF/SyntheticSections.cpp
@@ -1162,10 +1162,12 @@ void StringTableSection::writeTo(uint8_t *buf) {
   }
 }
 
-// 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.
-static unsigned getVerDefNum() { return config->versionDefinitions.size() + 1; }
+// Returns the number of entries in .gnu.version_d: the number of
+// non-VER_NDX_LOCAL-non-VER_NDX_GLOBAL definitions, plus 1.
+// Note that we don't support vd_cnt > 1 yet.
+static unsigned getVerDefNum() {
+  return namedVersionDefs().size() + 1;
+}
 
 template <class ELFT>
 DynamicSection<ELFT>::DynamicSection()
@@ -2771,7 +2773,7 @@ StringRef VersionDefinitionSection::getFileDefName() {
 
 void VersionDefinitionSection::finalizeContents() {
   fileDefNameOff = getPartition().dynStrTab->addString(getFileDefName());
-  for (VersionDefinition &v : config->versionDefinitions)
+  for (const VersionDefinition &v : namedVersionDefs())
     verDefNameOffs.push_back(getPartition().dynStrTab->addString(v.name));
 
   if (OutputSection *sec = getPartition().dynStrTab->getParent())
@@ -2805,7 +2807,7 @@ void VersionDefinitionSection::writeTo(uint8_t *buf) {
   writeOne(buf, 1, getFileDefName(), fileDefNameOff);
 
   auto nameOffIt = verDefNameOffs.begin();
-  for (VersionDefinition &v : config->versionDefinitions) {
+  for (const VersionDefinition &v : namedVersionDefs()) {
     buf += EntrySize;
     writeOne(buf, v.id, v.name, *nameOffIt++);
   }
diff --git a/lld/ELF/Writer.cpp b/lld/ELF/Writer.cpp
index a911b3e0ff3..a359b9379e4 100644
--- a/lld/ELF/Writer.cpp
+++ b/lld/ELF/Writer.cpp
@@ -396,7 +396,7 @@ template <class ELFT> static void createSyntheticSections() {
       part.verSym = make<VersionTableSection>();
       add(part.verSym);
 
-      if (!config->versionDefinitions.empty()) {
+      if (!namedVersionDefs().empty()) {
         part.verDef = make<VersionDefinitionSection>();
         add(part.verDef);
       }
diff --git a/lld/test/ELF/linkerscript/version-script.s b/lld/test/ELF/linkerscript/version-script.s
index 67a0fd68ca7..d751fbf04ab 100644
--- a/lld/test/ELF/linkerscript/version-script.s
+++ b/lld/test/ELF/linkerscript/version-script.s
@@ -17,7 +17,7 @@
 # CHECK-NEXT:     Name:
 # CHECK-NEXT:   }
 # CHECK-NEXT:   Symbol {
-# CHECK-NEXT:     Version: 0
+# CHECK-NEXT:     Version: 1
 # CHECK-NEXT:     Name: und
 # CHECK-NEXT:   }
 # CHECK-NEXT:   Symbol {
diff --git a/lld/test/ELF/version-script-reassign-glob.s b/lld/test/ELF/version-script-reassign-glob.s
new file mode 100644
index 00000000000..e25cdacae89
--- /dev/null
+++ b/lld/test/ELF/version-script-reassign-glob.s
@@ -0,0 +1,19 @@
+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
+
+# RUN: echo 'foo { foo*; }; bar { *; };' > %t.ver
+# RUN: ld.lld --version-script %t.ver %t.o -shared -o %t.so --fatal-warnings
+# RUN: llvm-readelf --dyn-syms %t.so | FileCheck --check-prefix=FOO %s
+
+# RUN: echo 'foo { foo*; }; bar { f*; };' > %t.ver
+# RUN: ld.lld --version-script %t.ver %t.o -shared -o %t.so --fatal-warnings
+# RUN: llvm-readelf --dyn-syms %t.so | FileCheck --check-prefix=BAR %s
+
+## If both a non-* glob and a * match, non-* wins.
+## This is GNU linkers' behavior. We don't feel strongly this should be supported.
+# FOO: GLOBAL DEFAULT 7 foo@@foo
+
+# BAR: GLOBAL DEFAULT 7 foo@@bar
+
+.globl foo
+foo:
diff --git a/lld/test/ELF/version-script-reassign.s b/lld/test/ELF/version-script-reassign.s
index 62b32cba82b..2ed5b15face 100644
--- a/lld/test/ELF/version-script-reassign.s
+++ b/lld/test/ELF/version-script-reassign.s
@@ -1,18 +1,22 @@
 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
+# RUN: echo '{ global: foo; };' > %tg.ver
 # RUN: echo '{ local: foo; };' > %tl.ver
-# RUN: echo '{ global: foo; local: *; };' > %tg.ver
+# RUN: echo '{ global: foo; local: *; };' > %tgl.ver
 # RUN: echo 'V1 { global: foo; };' > %t1.ver
 # RUN: echo 'V2 { global: foo; };' > %t2.ver
 # RUN: echo 'V2 { global: notexist; local: f*; };' > %t2w.ver
 
-## Note, ld.bfd errors on the two cases.
+## Note, ld.bfd errors on these cases.
 # RUN: ld.lld -shared %t.o --version-script %tl.ver --version-script %t1.ver \
 # RUN:   -o %t.so 2>&1 | FileCheck --check-prefix=LOCAL %s
 # RUN: llvm-readelf --dyn-syms %t.so | FileCheck --check-prefix=LOCAL-SYM %s
 # RUN: ld.lld -shared %t.o --version-script %tg.ver --version-script %t1.ver \
 # RUN:   -o %t.so 2>&1 | FileCheck --check-prefix=GLOBAL %s
 # RUN: llvm-readelf --dyn-syms %t.so | FileCheck --check-prefix=GLOBAL-SYM %s
+# RUN: ld.lld -shared %t.o --version-script %tgl.ver --version-script %t1.ver \
+# RUN:   -o %t.so 2>&1 | FileCheck --check-prefix=GLOBAL %s
+# RUN: llvm-readelf --dyn-syms %t.so | FileCheck --check-prefix=GLOBAL-SYM %s
 
 ## Note, ld.bfd silently accepts this case.
 # RUN: ld.lld -shared %t.o --version-script %t1.ver --version-script %t2.ver \