[ELF] Apply version script patterns to non-default version symbols

[MaskRay]

Summary

Currently version script patterns are ignored for .symver produced
non-default version (single @) symbols. This makes such symbols
not localizable by local:, e.g.

.symver foo3_v1,foo3@v1
.globl foo_v1
foo3_v1:

ld.lld --version-script=a.ver -shared a.o
# In a.out, foo3@v1 is incorrectly exported.

This patch adds the support:

• Move config->versionDefinitions[VER_NDX_LOCAL].patterns to config->versionDefinitions[versionId].localPatterns
• Rename config->versionDefinitions[versionId].patterns to config->versionDefinitions[versionId].nonLocalPatterns
• Allow findAllByVersion to find non-default version symbols when includeNonDefault is true. (Note: symtab keys do not have @@)
• Make each pattern check both the unversioned pat.name and the versioned ${pat.name}@${v.name}
• localPatterns can localize ${pat.name}@${v.name}. nonLocalPatterns can prevent localization by assigning verdefIndex (before parseSymbolVersion).

---

If a user notices new undefined symbol errors with a version script containing
local: *;, the issue is likely due to a missing global: pattern.

[peter.smith]

I struggled a bit to understand the implementation, probably not helped by the complexity of symbol version scripts. I can agree with what you are trying to achieve though. Do we need any more tests? For example the error message.

lld/ELF/Config.h

89–90

Is it worth renaming this to nonLocalPatterns (globalPatterns might work, but may not be strictly accurate).

lld/ELF/SymbolTable.cpp

189–191

Suggest adding
// return false if no symbol definition matches ver.

250

I'm struggling a bit to understand this part. IIUC if we have a script like:

v1 { global: foo; }

then we'll call

assignExactVersion({"foo@v1", /*pat.isExternCpp*/ false, /*hasWildcard*/ false}, v1 id, "v1");

However in assignExactVersion we have:

// Get a list of symbols which we need to assign the version to.
  std::vector<Symbol *> syms = findByVersion(ver);

Which I'd expect to return symbols matching "foo@v1";
but later on in assignExactVersion there is:

if (versionId != VER_NDX_LOCAL && sym->getName().contains('@'))
      continue;

which I'd assume foo@v1 would always end up in continue.

If I'm right, the only affect of calling assignExactVersion here is to update found

251

Would be useful to have /* hasWildcard */ false)

259

The body of the loop is almost identical, may be worth extracting into a lambda or a function? Just a mild suggestion, difficult to know if it will be an improvement or not.

260

Would be useful to have /* hasWildcard */ false)

270

/* checkAt */ false

272

/* hasWildcard */ true

273

/* checkAt */ true

lld/ELF/SymbolTable.h

68–69

Instead of "checkAt" I suggest "includeNonDefault" or "includeNonDefaultVer".

74–75

Same as above, I suggest "includeNonDefault"

[MaskRay]

Address comments.

Added a diagnostic test to version-script-noundef.s
The coverage should be quite good now.
version-script-symver.s & version-script-symver2.s check various complex cases.

verneed-* and other version-script-* cover misc ad-hoc cases.
(I was struggling with reorganizing them.)

lld/ELF/SymbolTable.cpp

250

Updating found is one use.

The other use is to allow a global: to override local: *. A symbol's version is unassigned when verdexIndex == UINT32_C(-1).
If a global: pattern assigns verdexIndex, a subsequent local: pattern won't be able to localize the symbol.

These tests would fail if I remove the assignExactVersion({"foo@v1" function call:

Failed Tests (5):
  lld :: ELF/partition-synthetic-sections.s
  lld :: ELF/verdef-defaultver.s
  lld :: ELF/verneed.s
  lld :: ELF/version-script-symver2.s
  lld :: ELF/version-symbol-undef.s

273

I picked /*checkAt=*/true.

clang-format will delete space before true if it recognizes this form.

clang-tidy -checks='-*,bugprone-argument-comment' recognizes the = form and can suggest argument renames.

[MaskRay]

Add extern "C++" tests

[peter.smith]

Thanks for the update LGTM.

lld/ELF/Config.h

struct SymbolVersion {
   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;
-  std::vector<SymbolVersion> patterns;
+  std::vector<SymbolVersion> nonLocalPatterns;
+  std::vector<SymbolVersion> localPatterns;
 };
 
 // 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;

lld/ELF/Driver.cpp

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, {}});
+      {"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->versionDefinitions[VER_NDX_LOCAL].patterns.push_back(
+    config->versionDefinitions[VER_NDX_LOCAL].nonLocalPatterns.push_back(
         {"*", /*isExternCpp=*/false, /*hasWildcard=*/true});
     if (Optional<MemoryBufferRef> buffer = readFile(arg->getValue()))
       for (StringRef s : args::getLines(*buffer))
-        config->versionDefinitions[VER_NDX_GLOBAL].patterns.push_back(
+        config->versionDefinitions[VER_NDX_GLOBAL].nonLocalPatterns.push_back(
             {s, /*isExternCpp=*/false, /*hasWildcard=*/false});
   }
 
   for (opt::Arg *arg : args.filtered(OPT_warn_backrefs_exclude)) {
     StringRef pattern(arg->getValue());
     if (Expected<GlobPattern> pat = GlobPattern::create(pattern))
       config->warnBackrefsExclude.push_back(std::move(*pat));
     else

lld/ELF/ScriptParser.cpp

 }
 
 // Reads an anonymous version declaration.
 void ScriptParser::readAnonymousDeclaration() {
   std::vector<SymbolVersion> locals;
   std::vector<SymbolVersion> globals;
   std::tie(locals, globals) = readSymbols();
   for (const SymbolVersion &pat : locals)
-    config->versionDefinitions[VER_NDX_LOCAL].patterns.push_back(pat);
+    config->versionDefinitions[VER_NDX_LOCAL].localPatterns.push_back(pat);
   for (const SymbolVersion &pat : globals)
-    config->versionDefinitions[VER_NDX_GLOBAL].patterns.push_back(pat);
+    config->versionDefinitions[VER_NDX_GLOBAL].nonLocalPatterns.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 (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.patterns = globals;
+  ver.nonLocalPatterns = std::move(globals);
+  ver.localPatterns = std::move(locals);
   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.

lld/ELF/SymbolTable.h

public:
 
   // Comdat groups define "link once" sections. If two comdat groups have the
   // same name, only one of them is linked, and the other is ignored. This map
   // is used to uniquify them.
   llvm::DenseMap<llvm::CachedHashStringRef, const InputFile *> comdatGroups;
 
 private:
   std::vector<Symbol *> findByVersion(SymbolVersion ver);
-  std::vector<Symbol *> findAllByVersion(SymbolVersion ver);
+  std::vector<Symbol *> findAllByVersion(SymbolVersion ver,
+                                         bool includeNonDefault);
 
   llvm::StringMap<std::vector<Symbol *>> &getDemangledSyms();
-  void assignExactVersion(SymbolVersion ver, uint16_t versionId,
+  bool assignExactVersion(SymbolVersion ver, uint16_t versionId,
                           StringRef versionName);
-  void assignWildcardVersion(SymbolVersion ver, uint16_t versionId);
+  void assignWildcardVersion(SymbolVersion ver, uint16_t versionId,
+                             bool includeNonDefault);
 
   // The order the global symbols are in is not defined. We can use an arbitrary
   // order, but it has to be reproducible. That is true even when cross linking.
   // The default hashing of StringRef produces different results on 32 and 64
   // bit systems so we use a map to a vector. That is arbitrary, deterministic
   // but a bit inefficient.
   // FIXME: Experiment with passing in a custom hashing or sorting the symbols
   // once symbol resolution is finished.

lld/ELF/SymbolTable.cpp

std::vector<Symbol *> SymbolTable::findByVersion(SymbolVersion ver) {
   if (ver.isExternCpp)
     return getDemangledSyms().lookup(ver.name);
   if (Symbol *sym = find(ver.name))
     if (canBeVersioned(*sym))
       return {sym};
   return {};
 }
 
-std::vector<Symbol *> SymbolTable::findAllByVersion(SymbolVersion ver) {
+std::vector<Symbol *> SymbolTable::findAllByVersion(SymbolVersion ver,
+                                                    bool includeNonDefault) {
   std::vector<Symbol *> res;
   SingleStringMatcher m(ver.name);
 
   if (ver.isExternCpp) {
     for (auto &p : getDemangledSyms())
       if (m.match(p.first()))
-        res.insert(res.end(), p.second.begin(), p.second.end());
+        for (Symbol *sym : p.second)
+          if (includeNonDefault || !sym->getName().contains('@'))
+            res.push_back(sym);
     return res;
   }
 
   for (Symbol *sym : symVector)
-    if (canBeVersioned(*sym) && m.match(sym->getName()))
+    if (canBeVersioned(*sym) &&
+        (includeNonDefault || !sym->getName().contains('@')) &&
+        m.match(sym->getName()))
       res.push_back(sym);
   return res;
 }
 
 // Handles -dynamic-list.
 void SymbolTable::handleDynamicList() {
   for (SymbolVersion &ver : config->dynamicList) {
     std::vector<Symbol *> syms;
     if (ver.hasWildcard)
-      syms = findAllByVersion(ver);
+      syms = findAllByVersion(ver, /*includeNonDefault=*/true);
     else
       syms = findByVersion(ver);
 
     for (Symbol *sym : syms)
       sym->inDynamicList = true;
   }
 }
 
-// Set symbol versions to symbols. This function handles patterns
-// containing no wildcard characters.
-void SymbolTable::assignExactVersion(SymbolVersion ver, uint16_t versionId,
+// Set symbol versions to symbols. This function handles patterns containing no
+// wildcard characters. Return false if no symbol definition matches ver.
+bool SymbolTable::assignExactVersion(SymbolVersion ver, uint16_t versionId,
                                      StringRef versionName) {
-  if (ver.hasWildcard)
-    return;
-
   // Get a list of symbols which we need to assign the version to.
   std::vector<Symbol *> syms = findByVersion(ver);
-  if (syms.empty()) {
-    if (!config->undefinedVersion)
-      error("version script assignment of '" + versionName + "' to symbol '" +
-            ver.name + "' failed: symbol not defined");
-    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].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('@'))
+    // For a non-local versionId, skip symbols containing version info because
+    // symbol versions specified by symbol names take precedence over version
+    // scripts. See parseSymbolVersion().
+    if (versionId != VER_NDX_LOCAL && sym->getName().contains('@'))
       continue;
 
     // 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));
   }
+  return !syms.empty();
 }
 
-void SymbolTable::assignWildcardVersion(SymbolVersion ver, uint16_t versionId) {
+void SymbolTable::assignWildcardVersion(SymbolVersion ver, uint16_t versionId,
+                                        bool includeNonDefault) {
   // Exact matching takes precedence 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 *sym : findAllByVersion(ver))
+  for (Symbol *sym : findAllByVersion(ver, includeNonDefault))
     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() {
+  SmallString<128> buf;
   // First, we assign versions to exact matching symbols,
   // i.e. version definitions not containing any glob meta-characters.
-  for (VersionDefinition &v : config->versionDefinitions)
-    for (SymbolVersion &pat : v.patterns)
-      assignExactVersion(pat, v.id, v.name);
+  std::vector<Symbol *> syms;
+  for (VersionDefinition &v : config->versionDefinitions) {
+    auto assignExact = [&](SymbolVersion pat, uint16_t id, StringRef ver) {
+      bool found = assignExactVersion(pat, id, ver);
+      found |= assignExactVersion({(pat.name + "@" + v.name).toStringRef(buf),
+                                   pat.isExternCpp, /*hasWildCard=*/false},
+                                  id, ver);
+      if (!found && !config->undefinedVersion)
+        errorOrWarn("version script assignment of '" + ver + "' to symbol '" +
+                    pat.name + "' failed: symbol not defined");
+    };
+    for (SymbolVersion &pat : v.nonLocalPatterns)
+      if (!pat.hasWildcard)
+        assignExact(pat, v.id, v.name);
+    for (SymbolVersion pat : v.localPatterns)
+      if (!pat.hasWildcard)
+        assignExact(pat, VER_NDX_LOCAL, "local");
+  }
 
   // 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 &pat : v.patterns)
+  auto assignWildcard = [&](SymbolVersion pat, uint16_t id, StringRef ver) {
+    assignWildcardVersion(pat, id, /*includeNonDefault=*/false);
+    assignWildcardVersion({(pat.name + "@" + ver).toStringRef(buf),
+                           pat.isExternCpp, /*hasWildCard=*/true},
+                          id,
+                          /*includeNonDefault=*/true);
+  };
+  for (VersionDefinition &v : llvm::reverse(config->versionDefinitions)) {
+    for (SymbolVersion &pat : v.nonLocalPatterns)
+      if (pat.hasWildcard && pat.name != "*")
+        assignWildcard(pat, v.id, v.name);
+    for (SymbolVersion &pat : v.localPatterns)
       if (pat.hasWildcard && pat.name != "*")
-        assignWildcardVersion(pat, v.id);
+        assignWildcard(pat, VER_NDX_LOCAL, v.name);
+  }
 
   // Then, assign versions to "*". In GNU linkers they have lower priority than
   // other wildcards.
-  for (VersionDefinition &v : config->versionDefinitions)
-    for (SymbolVersion &pat : v.patterns)
+  for (VersionDefinition &v : config->versionDefinitions) {
+    for (SymbolVersion &pat : v.nonLocalPatterns)
+      if (pat.hasWildcard && pat.name == "*")
+        assignWildcard(pat, v.id, v.name);
+    for (SymbolVersion &pat : v.localPatterns)
       if (pat.hasWildcard && pat.name == "*")
-        assignWildcardVersion(pat, v.id);
+        assignWildcard(pat, VER_NDX_LOCAL, v.name);
+  }
 
   // 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. Compute symbol versions before handling
   // --dynamic-list.
   handleDynamicList();
 }

lld/ELF/Symbols.cpp

if (auto *s = dyn_cast<Defined>(this)) {
     return nullptr;
   }
   return nullptr;
 }
 
 // If a symbol name contains '@', the characters after that is
 // a symbol version name. This function parses that.
 void Symbol::parseSymbolVersion() {
+  // Return if localized by a local: pattern in a version script.
+  if (versionId == VER_NDX_LOCAL)
+    return;
   StringRef s = getName();
   size_t pos = s.find('@');
   if (pos == 0 || pos == StringRef::npos)
     return;
   StringRef verstr = s.substr(pos + 1);
   if (verstr.empty())
     return;
 

lld/test/ELF/version-script-noundef.s

 # RUN:   %t.o -o %t.so 2>&1 | FileCheck -check-prefix=ERR2 %s
 # ERR2: version script assignment of 'VERSION_1.0' to symbol 'und' failed: symbol not defined
 
 # RUN: echo "VERSION_1.0 { local: und; };" > %t3.script
 # RUN: not ld.lld --version-script %t3.script -shared --no-undefined-version \
 # RUN:   %t.o -o %t.so 2>&1 | FileCheck -check-prefix=ERR3 %s
 # ERR3: version script assignment of 'local' to symbol 'und' failed: symbol not defined
 
+## Wildcard patterns do not error.
+# RUN: echo "VERSION_1.0 { global: b*; local: u*; };" > %t4.script
+# RUN: ld.lld --version-script %t4.script -shared --no-undefined-version --fatal-warnings %t.o -o /dev/null
+
 .text
 .globl foo
 .type foo,@function
 foo:
 callq und@PLT

lld/test/ELF/version-script-symver.s

 # EXACT-NOT:  {{.}}
 
 # RUN: echo 'v1 { local: foo*; }; v2 {};' > %t2.script
 # RUN: ld.lld --version-script %t2.script -shared %t.o -o %t2.so
 # RUN: llvm-readelf --dyn-syms %t2.so | FileCheck --check-prefix=WC %s
 # WC:      UND
 # WC-NEXT: [[#]] foo4@@v2
 # WC-NEXT: [[#]] _start{{$}}
-# WC-NEXT: [[#]] foo3@v1
 # WC-NOT:  {{.}}
 
 # RUN: echo 'v1 { global: *; local: foo*; }; v2 {};' > %t3.script
 # RUN: ld.lld --version-script %t3.script -shared %t.o -o %t3.so
 # RUN: llvm-readelf --dyn-syms %t3.so | FileCheck --check-prefix=MIX1 %s
 # MIX1:      UND
 # MIX1-NEXT: [[#]] foo4@@v2
 # MIX1-NEXT: [[#]] _start@@v1
-# MIX1-NEXT: [[#]] foo3@v1
 # MIX1-NOT:  {{.}}
 
 # RUN: echo 'v1 { global: foo*; local: *; }; v2 { global: foo4; local: *; };' > %t4.script
 # RUN: ld.lld --version-script %t4.script -shared %t.o -o %t4.so
 # RUN: llvm-readelf --dyn-syms %t4.so | FileCheck --check-prefix=MIX2 %s
 # MIX2:      UND
 # MIX2-NEXT: [[#]] foo1@@v1
 # MIX2-NEXT: [[#]] foo2@@v1