[ELF] - Implement extern "c++" version script tag

[grimar]

Summary

Patch implements 'extern' version script tag.
Currently only values in quotes(") are supported.

Matching of externs is performed in the same pass as exact match of globals.

[ruiu]

The new code seems to be a copy of the existing code with a small change to demangle symbols. Can you make it smaller?

ELF/SymbolTable.cpp

564 ↗

(On Diff #62501)

We already have elf::demangle function.

[grimar]

ELF/SymbolTable.cpp

564 ↗

(On Diff #62501)

Sure we have it, but it will not work here I think. At least it has 2 conditions that we don't want to see here:

if (!Config->Demangle)
  return Name;

// __cxa_demangle can be used to demangle strings other than symbol
// names which do not necessarily start with "_Z". Name can be
// either a C or C++ symbol. Don't call __cxa_demangle if the name
// does not look like a C++ symbol name to avoid getting unexpected
// result for a C symbol that happens to match a mangled type name.
if (!Name.startswith("_Z"))
  return Name;

[grimar]

In D21930#473140, @ruiu wrote:

The new code seems to be a copy of the existing code with a small change to demangle symbols. Can you make it smaller?

I'll try to do something. Let's land dependency D21894 first.

[emaste]

if (!Name.startswith("_Z"))
   return Name;

This one is needed, otherwise a symbol f would be demangled to float. We want only mangled symbol names (which start with _Z), not types etc.

[grimar]

In D21930#473665, @emaste wrote:

if (!Name.startswith("_Z"))
   return Name;

This one is needed, otherwise a symbol f would be demangled to float. We want only mangled symbol names (which start with _Z), not types etc.

Ah, oh. I was misunderstanding meaning of "_Z" then.

[ruiu]

ELF/Config.h

38 ↗

(On Diff #62501)

SymbolVersion is probably better?

ELF/SymbolTable.cpp

564 ↗

(On Diff #62501)

You can remove if (!Config->Demangle) from the function and check the condition on caller side. As to the second condition, you actually want it, no?

[grimar]

• Addressed review comments.

ELF/Config.h

38 ↗

(On Diff #62501)

Done.

ELF/SymbolTable.cpp

564 ↗

(On Diff #62501)

Right.

[ruiu]

ELF/SymbolTable.cpp

560 ↗

(On Diff #63045)

I'd name this setVersionId. Also rename Entry -> Name.

601–607 ↗

(On Diff #63045)

I do not understand the exact logic. Can this handle C++ mangled name patterns that contain glob meta characters?

[grimar]

ELF/SymbolTable.cpp

601–607 ↗

(On Diff #63045)

This patch handles only exact matches.
According to rules of processing version script files described in Ian Lance Taylor article,
handling of wildcards is the last step and the same should be applied for wildcarded
externs. So handling wildcards in externs is a job for following patches I think.

Speaking about exactly this piece of code - it just builds association map to map [extern name -> its version].
That is done to simplify and accelerate the pass to handle externs (lines 621-628). Without that map I would
need to iterate over all versions and all externs after demangle() call to find which version to use.
It would be something like next without this map (pseudocode):

for (auto I = Symtab.begin(); I != Symtab.end(); ++I) {
  StringRef Demangled = demangle(I->first.Val);

  for (size_t J = 0, E = Config->SymbolVersions.size(); J < E; ++J) {
    Version &V = Config->SymbolVersions[I];
    for (SymbolVersion Sym : V.Globals) {
        if (!Sym.IsExternCpp)
          continue;
      
        if (Sym.Name == Demangled) {
          overrideSymbolVersion(SymVector[I->second], Sym.Name, J + 2);
          return;
        }
      }
    }
}

[ruiu]

What I was trying to say is that the code did not look very straightforward. I think you have no choice other than demangle all symbols if "extern" directive is used, so why don't you create a map from demangled names to symbolbodies?

ELF/SymbolTable.cpp

591 ↗

(On Diff #63045)

Add

DenseMap<StringRef, SymbolBody *> Demangled;
if (hasExternCpp())
  Demangled = getDemangledSyms();

here and then

609 ↗

(On Diff #63045)

replace this line with

SymbolBody *B = Sym.IsExternCpp ? Demangled[Sym.Name] : find(Sym.Name);

[grimar]

• Addressed review comments.

ELF/SymbolTable.cpp

560 ↗

(On Diff #63045)

Done.

560 ↗

(On Diff #63045)

Done.

591 ↗

(On Diff #63045)

I implemented the close way below. I can't use StringRef as a key as demangle() returns string.
DenseMapInfo does not have specialization for std::strings.
And so I had to use std::map instead with std::string as a key.

609 ↗

(On Diff #63045)

This just will not work I think..
Ian Lance Taylor wrote about current rules in gold (http://www.airs.com/blog/archives/300):

Here are the current rules for gold:

If there is an exact match for the mangled name, we use it.

...

Otherwise, we look for an extern C++ or an extern Java exact match. If we find an exact match, we use it.

...

So externs matching should be done after common case matching, according to that rules we probably want to follow.

[ruiu]

ELF/SymbolTable.cpp

572 ↗

(On Diff #63497)

If you pass a SymbolBody, do you have to pass Name? I think you can obtain it by Body->getName().

589 ↗

(On Diff #63497)

Why don't you use a range-based for?

627–628 ↗

(On Diff #63497)

I'd disagree -- that rule doesn't make sense to me. If you specify a mangled name (that contains no wildcards), then that is as specific/strict as demangled names. I don't see a reason to look for extern C++ names after non-extern-C++ names.

[grimar]

• Addressed review comments.

ELF/SymbolTable.cpp

572 ↗

(On Diff #63497)

Here Name is the how it is written in linkerscript. It is SymbolVersion::Name.
So if we have demangled name under extern tag,
it will show it instead of mangled symbol name we can obtain from Body->getName() for example.
That is btw was why I named argument as Entry initially, because Name is a bit confusing in that case,
though it is still a name.

589 ↗

(On Diff #63497)

Fixed.

627–628 ↗

(On Diff #63497)

Ok. I agree here, implemented that just because I thought we want to follow the existent rules to prevent possible conflicts in existent software. Lets see how it works.

[grimar]

• Updated comment.

[ruiu]

LGTM

[grimar]

I had to revert this.
It broke 2 build bots:

1. http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/8204/steps/test/logs/stdio

I have no idea why it failed because I specified XFAIL: win32 in testcase, so it was expected fails, though bot errors out.

2. http://lab.llvm.org:8011/builders/lld-x86_64-freebsd/builds/19432/steps/test_lld/logs/stdio

Has the same error message. I guess it does not have HAVE_CXXABI_H.

So I am goint to rework the testcase and recommit later. Though I am curious why windows bot reports error when XFAIL is set ?

[grimar]

Recommitted as rL275682 + rL275683, FreeBSD bot is happy how. Thanks to Ed, Rafael and Rui for help in solving this.

lld/trunk/ELF/Config.h

enum ELFKind {
   ELF64LEKind,
   ELF64BEKind
 };
 
 enum class BuildIdKind { None, Fnv1, Md5, Sha1, Hexstring };
 
 enum class UnresolvedPolicy { NoUndef, Error, Warn, Ignore };
 
+struct SymbolVersion {
+  llvm::StringRef Name;
+  bool IsExternCpp;
+};
+
 // 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, size_t Id) : Name(Name), Id(Id) {}
   llvm::StringRef Name;
   size_t Id;
-  std::vector<llvm::StringRef> Globals;
+  std::vector<SymbolVersion> 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.
 struct Configuration {
struct Configuration {
   llvm::StringRef SoName;
   llvm::StringRef Sysroot;
   llvm::StringSet<> TraceSymbol;
   std::string RPath;
   std::vector<Version> SymbolVersions;
   std::vector<llvm::StringRef> DynamicList;
   std::vector<llvm::StringRef> SearchPaths;
   std::vector<llvm::StringRef> Undefined;
-  std::vector<llvm::StringRef> VersionScriptGlobals;
+  std::vector<SymbolVersion> VersionScriptGlobals;
   std::vector<uint8_t> BuildIdVector;
   bool AllowMultipleDefinition;
   bool AsNeeded = false;
   bool Bsymbolic;
   bool BsymbolicFunctions;
   bool Demangle = true;
   bool DisableVerify;
   bool DiscardAll;

lld/trunk/ELF/SymbolListFile.cpp

 
 class VersionScriptParser final : public ScriptParserBase {
 public:
   VersionScriptParser(StringRef S) : ScriptParserBase(S) {}
 
   void run();
 
 private:
+  void parseExtern(std::vector<SymbolVersion> *Globals);
   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.

 
 void VersionScriptParser::parseLocal() {
   expect("local:");
   expect("*");
   expect(";");
   Config->VersionScriptGlobalByDefault = false;
 }
 
+void VersionScriptParser::parseExtern(std::vector<SymbolVersion> *Globals) {
+  expect("extern");
+  expect("C++");
+  expect("{");
+
+  for (;;) {
+    if (peek() == "}" || Error)
+      break;
+    Globals->push_back({next(), true});
+    expect(";");
+  }
+
+  expect("}");
+  expect(";");
+}
+
 void VersionScriptParser::parseVersionSymbols(StringRef Version) {
-  std::vector<StringRef> *Globals;
+  std::vector<SymbolVersion> *Globals;
   if (Version.empty())
     Globals = &Config->VersionScriptGlobals;
   else
     Globals = &Config->SymbolVersions.back().Globals;
 
   for (;;) {
+    if (peek() == "extern")
+      parseExtern(Globals);
+
     StringRef Cur = peek();
-    if (Cur == "extern")
-      setError("extern keyword is not supported");
     if (Cur == "}" || Cur == "local:" || Error)
       return;
     next();
-    Globals->push_back(Cur);
+    Globals->push_back({Cur, false});
     expect(";");
   }
 }
 
 void VersionScriptParser::run() {
   StringRef Msg = "anonymous version definition is used in "
                   "combination with other version definitions";
   if (peek() == "{") {

lld/trunk/ELF/SymbolTable.h

private:
   std::pair<Symbol *, bool> insert(StringRef Name);
   std::pair<Symbol *, bool> insert(StringRef Name, uint8_t Type,
                                    uint8_t Visibility, bool CanOmitFromDynSym,
                                    bool IsUsedInRegularObj, InputFile *File);
 
   std::string conflictMsg(SymbolBody *Existing, InputFile *NewFile);
   void reportDuplicate(SymbolBody *Existing, InputFile *NewFile);
 
+  std::map<std::string, SymbolBody *> getDemangledSyms();
+
   // 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.
   llvm::DenseMap<SymName, unsigned> Symtab;

lld/trunk/ELF/SymbolTable.cpp

for (StringRef S : Config->DynamicList)
     if (SymbolBody *B = find(S))
       B->symbol()->ExportDynamic = true;
 }
 
 static bool hasWildcard(StringRef S) {
   return S.find_first_of("?*") != StringRef::npos;
 }
 
+static void setVersionId(SymbolBody *Body, StringRef VersionName,
+                         StringRef Name, uint16_t Version) {
+  if (!Body || Body->isUndefined()) {
+    if (Config->NoUndefinedVersion)
+      error("version script assignment of " + VersionName + " to symbol " +
+            Name + " failed: symbol not defined");
+    return;
+  }
+
+  Symbol *Sym = Body->symbol();
+  if (Sym->VersionId != VER_NDX_GLOBAL && Sym->VersionId != VER_NDX_LOCAL)
+    warning("duplicate symbol " + Name + " in version script");
+  Sym->VersionId = Version;
+}
+
+template <class ELFT>
+std::map<std::string, SymbolBody *> SymbolTable<ELFT>::getDemangledSyms() {
+  std::map<std::string, SymbolBody *> Result;
+  for (std::pair<SymName, unsigned> Sym : Symtab)
+    Result[demangle(Sym.first.Val)] = SymVector[Sym.second]->body();
+  return Result;
+}
+
+static bool hasExternCpp() {
+  for (Version& V : Config->SymbolVersions)
+    for (SymbolVersion Sym : V.Globals)
+      if (Sym.IsExternCpp)
+        return true;
+  return false;
+}
+
 // This function processes the --version-script option by marking all global
 // symbols with the VersionScriptGlobal flag, which acts as a filter on the
 // dynamic symbol table.
 template <class ELFT> void SymbolTable<ELFT>::scanVersionScript() {
   // If version script does not contain versions declarations,
   // we just should mark global symbols.
   if (!Config->VersionScriptGlobals.empty()) {
-    for (StringRef S : Config->VersionScriptGlobals)
-      if (SymbolBody *B = find(S))
+    for (SymbolVersion &Sym : Config->VersionScriptGlobals)
+      if (SymbolBody *B = find(Sym.Name))
         B->symbol()->VersionId = VER_NDX_GLOBAL;
     return;
   }
 
   if (Config->SymbolVersions.empty())
     return;
 
   // If we have symbols version declarations, we should
   // assign version references for each symbol.
   // Current rules are:
-  // * If there is an exact match for the mangled name, we use it.
+  // * If there is an exact match for the mangled name or we have extern C++
+  //   exact match, then we use it.
   // * Otherwise, we look through the wildcard patterns. We look through the
   //   version tags in reverse order. We use the first match we find (the last
   //   matching version tag in the file).
-  for (size_t I = 0, E = Config->SymbolVersions.size(); I < E; ++I) {
-    Version &V = Config->SymbolVersions[I];
-    for (StringRef Name : V.Globals) {
-      if (hasWildcard(Name))
-        continue;
 
-      SymbolBody *B = find(Name);
-      if (!B || B->isUndefined()) {
-        if (Config->NoUndefinedVersion)
-          error("version script assignment of " + V.Name + " to symbol " +
-                Name + " failed: symbol not defined");
+  // Handle exact matches and build a map of demangled externs for
+  // quick search during next step.
+  std::map<std::string, SymbolBody *> Demangled;
+  if (hasExternCpp())
+    Demangled = getDemangledSyms();
+
+  for (Version &V : Config->SymbolVersions) {
+    for (SymbolVersion Sym : V.Globals) {
+      if (hasWildcard(Sym.Name))
         continue;
-      }
-
-      if (B->symbol()->VersionId != VER_NDX_GLOBAL &&
-          B->symbol()->VersionId != VER_NDX_LOCAL)
-        warning("duplicate symbol " + Name + " in version script");
-      B->symbol()->VersionId = V.Id;
+      SymbolBody *B = Sym.IsExternCpp ? Demangled[Sym.Name] : find(Sym.Name);
+      setVersionId(B, V.Name, Sym.Name, V.Id);
     }
   }
 
+  // Handle wildcards.
   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))
+    for (SymbolVersion Sym : V.Globals) {
+      if (!hasWildcard(Sym.Name))
         continue;
 
-      for (SymbolBody *B : findAll(Name))
+      for (SymbolBody *B : findAll(Sym.Name))
         if (B->symbol()->VersionId == VER_NDX_GLOBAL ||
             B->symbol()->VersionId == VER_NDX_LOCAL)
           B->symbol()->VersionId = V.Id;
     }
   }
 }
 
 // Print the module names which define the notified

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

 // REQUIRES: x86
 
 // RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
 // RUN: not ld.lld -shared %t.o -o %t.so --version-script %p/Inputs/version-script-err.script 2>&1 | FileCheck %s
 // CHECK: ; expected, but got }
 
 // RUN: echo    "\"" > %terr1.script
 // RUN: not ld.lld --version-script %terr1.script -shared %t.o -o %t.so 2>&1 | \
 // RUN:   FileCheck -check-prefix=ERR1 %s
 // ERR1: unclosed quote
-
-// RUN: echo "VERSION { extern "C++" {}; }; " > %terr2.script
-// RUN: not ld.lld --version-script %terr2.script -shared %t.o -o %t.so 2>&1 | \
-// RUN:   FileCheck -check-prefix=ERR2 %s
-// ERR2: extern keyword is not supported

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

+# REQUIRES: x86
+# XFAIL: win32
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+# RUN: echo "LIBSAMPLE_1.0 { \
+# RUN:   global:             \
+# RUN:      extern "C++" {   \
+# RUN:         \"foo()\";    \
+# RUN:         \"zed()\";    \
+# RUN:   };                  \
+# RUN: };                    \
+# RUN: LIBSAMPLE_2.0 {       \
+# RUN:   global:             \
+# RUN:     extern "C++" {    \
+# RUN:       \"bar()\";      \
+# RUN:   };                  \
+# RUN: }; " > %t.script
+# RUN: ld.lld --version-script %t.script -shared %t.o -o %t.so
+# RUN: llvm-readobj -V -dyn-symbols %t.so | FileCheck --check-prefix=DSO %s
+
+# DSO:      DynamicSymbols [
+# DSO-NEXT:    Symbol {
+# DSO-NEXT:      Name: @
+# DSO-NEXT:      Value: 0x0
+# DSO-NEXT:      Size: 0
+# DSO-NEXT:      Binding: Local
+# DSO-NEXT:      Type: None
+# DSO-NEXT:      Other: 0
+# DSO-NEXT:      Section: Undefined
+# DSO-NEXT:    }
+# DSO-NEXT:    Symbol {
+# DSO-NEXT:      Name: _Z3barv@@LIBSAMPLE_2.0
+# DSO-NEXT:      Value: 0x1001
+# DSO-NEXT:      Size: 0
+# DSO-NEXT:      Binding: Global
+# DSO-NEXT:      Type: Function
+# DSO-NEXT:      Other: 0
+# DSO-NEXT:      Section: .text
+# DSO-NEXT:    }
+# DSO-NEXT:    Symbol {
+# DSO-NEXT:      Name: _Z3foov@@LIBSAMPLE_1.0
+# DSO-NEXT:      Value: 0x1000
+# DSO-NEXT:      Size: 0
+# DSO-NEXT:      Binding: Global
+# DSO-NEXT:      Type: Function
+# DSO-NEXT:      Other: 0
+# DSO-NEXT:      Section: .text
+# DSO-NEXT:    }
+# DSO-NEXT:    Symbol {
+# DSO-NEXT:      Name: _Z3zedv@@LIBSAMPLE_1.0
+# DSO-NEXT:      Value: 0x1002
+# DSO-NEXT:      Size: 0
+# DSO-NEXT:      Binding: Global (0x1)
+# DSO-NEXT:      Type: Function (0x2)
+# DSO-NEXT:      Other: 0
+# DSO-NEXT:      Section: .text (0x6)
+# DSO-NEXT:    }
+# DSO-NEXT:  ]
+# DSO-NEXT:  Version symbols {
+# DSO-NEXT:    Section Name: .gnu.version
+# DSO-NEXT:    Address: 0x228
+# DSO-NEXT:    Offset: 0x228
+# DSO-NEXT:    Link: 1
+# DSO-NEXT:    Symbols [
+# DSO-NEXT:      Symbol {
+# DSO-NEXT:        Version: 0
+# DSO-NEXT:        Name: @
+# DSO-NEXT:      }
+# DSO-NEXT:      Symbol {
+# DSO-NEXT:        Version: 3
+# DSO-NEXT:        Name: _Z3barv@@LIBSAMPLE_2.0
+# DSO-NEXT:      }
+# DSO-NEXT:      Symbol {
+# DSO-NEXT:        Version: 2
+# DSO-NEXT:        Name: _Z3foov@@LIBSAMPLE_1.0
+# DSO-NEXT:      }
+# DSO-NEXT:      Symbol {
+# DSO-NEXT:        Version: 2
+# DSO-NEXT:        Name: _Z3zedv@@LIBSAMPLE_1.0
+# DSO-NEXT:      }
+# DSO-NEXT:    ]
+# DSO-NEXT:  }
+
+.text
+.globl _Z3foov
+.type _Z3foov,@function
+_Z3foov:
+retq
+
+.globl _Z3barv
+.type _Z3barv,@function
+_Z3barv:
+retq
+
+.globl _Z3zedv
+.type _Z3zedv,@function
+_Z3zedv:
+retq