[ELF] Better error reporting for linker scripts

[evgeny777]

Summary

This patch shows both file and line for linker script errors, including:

• Errors occurred in lexer (ScriptParserBase::tokenize)
• Errors occurred in included files (via INCLUDE directive)

See updated test cases in diagnostics.s

[grimar]

ELF/LinkerScript.cpp

1154

In one of my latest patches I found that it is confusing to have multiple lines in output that are starting from "error: xxx".
I think we should not call error() for more than a single error at once.

1173

<removed (have some issue with applying del on comment in phab, it does not want to do that. I have no time to wait until it do, so had to edit in that way)>

[evgeny777]

ELF/LinkerScript.cpp

1154

Here multiple lines are used to highlight error position, which might be useful, especially when you run linker from command line. I would keep this bearing in mind that we already have tests for it.

[ruiu]

I wonder if we can remove INCLUDE directive from the linker script. I added that without thinking about it, but I've never seen any use of it. Linker scripts that add other linker scripts usually add that with INPUT() directive.

[evgeny777]

This is a question, I cannot answer :). You can't replace INCLUDE with INPUT and vice versa, but if you remove it then this patch will be much simpler.

[ruiu]

Let's try to remove it to see if we actually need it. Yeah, courage.

[evgeny777]

Looks like I found few projects which use INCLUDE directive:

http://libopencm3.org/wiki/Run_From_RAM
https://github.com/cobyism/edimax-br-6528n/blob/master/AP/mkimg/RTL8196C_1200_tools/libstrip/libstrip
https://github.com/RIOT-OS/RIOT/blob/master/cpu/stm32f4/ldscripts/stm32f415rg.ld
https://chromium.googlesource.com/chromiumos/third_party/coreboot/+/firmware-uboot_v2-1299.B/src/arch/x86/Makefile.bootblock.inc

The last one seems to be a chromebook boot loader: it includes script files in auto-generated linker script ldscript.ld

I don't know how important is to support those in lld, but please confirm that you want to get rid of INCLUDE

[ruiu]

I'm not speaking for Google, but Chromebook boot loader is probably a large user we can't ignore, so I'm inclined to not remove INCLUDE. I'll review your patch tomorrow.

[evgeny777]

Simplified and add test case

[• rafael]

ELF/LinkerScript.cpp

1020

That is pretty much what a MemomryBufferRef is. Can't you store that instead of the std::pair?

[ruiu]

ELF/LinkerScript.cpp

1131

Currently, error reporting is zero-cost if there's no error (we find an error location after an error is raised), and that seemed to be a nice hack to use a StringRef's pointer to find a location. However, it didn't work well for INCLUDE as you know. I think we don't want to keep the hack that's proved to be working not well.

Linker scripts are small, so it is OK to do more and use more memory when tokenizing them.

How about this? We can change tokenize to return not only tokens but token locations as a parallel array, like this.

std::pair<std::vector<StringRef>, std::vector<std::string>>
tokenize(MemoryBufferRef MB);

ScriptParser class then store the vectors to Tokens and LineNo.

[evgeny777]

ScriptParser class then store the vectors to Tokens and LineNo.

You also have to store file name for each token and I think this is too much. How about storing a vector of MemoryBufferRef instead of 'Input' in ScriptParserBase? Please take a look at the updated diff.

[evgeny777]

Moved file management to ScriptParserBase. Now errors in version scripts and dynamic lists are handled in a new way as well

[ruiu]

It's much better than before, but I still think that we could do more by using parallel arrays. We want to maintain four tuples (filename, line content, line number, column number) for each token, and all of them are small (they are intetgers and StringRefs). So I think it's not going to be too much.

That being said, we can do that later. It is indeed a very good improvement. Thanks!

LGTM

ELF/ScriptParser.cpp

161

I'd rather just use Tokens[Pos - 1] instead of current() everywhere because this function is used from those who already know internals of this class.

ELF/ScriptParser.h

41

Make this private.

45

Ditto

ELF/LinkerScript.cpp

for (PhdrsCommand &Cmd : Opt.PhdrsCommands) {
     if (Cmd.Name == PhdrName)
       return I;
     ++I;
   }
   error("section header '" + PhdrName + "' is not listed in PHDRS");
   return 0;
 }
 
-class elf::ScriptParser : public ScriptParserBase {
+class elf::ScriptParser final : public ScriptParserBase {
   typedef void (ScriptParser::*Handler)();
 
 public:
-  ScriptParser(StringRef S, bool B) : ScriptParserBase(S), IsUnderSysroot(B) {}
+  ScriptParser(MemoryBufferRef MB, bool B) : IsUnderSysroot(B) {
+    addScriptFile(MB.getBufferIdentifier(), MB.getBuffer());
+  }
 
   void readLinkerScript();
   void readVersionScript();
 
 private:
   void addFile(StringRef Path);
+  void addScriptFile(StringRef Path, StringRef Data);
 
   void readAsNeeded();
   void readEntry();
   void readExtern();
   void readGroup();
   void readInclude();
   void readOutput();
   void readOutputArch();
private:
   Expr readParenExpr();
 
   // For parsing version script.
   std::vector<SymbolVersion> readVersionExtern();
   void readAnonymousDeclaration();
   void readVersionDeclaration(StringRef VerStr);
   std::vector<SymbolVersion> readSymbols();
 
+  void setError(const Twine &Msg) override;
+
   ScriptConfiguration &Opt = *ScriptConfig;
   bool IsUnderSysroot;
+
+  // Array of {File_Name, File_Data} pairs used for
+  // error reporting.
+  std::vector<std::pair<StringRef, StringRef>> Files;
 };
 
 void ScriptParser::readVersionScript() {
   readVersionScriptCommand();
   if (!atEOF())
     setError("EOF expected, but got " + next());
 }
 
if (sys::path::is_absolute(S)) {
     std::string Path = findFromSearchPaths(S);
     if (Path.empty())
       setError("unable to find " + S);
     else
       Driver->addFile(Saver.save(Path));
   }
 }
 
+// Returns true if string 'Bigger' contains string 'Shorter'.
+static bool containsString(StringRef Bigger, StringRef Shorter) {
+  const char *BiggerEnd = Bigger.data() + Bigger.size();
+  const char *ShorterEnd = Shorter.data() + Shorter.size();
+
+  return Bigger.data() <= Shorter.data() && BiggerEnd >= ShorterEnd;
+}
+
+void ScriptParser::setError(const Twine &Msg) {
+  if (Error)
+    return;
+
+  assert(!Files.empty());
+  Error = true;
+  if (!Pos) {
+    error(Files.front().first + " (1): " + Msg);
+    return;
+  }
+
+  // Find piece of input data which contains token which has
+  // raised an error. When found report file name and line
+  // number.
+  for (auto &F : Files)
+    if (containsString(F.second, current())) {
+      // If we have valid token we can calculate line and column
+      // numbers to provide better error diagnostics.
+      StringRef Line = getTokenLine(F.second, current());
+      auto ErrLoc = getTokenLocation(F.second, current());
+      const Twine &Location = F.first + " (" + Twine(ErrLoc.first) + "): ";
+      error(Location + Msg);
+      error(Location + Line);
+      error(Location + std::string(ErrLoc.second, ' ') + "^");
+      break;
+    }
+}
+
+// Tokenizes script file and records file name and data for error
+// reporting.
+void ScriptParser::addScriptFile(StringRef Path, StringRef Data) {
+  std::vector<StringRef> V = tokenize(Path, Data);
+  Tokens.insert(Tokens.begin() + Pos, V.begin(), V.end());
+
+  Files.emplace_back(Path, Data);
+}
+
 void ScriptParser::readAsNeeded() {
   expect("(");
   bool Orig = Config->AsNeeded;
   Config->AsNeeded = true;
   while (!Error && !consume(")"))
     addFile(unquote(next()));
   Config->AsNeeded = Orig;
 }
 
 void ScriptParser::readEntry() {
   // -e <symbol> takes predecence over ENTRY(<symbol>).
   expect("(");
   StringRef Tok = next();
   if (Config->Entry.empty())
void ScriptParser::readInclude() {
   StringRef Tok = next();
   auto MBOrErr = MemoryBuffer::getFile(unquote(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());
+  addScriptFile(unquote(Tok), S);
 }
 
 void ScriptParser::readOutput() {
   // -o <file> takes predecence over OUTPUT(<file>).
   expect("(");
   StringRef Tok = next();
   if (Config->OutputFile.empty())
     Config->OutputFile = unquote(Tok);
static bool isUnderSysroot(StringRef Path) {
   for (; !Path.empty(); Path = sys::path::parent_path(Path))
     if (sys::fs::equivalent(Config->Sysroot, Path))
       return true;
   return false;
 }
 
 void elf::readLinkerScript(MemoryBufferRef MB) {
   StringRef Path = MB.getBufferIdentifier();
-  ScriptParser(MB.getBuffer(), isUnderSysroot(Path)).readLinkerScript();
+  ScriptParser(MB, isUnderSysroot(Path)).readLinkerScript();
 }
 
 void elf::readVersionScript(MemoryBufferRef MB) {
-  ScriptParser(MB.getBuffer(), false).readVersionScript();
+  ScriptParser(MB, false).readVersionScript();
 }
 
 template class elf::LinkerScript<ELF32LE>;
 template class elf::LinkerScript<ELF32BE>;
 template class elf::LinkerScript<ELF64LE>;
 template class elf::LinkerScript<ELF64BE>;

ELF/ScriptParser.h

 #include "lld/Core/LLVM.h"
 #include "llvm/ADT/StringRef.h"
 #include <utility>
 #include <vector>
 
 namespace lld {
 namespace elf {
 
+// Gets line containing a token.
+StringRef getTokenLine(StringRef Data, StringRef Token);
+
+// Gets token line and column as std::pair.
+std::pair<size_t, size_t> getTokenLocation(StringRef Data, StringRef Tok);
+
 class ScriptParserBase {
 public:
-  explicit ScriptParserBase(StringRef S) : Input(S), Tokens(tokenize(S)) {}
+  ScriptParserBase() = default;
+  explicit ScriptParserBase(StringRef S);
 
 protected:
-  void setError(const Twine &Msg);
-  static std::vector<StringRef> tokenize(StringRef S);
+  ~ScriptParserBase() = default;
+
+  virtual void setError(const Twine &Msg);
+  static std::vector<StringRef> tokenize(StringRef Filename, StringRef S);
   static StringRef skipSpace(StringRef S);
   bool atEOF();
   StringRef next();
   StringRef peek();
+  StringRef current();
   void skip();
   bool consume(StringRef Tok);
   void expect(StringRef Expect);
 
-  size_t getPos();
-  void printErrorPos();
-
   StringRef Input;
   std::vector<StringRef> Tokens;
   size_t Pos = 0;
   bool Error = false;
 };
 
 } // namespace elf
 } // namespace lld
 
 #endif

ELF/ScriptParser.cpp

 #include "ScriptParser.h"
 #include "Error.h"
 #include "llvm/ADT/Twine.h"
 
 using namespace llvm;
 using namespace lld;
 using namespace lld::elf;
 
-// Returns the line that the character S[Pos] is in.
-static StringRef getLine(StringRef S, size_t Pos) {
+// Returns the line that the token Tok is in.
+StringRef elf::getTokenLine(StringRef S, StringRef Tok) {
+  size_t Pos = Tok.data() - S.data();
   size_t Begin = S.rfind('\n', Pos);
   size_t End = S.find('\n', Pos);
   Begin = (Begin == StringRef::npos) ? 0 : Begin + 1;
   if (End == StringRef::npos)
     End = S.size();
   // rtrim for DOS-style newlines.
   return S.substr(Begin, End - Begin).rtrim();
 }
 
-void ScriptParserBase::printErrorPos() {
-  StringRef Tok = Tokens[Pos == 0 ? 0 : Pos - 1];
-  StringRef Line = getLine(Input, Tok.data() - Input.data());
+// Returns the current line number.
+static size_t getLineNo(StringRef Data, StringRef Tok) {
+  return StringRef(Data.data(), Tok.data() - Data.data()).count('\n') + 1;
+}
+
+std::pair<size_t, size_t> elf::getTokenLocation(StringRef Data, StringRef Tok) {
+  StringRef Line = getTokenLine(Data, Tok);
+  return {getLineNo(Data, Tok), Tok.data() - Line.data()};
+}
+
+static void printErrorPos(StringRef Input, StringRef Tok) {
+  StringRef Line = getTokenLine(Input, Tok);
   size_t Col = Tok.data() - Line.data();
   error(Line);
   error(std::string(Col, ' ') + "^");
 }
 
+ScriptParserBase::ScriptParserBase(StringRef S)
+    : Input(S), Tokens(tokenize(StringRef(), S)) {}
+
 // We don't want to record cascading errors. Keep only the first one.
 void ScriptParserBase::setError(const Twine &Msg) {
   if (Error)
     return;
-  if (Input.empty() || Tokens.empty()) {
+  if (Pos == 0) {
     error(Msg);
   } else {
-    error("line " + Twine(getPos()) + ": " + Msg);
-    printErrorPos();
+    error("line " + Twine(getLineNo(Input, current())) + ": " + Msg);
+    printErrorPos(Input, current());
   }
   Error = true;
 }
 
 // Split S into linker script tokens.
-std::vector<StringRef> ScriptParserBase::tokenize(StringRef S) {
+std::vector<StringRef> ScriptParserBase::tokenize(StringRef Filename,
+                                                  StringRef S) {
   std::vector<StringRef> Ret;
+  StringRef Data = S;
   for (;;) {
     S = skipSpace(S);
     if (S.empty())
       return Ret;
 
     // Quoted token. Note that double-quote characters are parts of a token
     // because, in a glob match context, only unquoted tokens are interpreted
     // as glob patterns. Double-quoted tokens are literal patterns in that
     // context.
     if (S.startswith("\"")) {
       size_t E = S.find("\"", 1);
       if (E == StringRef::npos) {
+        if (!Filename.empty())
+          error(Filename + " (" + Twine(getLineNo(Data, S)) +
+                "): unclosed quote");
+        else
           error("unclosed quote");
         return {};
       }
       Ret.push_back(S.take_front(E + 1));
       S = S.substr(E + 1);
       continue;
     }
 
     // Unquoted token. This is more relaxed than tokens in C-like language,

 StringRef ScriptParserBase::peek() {
   StringRef Tok = next();
   if (Error)
     return "";
   --Pos;
   return Tok;
 }
 
+StringRef ScriptParserBase::current() {
+  assert(Pos);
+  return Tokens[Pos - 1];
+}
+
 bool ScriptParserBase::consume(StringRef Tok) {
   if (peek() == Tok) {
     skip();
     return true;
   }
   return false;
 }
 
 void ScriptParserBase::skip() { (void)next(); }
 
 void ScriptParserBase::expect(StringRef Expect) {
   if (Error)
     return;
   StringRef Tok = next();
   if (Tok != Expect)
     setError(Expect + " expected, but got " + Tok);
 }
-
-// Returns the current line number.
-size_t ScriptParserBase::getPos() {
-  if (Pos == 0)
-    return 1;
-  const char *Begin = Input.data();
-  const char *Tok = Tokens[Pos - 1].data();
-  return StringRef(Begin, Tok - Begin).count('\n') + 1;
-}

test/ELF/linkerscript/diagnostic.s

 ## message starts from correct line number:
 # RUN: echo "SECTIONS {" > %t.script
 # RUN: echo ".text + { *(.text) }" >> %t.script
 # RUN: echo ".keep : { *(.keep) } /*" >> %t.script
 # RUN: echo "comment line 1" >> %t.script
 # RUN: echo "comment line 2 */" >> %t.script
 # RUN: echo ".temp : { *(.temp) } }" >> %t.script
 # RUN: not ld.lld -shared %t -o %t1 --script %t.script 2>&1 | FileCheck -check-prefix=ERR1 %s
-# ERR1: line 2:
+# ERR1: {{.*}}.script (2):
 
 ## Change ":" to "+" at line 3 now, check correct error line number:
 # RUN: echo "SECTIONS {" > %t.script
 # RUN: echo ".text : { *(.text) }" >> %t.script
 # RUN: echo ".keep + { *(.keep) } /*" >> %t.script
 # RUN: echo "comment line 1" >> %t.script
 # RUN: echo "comment line 2 */" >> %t.script
 # RUN: echo ".temp : { *(.temp) } }" >> %t.script
 # RUN: not ld.lld -shared %t -o %t1 --script %t.script 2>&1 | FileCheck -check-prefix=ERR2 %s
-# ERR2: line 3:
+# ERR2: {{.*}}.script (3):
 
 ## Change ":" to "+" at line 6, after multiline comment,
 ## check correct error line number:
 # RUN: echo "SECTIONS {" > %t.script
 # RUN: echo ".text : { *(.text) }" >> %t.script
 # RUN: echo ".keep : { *(.keep) } /*" >> %t.script
 # RUN: echo "comment line 1" >> %t.script
 # RUN: echo "comment line 2 */" >> %t.script
 # RUN: echo ".temp + { *(.temp) } }" >> %t.script
 # RUN: not ld.lld -shared %t -o %t1 --script %t.script 2>&1 | FileCheck -check-prefix=ERR5 %s
-# ERR5: line 6:
+# ERR5: {{.*}}.script (6):
 
 ## Check that text of lines and pointer to 'bad' token are working ok.
 # RUN: echo "UNKNOWN_TAG {" > %t.script
 # RUN: echo ".text : { *(.text) }" >> %t.script
 # RUN: echo ".keep : { *(.keep) }" >> %t.script
 # RUN: echo ".temp : { *(.temp) } }" >> %t.script
 # RUN: not ld.lld -shared %t -o %t1 --script %t.script 2>&1 | \
 # RUN:   FileCheck -check-prefix=ERR6 -strict-whitespace %s
-# ERR6:      error: line 1:
-# ERR6-NEXT: error: UNKNOWN_TAG {
-# ERR6-NEXT: error: ^
+# ERR6:      error: {{.*}}.script (1):
+# ERR6-NEXT: error: {{.*}}.script (1): UNKNOWN_TAG {
+# ERR6-NEXT: error: {{.*}}.script (1): ^
 
 ## One more check that text of lines and pointer to 'bad' token are working ok.
 # RUN: echo "SECTIONS {" > %t.script
 # RUN: echo ".text : { *(.text) }" >> %t.script
 # RUN: echo ".keep : { *(.keep) }" >> %t.script
 # RUN: echo "boom .temp : { *(.temp) } }" >> %t.script
 # RUN: not ld.lld -shared %t -o %t1 --script %t.script 2>&1 | \
 # RUN:   FileCheck -check-prefix=ERR7 -strict-whitespace %s
-# ERR7:      error: line 4: malformed number: .temp
-# ERR7-NEXT: error: boom .temp : { *(.temp) } }
-# ERR7-NEXT: error:      ^
+# ERR7:      error: {{.*}}.script (4): malformed number: .temp
+# ERR7-NEXT: error: {{.*}}.script (4): boom .temp : { *(.temp) } }
+# ERR7-NEXT: error: {{.*}}.script (4):      ^
+
+## Check tokenize() error
+# RUN: echo "SECTIONS {}" > %t.script
+# RUN: echo "\"" >> %t.script
+# RUN: not ld.lld -shared %t -o %t1 --script %t.script 2>&1 | \
+# RUN:   FileCheck -check-prefix=ERR8 -strict-whitespace %s
+# ERR8: {{.*}}.script (2): unclosed quote
+
+## Check tokenize() error in included script file
+# RUN: echo "SECTIONS {}" > %t.script.inc
+# RUN: echo "\"" >> %t.script.inc
+# RUN: echo "INCLUDE \"%t.script.inc\"" > %t.script
+# RUN: not ld.lld -shared %t -o %t1 --script %t.script 2>&1 | \
+# RUN:   FileCheck -check-prefix=ERR9 -strict-whitespace %s
+# ERR9: {{.*}}.script.inc (2): unclosed quote
+
+## Check error reporting correctness for included files.
+# RUN: echo "SECTIONS {" > %t.script.inc
+# RUN: echo ".text : { *(.text) }" >> %t.script.inc
+# RUN: echo ".keep : { *(.keep) }" >> %t.script.inc
+# RUN: echo "boom .temp : { *(.temp) } }" >> %t.script.inc
+# RUN: echo "INCLUDE \"%t.script.inc\"" > %t.script
+# RUN: not ld.lld -shared %t -o %t1 --script %t.script 2>&1 | \
+# RUN:   FileCheck -check-prefix=ERR10 -strict-whitespace %s
+# ERR10:      error: {{.*}}.script.inc (4): malformed number: .temp
+# ERR10-NEXT: error: {{.*}}.script.inc (4): boom .temp : { *(.temp) } }
+# ERR10-NEXT: error: {{.*}}.script.inc (4):      ^
+
+## Check error reporting in script with INCLUDE directive.
+# RUN: echo "SECTIONS {" > %t.script.inc
+# RUN: echo ".text : { *(.text) }" >> %t.script.inc
+# RUN: echo ".keep : { *(.keep) }" >> %t.script.inc
+# RUN: echo ".temp : { *(.temp) } }" >> %t.script.inc
+# RUN: echo "/* One line before INCLUDE */" > %t.script
+# RUN: echo "INCLUDE \"%t.script.inc\"" >> %t.script
+# RUN: echo "/* One line ater INCLUDE */" >> %t.script
+# RUN: echo "Error" >> %t.script
+# RUN: not ld.lld -shared %t -o %t1 --script %t.script 2>&1 | \
+# RUN:   FileCheck -check-prefix=ERR11 -strict-whitespace %s
+# ERR11: error: {{.*}}.script (4): unexpected EOF