[ELF] - Remove VersionScriptParser class and move the members to ScriptParser

[grimar]

Summary

Patch removes VersionScriptParser class and moves the members to ScriptParser

This was requested in review comments for D23609, D23660.

It opens road for implementation of VERSION linkerscript command.

[ruiu]

ELF/LinkerScript.cpp

593–596

Instead of adding another parameter to the constructor, add another entry point function than run. Actually I'd rename run parseLinkerScript and add parseVersionScript.

635–638

What's the difference between read prefix and parse prefix? If there's no reason, rename them to start with read.

1416

Remove this empty line.

[grimar]

• Addressed review comments.

[grimar]

ELF/LinkerScript.cpp

593–596

Done.

635–638

Done.

1416

Done.

[• rafael]

Fine by me, but wait for a final OK from Rui.

ELF/LinkerScript.cpp

593–596

Given the other names, maybe readLinkerScript and readVersionScript?

1411

The comment is out of date, no? We can probably delete it.

[ruiu]

LGTM

ELF/LinkerScript.cpp

1411

Yeah, it is I think out of date.

1415

parse -> read for the sake of consistency.

ELF/LinkerScript.h

 template <class ELFT> class LayoutInputSection;
 
 typedef std::function<uint64_t(uint64_t)> Expr;
 
 // Parses a linker script. Calling this function updates
 // Config and ScriptConfig.
 void readLinkerScript(MemoryBufferRef MB);
 
+void parseVersionScript(MemoryBufferRef MB);
+
 // This enum is used to implement linker script SECTIONS command.
 // https://sourceware.org/binutils/docs/ld/SECTIONS.html#SECTIONS
 enum SectionsCommandKind {
   AssignmentKind,
   OutputSectionKind,
   InputSectionKind,
   AssertKind
 };

ELF/LinkerScript.cpp

size_t LinkerScript<ELFT>::getPhdrIndex(StringRef PhdrName) {
   error("section header '" + PhdrName + "' is not listed in PHDRS");
   return 0;
 }
 
 class elf::ScriptParser : public ScriptParserBase {
   typedef void (ScriptParser::*Handler)();
 
 public:
   ScriptParser(StringRef S, bool B) : ScriptParserBase(S), IsUnderSysroot(B) {}
 
-  void run();
+  void parseLinkerScript();
+  void parseVersionScript();
 
 private:
   void addFile(StringRef Path);
 
   void readAsNeeded();
   void readEntry();
   void readExtern();
   void readGroup();
private:
   Expr readAssert();
 
   Expr readExpr();
   Expr readExpr1(Expr Lhs, int MinPrec);
   Expr readPrimary();
   Expr readTernary(Expr Cond);
   Expr readParenExpr();
 
+  // For parsing version script.
+  void readExtern(std::vector<SymbolVersion> *Globals);
+  void readVersion(StringRef VerStr);
+  void readGlobal(StringRef VerStr);
+  void readLocal();
+
   const static StringMap<Handler> Cmd;
   ScriptConfiguration &Opt = *ScriptConfig;
   StringSaver Saver = {ScriptConfig->Alloc};
   bool IsUnderSysroot;
 };
 
 const StringMap<elf::ScriptParser::Handler> elf::ScriptParser::Cmd = {
     {"ENTRY", &ScriptParser::readEntry},
     {"EXTERN", &ScriptParser::readExtern},
     {"GROUP", &ScriptParser::readGroup},
     {"INCLUDE", &ScriptParser::readInclude},
     {"INPUT", &ScriptParser::readGroup},
     {"OUTPUT", &ScriptParser::readOutput},
     {"OUTPUT_ARCH", &ScriptParser::readOutputArch},
     {"OUTPUT_FORMAT", &ScriptParser::readOutputFormat},
     {"PHDRS", &ScriptParser::readPhdrs},
     {"SEARCH_DIR", &ScriptParser::readSearchDir},
     {"SECTIONS", &ScriptParser::readSections},
     {";", &ScriptParser::readNothing}};
 
-void ScriptParser::run() {
+void ScriptParser::parseVersionScript() {
+  StringRef Msg = "anonymous version definition is used in "
+                  "combination with other version definitions";
+  if (skip("{")) {
+    readVersion("");
+    if (!atEOF())
+      setError(Msg);
+    return;
+  }
+
+  while (!atEOF() && !Error) {
+    StringRef VerStr = next();
+    if (VerStr == "{") {
+      setError(Msg);
+      return;
+    }
+    expect("{");
+    readVersion(VerStr);
+  }
+}
+
+void ScriptParser::parseLinkerScript() {
   while (!atEOF()) {
     StringRef Tok = next();
     if (Handler Fn = Cmd.lookup(Tok))
       (this->*Fn)();
     else if (SymbolAssignment *Cmd = readProvideOrAssignment(Tok))
       Opt.Commands.emplace_back(Cmd);
     else
       setError("unknown directive: " + Tok);
unsigned ScriptParser::readPhdrType() {
 
   if (Ret == (unsigned)-1) {
     setError("invalid program header type: " + Tok);
     return PT_NULL;
   }
   return Ret;
 }
 
+void ScriptParser::readVersion(StringRef VerStr) {
+  // Identifiers start at 2 because 0 and 1 are reserved
+  // for VER_NDX_LOCAL and VER_NDX_GLOBAL constants.
+  size_t VersionId = Config->VersionDefinitions.size() + 2;
+  Config->VersionDefinitions.push_back({VerStr, VersionId});
+
+  if (skip("global:") || peek() != "local:")
+    readGlobal(VerStr);
+  if (skip("local:"))
+    readLocal();
+  expect("}");
+
+  // Each version may have a parent version. For example, "Ver2" defined as
+  // "Ver2 { global: foo; local: *; } Ver1;" has "Ver1" as a parent. This
+  // version hierarchy is, probably against your instinct, purely for human; the
+  // runtime doesn't care about them at all. In LLD, we simply skip the token.
+  if (!VerStr.empty() && peek() != ";")
+    next();
+  expect(";");
+}
+
+void ScriptParser::readLocal() {
+  Config->DefaultSymbolVersion = VER_NDX_LOCAL;
+  expect("*");
+  expect(";");
+}
+
+void ScriptParser::readExtern(std::vector<SymbolVersion> *Globals) {
+  expect("C++");
+  expect("{");
+
+  for (;;) {
+    if (peek() == "}" || Error)
+      break;
+    Globals->push_back({next(), true});
+    expect(";");
+  }
+
+  expect("}");
+  expect(";");
+}
+
+void ScriptParser::readGlobal(StringRef VerStr) {
+  std::vector<SymbolVersion> *Globals;
+  if (VerStr.empty())
+    Globals = &Config->VersionScriptGlobals;
+  else
+    Globals = &Config->VersionDefinitions.back().Globals;
+
+  for (;;) {
+    if (skip("extern"))
+      readExtern(Globals);
+
+    StringRef Cur = peek();
+    if (Cur == "}" || Cur == "local:" || Error)
+      return;
+    next();
+    Globals->push_back({Cur, false});
+    expect(";");
+  }
+}
+
 static bool isUnderSysroot(StringRef Path) {
   if (Config->Sysroot == "")
     return false;
   for (; !Path.empty(); Path = sys::path::parent_path(Path))
     if (sys::fs::equivalent(Config->Sysroot, Path))
       return true;
   return false;
 }
 
 // Entry point.
 void elf::readLinkerScript(MemoryBufferRef MB) {
   StringRef Path = MB.getBufferIdentifier();
-  ScriptParser(MB.getBuffer(), isUnderSysroot(Path)).run();
+  ScriptParser(MB.getBuffer(), isUnderSysroot(Path)).parseLinkerScript();
+}
+
+// Parse the --version-script argument. We currently only accept the following
+// version script syntax:
+//
+//  { [ global: symbol1; symbol2; [...]; symbolN; ] local: *; };
+//
+void elf::parseVersionScript(MemoryBufferRef MB) {
+  ScriptParser(MB.getBuffer(), false).parseVersionScript();
 }
 
 template class elf::LinkerScript<ELF32LE>;
 template class elf::LinkerScript<ELF32BE>;
 template class elf::LinkerScript<ELF64LE>;
 template class elf::LinkerScript<ELF64BE>;

ELF/SymbolListFile.h

 
 #include "lld/Core/LLVM.h"
 #include "llvm/Support/MemoryBuffer.h"
 
 namespace lld {
 namespace elf {
 
 void parseDynamicList(MemoryBufferRef MB);
-void parseVersionScript(MemoryBufferRef MB);
 
 } // namespace elf
 } // namespace lld
 
 #endif

ELF/SymbolListFile.cpp

while (!atEOF()) {
     }
     expect(";");
   }
 }
 
 void elf::parseDynamicList(MemoryBufferRef MB) {
   DynamicListParser(MB.getBuffer()).run();
 }
-
-// Parse the --version-script argument. We currently only accept the following
-// version script syntax:
-//
-//  { [ global: symbol1; symbol2; [...]; symbolN; ] local: *; };
-//
-// No wildcards are supported, other than for the local entry. Symbol versioning
-// is also not supported.
-
-namespace {
-class VersionScriptParser final : public ScriptParserBase {
-public:
-  VersionScriptParser(StringRef S) : ScriptParserBase(S) {}
-
-  void run();
-
-private:
-  void parseExtern(std::vector<SymbolVersion> *Globals);
-  void parseVersion(StringRef VerStr);
-  void parseGlobal(StringRef VerStr);
-  void parseLocal();
-};
-} // end anonymous namespace
-
-void VersionScriptParser::parseVersion(StringRef VerStr) {
-  // Identifiers start at 2 because 0 and 1 are reserved
-  // for VER_NDX_LOCAL and VER_NDX_GLOBAL constants.
-  size_t VersionId = Config->VersionDefinitions.size() + 2;
-  Config->VersionDefinitions.push_back({VerStr, VersionId});
-
-  if (skip("global:") || peek() != "local:")
-    parseGlobal(VerStr);
-  if (skip("local:"))
-    parseLocal();
-  expect("}");
-
-  // Each version may have a parent version. For example, "Ver2" defined as
-  // "Ver2 { global: foo; local: *; } Ver1;" has "Ver1" as a parent. This
-  // version hierarchy is, probably against your instinct, purely for human; the
-  // runtime doesn't care about them at all. In LLD, we simply skip the token.
-  if (!VerStr.empty() && peek() != ";")
-    next();
-  expect(";");
-}
-
-void VersionScriptParser::parseLocal() {
-  Config->DefaultSymbolVersion = VER_NDX_LOCAL;
-  expect("*");
-  expect(";");
-}
-
-void VersionScriptParser::parseExtern(std::vector<SymbolVersion> *Globals) {
-  expect("C++");
-  expect("{");
-
-  for (;;) {
-    if (peek() == "}" || Error)
-      break;
-    Globals->push_back({next(), true});
-    expect(";");
-  }
-
-  expect("}");
-  expect(";");
-}
-
-void VersionScriptParser::parseGlobal(StringRef VerStr) {
-  std::vector<SymbolVersion> *Globals;
-  if (VerStr.empty())
-    Globals = &Config->VersionScriptGlobals;
-  else
-    Globals = &Config->VersionDefinitions.back().Globals;
-
-  for (;;) {
-    if (skip("extern"))
-      parseExtern(Globals);
-
-    StringRef Cur = peek();
-    if (Cur == "}" || Cur == "local:" || Error)
-      return;
-    next();
-    Globals->push_back({Cur, false});
-    expect(";");
-  }
-}
-
-void VersionScriptParser::run() {
-  StringRef Msg = "anonymous version definition is used in "
-                  "combination with other version definitions";
-  if (skip("{")) {
-    parseVersion("");
-    if (!atEOF())
-      setError(Msg);
-    return;
-  }
-
-  while (!atEOF() && !Error) {
-    StringRef VerStr = next();
-    if (VerStr == "{") {
-      setError(Msg);
-      return;
-    }
-    expect("{");
-    parseVersion(VerStr);
-  }
-}
-
-void elf::parseVersionScript(MemoryBufferRef MB) {
-  VersionScriptParser(MB.getBuffer()).run();
-}