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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

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.



================
Comment at: lld/ELF/Config.h:89
   uint16_t id;
   std::vector<SymbolVersion> patterns;
+  std::vector<SymbolVersion> localPatterns;
----------------
Is it worth renaming this to nonLocalPatterns (globalPatterns might work, but may not be strictly accurate).


================
Comment at: lld/ELF/SymbolTable.cpp:190
 // containing no wildcard characters.
-void SymbolTable::assignExactVersion(SymbolVersion ver, uint16_t versionId,
+bool SymbolTable::assignExactVersion(SymbolVersion ver, uint16_t versionId,
                                      StringRef versionName) {
----------------
Suggest adding
// return false if no symbol definition matches ver.


================
Comment at: lld/ELF/SymbolTable.cpp:256
+        bool found = assignExactVersion(pat, v.id, v.name);
+        found |= assignExactVersion({(pat.name + "@" + v.name).toStringRef(buf),
+                                     pat.isExternCpp, false},
----------------
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 at 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 at v1";
but later on in assignExactVersion there is:
```
if (versionId != VER_NDX_LOCAL && sym->getName().contains('@'))
      continue;
```
which I'd assume foo at v1 would always end up in continue.

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



================
Comment at: lld/ELF/SymbolTable.cpp:257
+        found |= assignExactVersion({(pat.name + "@" + v.name).toStringRef(buf),
+                                     pat.isExternCpp, false},
+                                    v.id, v.name);
----------------
Would be useful to have /* hasWildcard */ false)


================
Comment at: lld/ELF/SymbolTable.cpp:265
+        bool found = assignExactVersion(pat, VER_NDX_LOCAL, "local");
+        found |= assignExactVersion({(pat.name + "@" + v.name).toStringRef(buf),
+                                     pat.isExternCpp, false},
----------------
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.


================
Comment at: lld/ELF/SymbolTable.cpp:266
+        found |= assignExactVersion({(pat.name + "@" + v.name).toStringRef(buf),
+                                     pat.isExternCpp, false},
+                                    VER_NDX_LOCAL, "local");
----------------
Would be useful to have /* hasWildcard */ false)


================
Comment at: lld/ELF/SymbolTable.cpp:277
+  auto assignWildcard = [&](SymbolVersion pat, uint16_t id, StringRef ver) {
+    assignWildcardVersion(pat, id, false);
+    assignWildcardVersion(
----------------
/* checkAt */ false


================
Comment at: lld/ELF/SymbolTable.cpp:279
+    assignWildcardVersion(
+        {(pat.name + "@" + ver).toStringRef(buf), pat.isExternCpp, true}, id,
+        true);
----------------
/* hasWildcard */ true


================
Comment at: lld/ELF/SymbolTable.cpp:280
+        {(pat.name + "@" + ver).toStringRef(buf), pat.isExternCpp, true}, id,
+        true);
+  };
----------------
/* checkAt */ true


================
Comment at: lld/ELF/SymbolTable.h:68
   std::vector<Symbol *> findByVersion(SymbolVersion ver);
-  std::vector<Symbol *> findAllByVersion(SymbolVersion ver);
+  std::vector<Symbol *> findAllByVersion(SymbolVersion ver, bool checkAt);
 
----------------
Instead of "checkAt" I suggest "includeNonDefault" or "includeNonDefaultVer". 


================
Comment at: lld/ELF/SymbolTable.h:73
                           StringRef versionName);
-  void assignWildcardVersion(SymbolVersion ver, uint16_t versionId);
+  void assignWildcardVersion(SymbolVersion ver, uint16_t versionId,
+                             bool checkAt);
----------------
Same as above, I suggest "includeNonDefault"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107234