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

[George Rimar via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/LinkerScript.cpp:1807
+  if (VerStr.empty())
+    error("locals list for anonymous version is not supported");
+
----------------
ruiu wrote:
> Use setError.
Done.


================
Comment at: ELF/LinkerScript.cpp:1810
+  std::vector<SymbolVersion> &Locals = Config->VersionDefinitions.back().Locals;
+  for (;;) {
+    StringRef Cur = peek();
----------------
ruiu wrote:
> Please take a look at other places. We usually write a loop this way to consume tokens until "}".
> 
>   while (!Error && !consume("}")) {
>     ...
>   }
I did :) readLocal was written similar to readExtern.
Changed code to use while + peek, consume will not work here.


================
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:
> 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.


================
Comment at: ELF/SymbolTable.cpp:697
+        continue;
+      setVersionId(find(Sym.Name), V.Name, Sym.Name, VER_NDX_LOCAL, Proccessed);
     }
----------------
ruiu wrote:
> You want to care about the case in which IsExternCpp is true.
Yes, but currently IsExternCpp is never set to true for locals. 
Scripts with externs in locals will fail to parse, added testcase to demonstrate.
I suggest to implement them in a separate patch.
(as well as support for locals in anonymous versions declarations).


https://reviews.llvm.org/D26395