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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
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},
----------------
peter.smith wrote:
> 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`
> 
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 at 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
```


================
Comment at: lld/ELF/SymbolTable.cpp:280
+        {(pat.name + "@" + ver).toStringRef(buf), pat.isExternCpp, true}, id,
+        true);
+  };
----------------
peter.smith wrote:
> /* checkAt */ true
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107234