[PATCH] D26395: [ELF] - Add support for locals list in version script.

[George Rimar via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/SymbolTable.cpp:580-581
   Symbol *Sym = Body->symbol();
-  if (Sym->VersionId != Config->DefaultSymbolVersion)
+  auto It = Proccessed.insert(Body);
+  if (!It.second)
     warn("duplicate symbol " + Name + " in version script");
----------------
ruiu wrote:
> grimar wrote:
> > ruiu wrote:
> > > Do you actually need this guard?
> > I think so. We want to show this warning if assigning version to symbol that is already has one,
> > (and switch to error one day https://llvm.org/bugs/show_bug.cgi?id=28342) 
> > also we want to show warning if symbol is both in local and global list as it is obvious error.
> > Actually I even thing we want to error out in latter case, that is how gold do afaik, planned to suggest that in a separate patch to keep this bit simpler for review.
> I mean, why the previous condition `Sym->VersionId != Config->DefaultSymbolVersion` wouldn't work?
Ah, right. It is enough, no need in additional set at all.


================
Comment at: ELF/SymbolTable.cpp:725
   // we iterate over the definitions in the reverse order.
+  auto assignFuzzyVersion = [&](SymbolVersion &Sym, size_t Version) {
+    if (!Sym.HasWildcards)
----------------
ruiu wrote:
> Why symbol version number is of `size_t`?
Because we pass VersionDefinition::Id, which is size_t,
because we use next code:

```
  size_t VersionId = Config->VersionDefinitions.size() + 2;
  Config->VersionDefinitions.push_back({VerStr, VersionId});
```


https://reviews.llvm.org/D26395