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

[MaskRay]

Summary

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.

[peter.smith]

Overall looks good to me. I've got a few suggestions on the refactoring. One other thing that might be worth doing to make this easier to review is to split it into two patches:

• A refactoring change that doesn't change the wildcard behaviour.
• The change to the wildcard behaviour.

Not got a strong opinion on that now I've gone through it.

ELF/Config.h

75 ↗

(On Diff #213238)

names is very close to name. Perhaps patterns as this is what I think a SymbolVersion represents. For example foo*. Come to think of it, SymbolVersion could be confusing as it is more like a VersionPattern.

ELF/Driver.cpp

1021 ↗

(On Diff #213238)

Would it be clearer to say [VER_NDX_LOCAL] instead of [0] and [VER_NDX_GLOBAL] instead of 1? In this file it is quite easy to see what 0 and 1 are from the push_back above, but this doesn't necessarily hold for other files.

ELF/ScriptParser.cpp

1347 ↗

(On Diff #213238)

This file is where I was thinking that versionDefinitions[VER_NDX_LOCAL].names would be clearer for 0, and versionDefinitions[VER_NDX_GLOBAL] for 1.

1367 ↗

(On Diff #213238)

Is this comment needed now that the +2 has been removed.

ELF/SymbolTable.cpp

207 ↗

(On Diff #213238)

Would prefer verdefIndex = VER_NDX_LOCAL;

ELF/SyntheticSections.cpp

2775 ↗

(On Diff #213238)

Any reason why in Sybols.cpp auto & was used instead of the const VersionDefinition & used here?

Maybe worth a named helper function that returns makeArrayRef(config->versionDefinitions).slice(2)); ? Looking at this line in isolation it is not obvious why?

ELF/Writer.cpp

399 ↗

(On Diff #213238)

A similar comment to the slice(2). It isn't obvious from the line why the > 2 is here. Not a big problem but possibly worth a comment.

[grimar]

Generally I also think it is fine.
A few comments from me (some of them seems were already mentioned by Peter).

ELF/Driver.cpp

1021 ↗

(On Diff #213238)

What about accessing here and elsewhere as config->versionDefinitions[VER_NDX_LOCAL] and config->versionDefinitions[VER_NDX_GLOBAL]? I.e. idea is to get rid of magic values + increase the readability.

ELF/ScriptParser.cpp

1348 ↗

(On Diff #213238)

During my work on LLD I was often asked to avoid using STL algorithms in favor of straightforward loops.

Perhaps I would do this here too now, for consistency with the rest of LLD code.

ELF/SyntheticSections.cpp

1166 ↗

(On Diff #213238)

, plus 1 looks a bit strange here IMO because code has -1.
I understand this is because of "-2 + 1 == -1", but would be better if comment described this explicitly probably.
(i.e. it is assumed that reader knows there are exactly 2 more (VER_NDX_LOCAL + VER_NDX_GLOBAL) definitions in this array,
but this is not obvious, probably). Though may be it just me, so feel free to ignore.

ELF/Writer.cpp

399 ↗

(On Diff #213238)

Needs a comment about what is 2.

test/ELF/version-script-reassign-glob.s

16 ↗

(On Diff #213238)

CHECK -> FOO probably for consistency.

[MaskRay]

Address review comments

Add a helper:

static inline ArrayRef<VersionDefinition> versionDefinitionsNotLocalOrGlobal() {
  return llvm::makeArrayRef(config->versionDefinitions).slice(2);
}

This turns out to simplify several places (.slice(2), .size() > 2, etc).

[MaskRay]

In D65716#1614431, @peter.smith wrote:

Overall looks good to me. I've got a few suggestions on the refactoring. One other thing that might be worth doing to make this easier to review is to split it into two patches:

• A refactoring change that doesn't change the wildcard behaviour.
• The change to the wildcard behaviour.

Not got a strong opinion on that now I've gone through it.

I starts from two patches :/ 1) merge config->versionScript{Locals,Globals} into config->versionDefinitions 2) delete defaultSymbolVersion. Then I realized just 1) would fix the ld.bfd/gold incompatibility though the code was a big uglier in readAnonymousDeclaration and readVersionDeclaration. So I think probably I should just merge 2) into 1)...

ELF/SymbolTable.cpp

207 ↗

(On Diff #213238)

Here 0 (any number other than -1 works) is an arbitrary number to indicate the version has been assigned. Added a comment about this.

This change is why we can detect { global: foo; }; and V1 { global: foo}; duplicate assignment now.

[MaskRay]

for (SymbolVersion &ver : v.patterns)
->
for (SymbolVersion &pat : v.patterns)

[MaskRay]

Address review comment: CHECK -> FOO probably for consistency.

[grimar]

Minor nit is below and I do not have any more comments atm probably, looks good to me.

ELF/Config.h

310 ↗

(On Diff #213324)

versionDefinitionsNotLocalOrGlobal looks a bit too long name.
May be namedVersionDefs or something else would be better?

[peter.smith]

Thanks for the update. I've not got any more comments.

[ruiu]

ELF/Config.h

310 ↗

(On Diff #213324)

I feel the same. How about getUserDefinedVersions? Also I feel that this function needs a comment, as the magic number 2 is not obvious.

[MaskRay]

Rename the helper from versionDefinitionsNotLocalOrGlobal to namedVersionDefs

[MaskRay]

Add a comment to namedVersionDefs

[MaskRay]

ELF/Config.h

310 ↗

(On Diff #213324)

I went with namedVersionDefs, because global and local are sometimes referred to as anonymous, so named seems a good name. Will happy to change if there is a better name.

[ruiu]

LGTM

lld/trunk/ELF/Config.h

struct SymbolVersion {
   bool isExternCpp;
   bool hasWildcard;
 };
 
 // This struct contains symbols version definition that
 // 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.
 // 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.
 struct Configuration {
   uint8_t osabi = 0;
struct Configuration {
   std::string rpath;
   std::vector<VersionDefinition> versionDefinitions;
   std::vector<llvm::StringRef> auxiliaryList;
   std::vector<llvm::StringRef> filterList;
   std::vector<llvm::StringRef> searchPaths;
   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>
       callGraphProfile;
   bool allowMultipleDefinition;
   bool allowShlibUndefined;
   bool androidPackDynRelocs;
   bool armHasBlx = false;
struct Configuration {
   OrphanHandlingPolicy orphanHandling;
   SortSectionPolicy sortSection;
   StripPolicy strip;
   UnresolvedPolicy unresolvedSymbols;
   Target2Policy target2;
   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;
   uint64_t maxPageSize;
   uint64_t mipsGotSize;
   uint64_t zStackSize;
   unsigned ltoPartitions;
   unsigned ltoo;
struct Configuration {
 
   // 4 for ELF32, 8 for ELF64.
   int wordsize;
 };
 
 // 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);
   else
     warn(msg);
 }
 } // namespace elf
 } // namespace lld
 
 #endif

lld/trunk/ELF/Driver.cpp

if (auto *arg = args.getLastArg(OPT_symbol_ordering_file)){
     if (Optional<MemoryBufferRef> buffer = readFile(arg->getValue())){
       config->symbolOrderingFile = getSymbolOrderingFile(*buffer);
       // Also need to disable CallGraphProfileSort to prevent
       // LLD order symbols with CGProfile
       config->callGraphProfileSort = false;
     }
   }
 
+  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 =
       args.hasFlag(OPT_export_dynamic, OPT_no_export_dynamic, false);
 
   // Parses -dynamic-list and -export-dynamic-symbol. They make some
   // symbols private. Note that -export-dynamic takes precedence over them
   // as it says all symbols should be exported.

lld/trunk/ELF/ScriptParser.cpp

unsigned ScriptParser::readPhdrType() {
   return ret;
 }
 
 // Reads an anonymous version declaration.
 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(";");
 }
 
 // Reads a non-anonymous version definition,
 // e.g. "VerStr { global: foo; bar; local: *; };".
 void ScriptParser::readVersionDeclaration(StringRef verStr) {
   // Read a symbol list.
   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"
   // defined as "Ver2 { global: foo; local: *; } Ver1;" has "Ver1"
   // as a parent. This version hierarchy is, probably against your
   // instinct, purely for hint; the runtime doesn't care about it
   // at all. In LLD, we simply ignore it.
   if (peek() != ";")

lld/trunk/ELF/SymbolTable.cpp

Symbol *SymbolTable::insert(StringRef name) {
   if (!isNew)
     return symVector[symIndex];
 
   Symbol *sym = reinterpret_cast<Symbol *>(make<SymbolUnion>());
   symVector.push_back(sym);
 
   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;
   sym->canInline = true;
   sym->scriptDefined = false;
   sym->partition = 1;
   return sym;
 }
if (syms.empty()) {
     return;
   }
 
   auto getName = [](uint16_t ver) -> std::string {
     if (ver == VER_NDX_LOCAL)
       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.
   for (Symbol *sym : syms) {
     // Skip symbols containing version info because symbol versions
     // specified by symbol names take precedence over version scripts.
     // See parseSymbolVersion().
     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;
 
     warn("attempt to reassign symbol '" + ver.name + "' of " +
          getName(sym->versionId) + " to " + getName(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
 // member of symbols.
 // If there's only one anonymous version definition in a version
 // script file, the script does not actually define any symbol version,
 // but just specifies symbols visibilities.
 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);
+    for (SymbolVersion &pat : v.patterns)
+      assignExactVersion(pat, v.id, v.name);
 
-  // Note that because the last match takes precedence over previous matches,
-  // we iterate over the definitions in the reverse order.
+  // 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>.
   // Let them parse and update their names to exclude version suffix.
   for (Symbol *sym : symVector)
     sym->parseSymbolVersion();
 
   // 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();
 }

lld/trunk/ELF/Symbols.cpp

if (!isDefined())
     return;
 
   // '@@' in a symbol name means the default version.
   // It is usually the most recent one.
   bool isDefault = (verstr[0] == '@');
   if (isDefault)
     verstr = verstr.substr(1);
 
-  for (VersionDefinition &ver : config->versionDefinitions) {
+  for (const VersionDefinition &ver : namedVersionDefs()) {
     if (ver.name != verstr)
       continue;
 
     if (isDefault)
       versionId = ver.id;
     else
       versionId = ver.id | VERSYM_HIDDEN;
     return;

lld/trunk/ELF/SyntheticSections.cpp

 void StringTableSection::writeTo(uint8_t *buf) {
   for (StringRef s : strings) {
     memcpy(buf, s.data(), s.size());
     buf[s.size()] = '\0';
     buf += s.size() + 1;
   }
 }
 
-// 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()
     : SyntheticSection(SHF_ALLOC | SHF_WRITE, SHT_DYNAMIC, config->wordsize,
                        ".dynamic") {
   this->entsize = ELFT::Is64Bits ? 16 : 8;
 
   // .dynamic section is not writable on MIPS and on Fuchsia OS
if (!getPartition().name.empty())
     return getPartition().name;
   if (!config->soName.empty())
     return config->soName;
   return config->outputFile;
 }
 
 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())
     getParent()->link = sec->sectionIndex;
 
   // sh_info should be set to the number of definitions. This fact is missed in
   // documentation, but confirmed by binutils community:
   // https://sourceware.org/ml/binutils/2014-11/msg00355.html
void VersionDefinitionSection::writeOne(uint8_t *buf, uint32_t index,
   write32(buf + 20, nameOff); // vda_name
   write32(buf + 24, 0);       // vda_next
 }
 
 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++);
   }
 
   // Need to terminate the last version definition.
   write32(buf + 16, 0); // vd_next
 }
 

lld/trunk/ELF/Writer.cpp

for (Partition &part : partitions) {
 
     if (config->hasDynSymTab) {
       part.dynSymTab = make<SymbolTableSection<ELFT>>(*part.dynStrTab);
       add(part.dynSymTab);
 
       part.verSym = make<VersionTableSection>();
       add(part.verSym);
 
-      if (!config->versionDefinitions.empty()) {
+      if (!namedVersionDefs().empty()) {
         part.verDef = make<VersionDefinitionSection>();
         add(part.verDef);
       }
 
       part.verNeed = make<VersionNeedSection<ELFT>>();
       add(part.verNeed);
 
       if (config->gnuHash) {

lld/trunk/test/ELF/linkerscript/version-script.s

 
 ## Check that we are able to version symbols defined in script.
 # CHECK:      Symbols [
 # CHECK-NEXT:   Symbol {
 # CHECK-NEXT:     Version: 0
 # 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 {
 # CHECK-NEXT:     Version: 2
 # CHECK-NEXT:     Name: foo@@V
 # CHECK-NEXT:   }
 # CHECK-NEXT:   Symbol {
 # CHECK-NEXT:     Version: 2

lld/trunk/test/ELF/version-script-reassign-glob.s

+# 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:

lld/trunk/test/ELF/version-script-reassign.s

 # 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 \
 # RUN:   -o %t.so 2>&1 | FileCheck --check-prefix=V1-WARN %s
 # RUN: llvm-readelf --dyn-syms %t.so | FileCheck --check-prefix=V1-SYM %s
 
 # RUN: ld.lld -shared %t.o --version-script %t1.ver --version-script %t2w.ver \
 # RUN:   -o %t.so --fatal-warnings