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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I'm not much of a symbol versioning expert (see my inline comment), but this approach seems reasonable to me at least.

In D92259#2421021 <https://reviews.llvm.org/D92259#2421021>, @MaskRay wrote:

> (FWIW I have made some notes about symbol versioning at https://translate.google.com/translate?sl=zh-CN&tl=en&u=https%3A%2F%2Fmaskray.me%2Fblog%2F2020-11-26-all-about-symbol-versioning)
> (Apparently people are annoyed by lack of symbol versioning documentation https://invisible-island.net/ncurses/ncurses-mapsyms.html  :) )
> (We may need one document under `docs/ELF/` describing the LLD behavior)

In my opinion, you can never have too much documentation.



================
Comment at: lld/ELF/Driver.cpp:1915
 // so that wrapped symbols are swapped as instructed by the command line.
-static void wrapSymbols(ArrayRef<WrappedSymbol> wrapped) {
+static void redirectSymbols(ArrayRef<WrappedSymbol> wrapped) {
   DenseMap<Symbol *, Symbol *> map;
----------------
Given the possibility for a significant link time cost to this section of code where there are lots of (versioned) symbols, perhaps it is worth adding a time scope to this function with this change?


================
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
----------------
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?


================
Comment at: lld/ELF/Driver.cpp:1922-1923
+  for (Symbol *sym : symtab->symbols()) {
+    // Enumerate symbols with a non-default version (foo at v1). Its name was
+    // truncated at '@' by Symbol::parseSymbolVersion().
+    StringRef name = sym->getName();
----------------
There's a slight inconsistency in your grammar here - the first sentence refers to multiple symbols, but "Its name" implies a singular symbol. Probably best to rewrite as suggested.


================
Comment at: lld/ELF/Driver.cpp:1929
+
+    // Check whether the default version foo@@v1 exists. If exists, the symbol
+    // can be found by the name "foo" in the symbol table.
----------------



================
Comment at: lld/ELF/Driver.cpp:1939
+
+    // foo at v1 and foo@@v1 should be considered as one symbol. So we redirect
+    // foo at v1 references to foo@@v1.
----------------



================
Comment at: lld/test/ELF/symver.s:15
 
-# TODO Report an duplicate definition error for foo at v1 and foo@@v1.
-# RUN: ld.lld -shared --version-script=%t/ver %t/def1.o %t/hid1.o -o /dev/null
-
+# Report an duplicate definition error for foo at v1 and foo@@v1.
+# RUN: not ld.lld -shared --version-script=%t/ver %t/def1.o %t/hid1.o -o /dev/null 2>&1 | \
----------------



================
Comment at: lld/test/ELF/symver.s:41
 
-# CHECK:       1: {{.*}} NOTYPE GLOBAL DEFAULT UND       foo{{$}}
-# CHECK-NEXT:  2: {{.*}} NOTYPE GLOBAL DEFAULT {{[1-9]}} foo@@v1
+# CHECK:       1: {{.*}} NOTYPE GLOBAL DEFAULT {{[1-9]}} foo@@v1
 # CHECK-EMPTY:
----------------
Optional improvement to this check, as it will a) still pass if there are more than 10 section headers, and b) is shorter. (It will start matching against `0` too, but that should be printed as `UND` so there is no issue here).

You can use the same technique for the protected case too, if you want.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92259