ELF: Do not use fatal in LinkerScript.cpp.

[ruiu]

Summary

Propagating errors back to callers is annoying in recursive descendent
parser because they are naturally constructed as mutually recursive
functions. In this patch, I took a simple approach. The accessors of
the token stream behaves as if they are at EOF once we saw any errors.
It naturally makes all functions to return.

This is the final change to not use fatal in the linker.

[dblaikie]

Totally a casual comment & I could be totally off: Parser error recovery
can be really helpful to users (eg: Clang's error recovery), though (much
like using TLS for the general error handling improvements, and my comments
there) I expect this is a good first step and better error recovery can be
added over time to improve the user experience here.

As you've said, it's certainly not a terribly easy thing to do great error
recovery, and I wouldn't suggest trying to solve everything up-front, for
sure.

[• rafael]

testcase?

[ruiu]

• Added test cases and fixed a few bugs found by the tests.

[• rafael]

LGTM

test/ELF/invalid-linkerscript.test

6

Add a comment saying all the "no input files" are here just to show that the parser gives up, but the linker keeps going when an error is found.

ELF/LinkerScript.cpp

 namespace {
 class LinkerScript {
 public:
   LinkerScript(BumpPtrAllocator *A, StringRef S, bool B)
       : Saver(*A), Tokens(tokenize(S)), IsUnderSysroot(B) {}
   void run();
 
 private:
+  void setError(const Twine &Msg);
   static std::vector<StringRef> tokenize(StringRef S);
   static StringRef skipSpace(StringRef S);
+  bool atEOF();
   StringRef next();
   bool skip(StringRef Tok);
-  bool atEOF() { return Tokens.size() == Pos; }
   void expect(StringRef Expect);
 
   void addFile(StringRef Path);
 
   void readAsNeeded();
   void readEntry();
   void readExtern();
   void readGroup();
   void readInclude();
   void readOutput();
   void readOutputArch();
   void readOutputFormat();
   void readSearchDir();
   void readSections();
 
   void readOutputSectionDescription();
 
   StringSaver Saver;
   std::vector<StringRef> Tokens;
+  bool Error = false;
   size_t Pos = 0;
   bool IsUnderSysroot;
 };
 }
 
 void LinkerScript::run() {
   while (!atEOF()) {
     StringRef Tok = next();
if (Tok == "ENTRY") {
       readOutputArch();
     } else if (Tok == "OUTPUT_FORMAT") {
       readOutputFormat();
     } else if (Tok == "SEARCH_DIR") {
       readSearchDir();
     } else if (Tok == "SECTIONS") {
       readSections();
     } else {
-      fatal("unknown directive: " + Tok);
+      setError("unknown directive: " + Tok);
+      return;
     }
   }
 }
 
+// We don't want to record cascading errors. Keep only the first one.
+void LinkerScript::setError(const Twine &Msg) {
+  if (Error)
+    return;
+  error(Msg);
+  Error = true;
+}
+
 // Split S into linker script tokens.
 std::vector<StringRef> LinkerScript::tokenize(StringRef S) {
   std::vector<StringRef> Ret;
   for (;;) {
     S = skipSpace(S);
     if (S.empty())
       return Ret;
 
     // Quoted token
     if (S.startswith("\"")) {
       size_t E = S.find("\"", 1);
-      if (E == StringRef::npos)
-        fatal("unclosed quote");
+      if (E == StringRef::npos) {
+        error("unclosed quote");
+        return {};
+      }
       Ret.push_back(S.substr(1, E - 1));
       S = S.substr(E + 1);
       continue;
     }
 
     // Unquoted token
     size_t Pos = S.find_first_not_of(
         "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
         "0123456789_.$/\\~=+[]*?-:");
     // A character that cannot start a word (which is usually a
     // punctuation) forms a single character token.
     if (Pos == 0)
       Pos = 1;
     Ret.push_back(S.substr(0, Pos));
     S = S.substr(Pos);
   }
 }
 
 // Skip leading whitespace characters or /**/-style comments.
 StringRef LinkerScript::skipSpace(StringRef S) {
   for (;;) {
     if (S.startswith("/*")) {
       size_t E = S.find("*/", 2);
-      if (E == StringRef::npos)
-        fatal("unclosed comment in a linker script");
+      if (E == StringRef::npos) {
+        error("unclosed comment in a linker script");
+        return "";
+      }
       S = S.substr(E + 2);
       continue;
     }
     size_t Size = S.size();
     S = S.ltrim();
     if (S.size() == Size)
       return S;
   }
 }
 
+// An errneous token is handled as if it were the last token before EOF.
+bool LinkerScript::atEOF() { return Error || Tokens.size() == Pos; }
+
 StringRef LinkerScript::next() {
-  if (atEOF())
-    fatal("unexpected EOF");
+  if (Error)
+    return "";
+  if (atEOF()) {
+    setError("unexpected EOF");
+    return "";
+  }
   return Tokens[Pos++];
 }
 
 bool LinkerScript::skip(StringRef Tok) {
-  if (atEOF())
-    fatal("unexpected EOF");
+  if (Error)
+    return false;
+  if (atEOF()) {
+    setError("unexpected EOF");
+    return false;
+  }
   if (Tok != Tokens[Pos])
     return false;
   ++Pos;
   return true;
 }
 
 void LinkerScript::expect(StringRef Expect) {
+  if (Error)
+    return;
   StringRef Tok = next();
   if (Tok != Expect)
-    fatal(Expect + " expected, but got " + Tok);
+    setError(Expect + " expected, but got " + Tok);
 }
 
 void LinkerScript::addFile(StringRef S) {
   if (IsUnderSysroot && S.startswith("/")) {
     SmallString<128> Path;
     (Config->Sysroot + S).toStringRef(Path);
     if (sys::fs::exists(Path)) {
       Driver->addFile(Saver.save(Path.str()));
else
       Driver->addFile(Saver.save(Config->Sysroot + "/" + S.substr(1)));
   } else if (S.startswith("-l")) {
     Driver->addFile(searchLibrary(S.substr(2)));
   } else if (sys::fs::exists(S)) {
     Driver->addFile(S);
   } else {
     std::string Path = findFromSearchPaths(S);
     if (Path.empty())
-      fatal("Unable to find " + S);
+      setError("Unable to find " + S);
+    else
       Driver->addFile(Saver.save(Path));
   }
 }
 
 void LinkerScript::readAsNeeded() {
   expect("(");
   bool Orig = Config->AsNeeded;
   Config->AsNeeded = true;
-  for (;;) {
+  while (!Error) {
     StringRef Tok = next();
     if (Tok == ")")
       break;
     addFile(Tok);
   }
   Config->AsNeeded = Orig;
 }
 
 void LinkerScript::readEntry() {
   // -e <symbol> takes predecence over ENTRY(<symbol>).
   expect("(");
   StringRef Tok = next();
   if (Config->Entry.empty())
     Config->Entry = Tok;
   expect(")");
 }
 
 void LinkerScript::readExtern() {
   expect("(");
-  for (;;) {
+  while (!Error) {
     StringRef Tok = next();
     if (Tok == ")")
       return;
     Config->Undefined.push_back(Tok);
   }
 }
 
 void LinkerScript::readGroup() {
   expect("(");
-  for (;;) {
+  while (!Error) {
     StringRef Tok = next();
     if (Tok == ")")
       return;
     if (Tok == "AS_NEEDED") {
       readAsNeeded();
       continue;
     }
     addFile(Tok);
   }
 }
 
 void LinkerScript::readInclude() {
   StringRef Tok = next();
   auto MBOrErr = MemoryBuffer::getFile(Tok);
-  fatal(MBOrErr, "cannot open " + Tok);
+  if (!MBOrErr) {
+    setError("cannot open " + Tok);
+    return;
+  }
   std::unique_ptr<MemoryBuffer> &MB = *MBOrErr;
   StringRef S = Saver.save(MB->getMemBufferRef().getBuffer());
   std::vector<StringRef> V = tokenize(S);
   Tokens.insert(Tokens.begin() + Pos, V.begin(), V.end());
 }
 
 void LinkerScript::readOutput() {
   // -o <file> takes predecence over OUTPUT(<file>).

 
 void LinkerScript::readOutputFormat() {
   // Error checking only for now.
   expect("(");
   next();
   StringRef Tok = next();
   if (Tok == ")")
    return;
-  if (Tok != ",")
-    fatal("unexpected token: " + Tok);
+  if (Tok != ",") {
+    setError("unexpected token: " + Tok);
+    return;
+  }
   next();
   expect(",");
   next();
   expect(")");
 }
 
 void LinkerScript::readSearchDir() {
   expect("(");
   Config->SearchPaths.push_back(next());
   expect(")");
 }
 
 void LinkerScript::readSections() {
   expect("{");
-  while (!skip("}"))
+  while (!Error && !skip("}"))
     readOutputSectionDescription();
 }
 
 void LinkerScript::readOutputSectionDescription() {
   StringRef Name = next();
   std::vector<StringRef> &InputSections = Config->OutputSections[Name];
 
   expect(":");
   expect("{");
-  while (!skip("}")) {
+  while (!Error && !skip("}")) {
     next(); // Skip input file name.
     expect("(");
-    while (!skip(")"))
+    while (!Error && !skip(")"))
       InputSections.push_back(next());
   }
 }
 
 static bool isUnderSysroot(StringRef Path) {
   if (Config->Sysroot == "")
     return false;
   for (; !Path.empty(); Path = sys::path::parent_path(Path))

test/ELF/invalid-linkerscript.test

+# RUN: mkdir -p %t.dir
+
+# RUN: echo foobar > %t1
+# RUN: not ld.lld %t1 2>&1 | FileCheck -check-prefix=ERR1 %s
+# ERR1: unknown directive: foobar
+# ERR1: no input files
+
+# RUN: echo "foo \"bar" > %t2
+# RUN: not ld.lld %t2 2>&1 | FileCheck -check-prefix=ERR2 %s
+# ERR2: unclosed quote
+# ERR2: no input files
+
+# RUN: echo "/*" > %t3
+# RUN: not ld.lld %t3 2>&1 | FileCheck -check-prefix=ERR3 %s
+# ERR3: unclosed comment
+# ERR3: no input files
+
+# RUN: echo "EXTERN (" > %t4
+# RUN: not ld.lld %t4 2>&1 | FileCheck -check-prefix=ERR4 %s
+# ERR4: unexpected EOF
+# ERR4: no input files
+
+# RUN: echo "EXTERN (" > %t5
+# RUN: not ld.lld %t5 2>&1 | FileCheck -check-prefix=ERR5 %s
+# ERR5: unexpected EOF
+# ERR5: no input files
+
+# RUN: echo "EXTERN xyz" > %t6
+# RUN: not ld.lld %t6 2>&1 | FileCheck -check-prefix=ERR6 %s
+# ERR6: ( expected, but got xyz
+# ERR6: no input files
+
+# RUN: echo "INCLUDE /no/such/file" > %t7
+# RUN: not ld.lld %t7 2>&1 | FileCheck -check-prefix=ERR7 %s
+# ERR7: cannot open /no/such/file
+# ERR7: no input files
+
+# RUN: echo "OUTPUT_FORMAT(x y z)" > %t8
+# RUN: not ld.lld %t8 2>&1 | FileCheck -check-prefix=ERR8 %s
+# ERR8: unexpected token: y
+# ERR8: no input files