[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

583–586 ↗

(On Diff #68865)

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

626–629 ↗

(On Diff #68865)

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

1389 ↗

(On Diff #68865)

Remove this empty line.

[grimar]

• Addressed review comments.

[grimar]

ELF/LinkerScript.cpp

583–586 ↗

(On Diff #68865)

Done.

626–629 ↗

(On Diff #68865)

Done.

1389 ↗

(On Diff #68865)

Done.

[• rafael]

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

ELF/LinkerScript.cpp

593–596 ↗

(On Diff #69678)

Given the other names, maybe readLinkerScript and readVersionScript?

1411 ↗

(On Diff #69678)

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

[ruiu]

LGTM

ELF/LinkerScript.cpp

1411 ↗

(On Diff #69678)

Yeah, it is I think out of date.

1415 ↗

(On Diff #69678)

parse -> read for the sake of consistency.

lld/trunk/ELF/Driver.cpp

if (auto *Arg = Args.getLastArg(OPT_dynamic_list))
     if (Optional<MemoryBufferRef> Buffer = readFile(Arg->getValue()))
       parseDynamicList(*Buffer);
 
   for (auto *Arg : Args.filtered(OPT_export_dynamic_symbol))
     Config->DynamicList.push_back(Arg->getValue());
 
   if (auto *Arg = Args.getLastArg(OPT_version_script))
     if (Optional<MemoryBufferRef> Buffer = readFile(Arg->getValue()))
-      parseVersionScript(*Buffer);
+      readVersionScript(*Buffer);
 }
 
 void LinkerDriver::createFiles(opt::InputArgList &Args) {
   for (auto *Arg : Args) {
     switch (Arg->getOption().getID()) {
     case OPT_l:
       addLibrary(Arg->getValue());
       break;

lld/trunk/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 readVersionScript(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
 };

lld/trunk/ELF/LinkerScript.cpp

 }
 
 class elf::ScriptParser : public ScriptParserBase {
   typedef void (ScriptParser::*Handler)();
 
 public:
   ScriptParser(StringRef S, bool B) : ScriptParserBase(S), IsUnderSysroot(B) {}
 
-  void run();
+  void readLinkerScript();
+  void readVersionScript();
 
 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::readVersionScript() {
+  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::readLinkerScript() {
   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)).readLinkerScript();
+}
+
+void elf::readVersionScript(MemoryBufferRef MB) {
+  ScriptParser(MB.getBuffer(), false).readVersionScript();
 }
 
 template class elf::LinkerScript<ELF32LE>;
 template class elf::LinkerScript<ELF32BE>;
 template class elf::LinkerScript<ELF64LE>;
 template class elf::LinkerScript<ELF64BE>;

lld/trunk/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

lld/trunk/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();
-}