[PATCH] D92259: [ELF] Make foo@@v1 resolve undefined foo at v1

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Driver.cpp:1921
   }
+  for (Symbol *sym : symtab->symbols()) {
+    // Enumerate symbols with a non-default version (foo at v1). Its name was
----------------
jhenderson wrote:
> Seems to me like iterating through all symbols could be quite expensive, based on an assumption that most symbols aren't versioned. At least for our platform, we don't even support symbol versioning, so there shouldn't be ANY versioned symbols, meaning an unnecessary trawl through the symbols. Would we be better off recording earlier and then using here a list of versioned symbols?
Added a TimeScope.

In my testing, 
for a 4.9GiB output, redirectSymbols takes 49ms in a 31s link, so the cost seems acceptable.
For a 161MiB clang, redirectSymbols takes 14ms in the 923ms link, so the cost (1.5%) is a bit significant.

I added an early return below, then it just takes 2ms for clang. I think the cost is acceptable.

(On an unrelated thing, 048b16f7fbb745635b48d31ee957bb8865597606 reclaimed a 1%~1.7% performance loss.)


================
Comment at: lld/ELF/Driver.cpp:1924
+    // truncated at '@' by Symbol::parseSymbolVersion().
+    StringRef name = sym->getName();
+    const char *end1 = name.data() + name.size();
----------------
grimar wrote:
> Should we add something like `Symbol::getFullName` or `Symbol::getVersionedName`?
> The code like `const char *end1 = name.data() + name.size()` looks hacky. I'd isolate it.
Added `Symbol::getVersionSuffix`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92259/new/

https://reviews.llvm.org/D92259