[ELF] - Better diagnostics for linker script errors

[grimar]

Summary

Patch adds "line X:" to output for linker script errors.

It is sometimes difficult to understand what is going on when you
see something like ": expected, but got =".
With this patch that would be: "line XXX: : expected, but got =".

If this is reasonable I can also add the exact line of script to the output
in separate patch if this be landed (so users will not need to search
the line by number by himself).

[emaste]

If you do display the full line and keep track of the column too then the "but got" can be removed from the message.

.keep + { *(.keep) }
      ^
test.script:3:7: error: ':' expected

Having at least the line number for now though would be a good help while testing the in-progress linker script support.

[grimar]

In D18545#385425, @emaste wrote:

If you do display the full line and keep track of the column too then the "but got" can be removed from the message.

.keep + { *(.keep) }
      ^
test.script:3:7: error: ':' expected

Having at least the line number for now though would be a good help while testing the in-progress linker script support.

Yes, that would look better IMO. This patch can put a start for implementation of such messages.

[ruiu]

Please take a look at http://reviews.llvm.org/D18563 (I didn't test it but made up to describe my idea). Because we have a complete string in memory, it is easier to count the number of newlines in string after an error is raised, instead of counting it while parsing the string.

[grimar]

D18563 is better

ELF/LinkerScript.cpp

 
 template <class ELFT> bool SectionRule::match(InputSectionBase<ELFT> *S) {
   return matchStr(SectionPattern, S->getSectionName());
 }
 
 class elf::ScriptParser {
   typedef void (ScriptParser::*Handler)();
 
+  struct Token {
+    StringRef Val;
+    size_t Line;
+  };
+
 public:
   ScriptParser(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 std::vector<Token> tokenize(StringRef S);
   static StringRef skipSpace(StringRef S);
   bool atEOF();
   StringRef next();
   StringRef peek();
   bool skip(StringRef Tok);
   void expect(StringRef Expect);
 
   void addFile(StringRef Path);
private:
   void readSections();
 
   void readOutputSectionDescription();
   void readSectionPatterns(StringRef OutSec, bool Keep);
 
   std::vector<uint8_t> parseHex(StringRef S);
 
   StringSaver Saver;
-  std::vector<StringRef> Tokens;
+  std::vector<Token> Tokens;
   const static StringMap<Handler> Cmd;
   size_t Pos = 0;
+  size_t Line = 0;
   bool IsUnderSysroot;
   bool Error = false;
 };
 
 const StringMap<elf::ScriptParser::Handler> elf::ScriptParser::Cmd = {
     {"ENTRY", &ScriptParser::readEntry},
     {"EXTERN", &ScriptParser::readExtern},
     {"GROUP", &ScriptParser::readGroup},
else
       setError("unknown directive: " + Tok);
   }
 }
 
 // We don't want to record cascading errors. Keep only the first one.
 void ScriptParser::setError(const Twine &Msg) {
   if (Error)
     return;
-  error(Msg);
+  error("line " + Twine(Line) + ": " + Msg);
   Error = true;
 }
 
 // Split S into linker script tokens.
-std::vector<StringRef> ScriptParser::tokenize(StringRef S) {
-  std::vector<StringRef> Ret;
+std::vector<ScriptParser::Token> ScriptParser::tokenize(StringRef S) {
+  std::vector<Token> Ret;
+  size_t Line = 0;
   for (;;) {
-    S = skipSpace(S);
+    StringRef Clip = skipSpace(S);
+    size_t Space = S.size() - Clip.size();
+    if (Space) {
+      Line += S.substr(0, Space).count('\n');
+      S = Clip;
+    }
+
     if (S.empty())
       return Ret;
 
     // Quoted token
     if (S.startswith("\"")) {
       size_t E = S.find("\"", 1);
       if (E == StringRef::npos) {
         error("unclosed quote");
         return {};
       }
-      Ret.push_back(S.substr(1, E - 1));
+      Ret.push_back({S.substr(1, E - 1), Line});
       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));
+    Ret.push_back({S.substr(0, Pos), Line});
     S = S.substr(Pos);
   }
 }
 
 // Skip leading whitespace characters or /**/-style comments.
 StringRef ScriptParser::skipSpace(StringRef S) {
   for (;;) {
     if (S.startswith("/*")) {

 
 StringRef ScriptParser::next() {
   if (Error)
     return "";
   if (atEOF()) {
     setError("unexpected EOF");
     return "";
   }
-  return Tokens[Pos++];
+  Line = Tokens[Pos].Line;
+  return Tokens[Pos++].Val;
 }
 
 StringRef ScriptParser::peek() {
   StringRef Tok = next();
   if (Error)
     return "";
   --Pos;
   return Tok;
 }
 
 bool ScriptParser::skip(StringRef Tok) {
   if (Error)
     return false;
   if (atEOF()) {
     setError("unexpected EOF");
     return false;
   }
-  if (Tok != Tokens[Pos])
+  if (Tok != Tokens[Pos].Val)
     return false;
   ++Pos;
   return true;
 }
 
 void ScriptParser::expect(StringRef Expect) {
   if (Error)
     return;
void ScriptParser::readInclude() {
   StringRef Tok = next();
   auto MBOrErr = MemoryBuffer::getFile(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);
+  std::vector<Token> V = tokenize(S);
   Tokens.insert(Tokens.begin() + Pos, V.begin(), V.end());
 }
 
 void ScriptParser::readOutput() {
   // -o <file> takes predecence over OUTPUT(<file>).
   expect("(");
   StringRef Tok = next();
   if (Config->OutputFile.empty())

test/ELF/linkerscript-diagnostic.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
+
+## Take some valid script with multiline comments
+## and check it actually works:
+# RUN: rm -f %t.script
+# 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: ld.lld -shared %t -o %t1 --script %t.script
+
+## Change ":" to "+" at line 1, check that error
+## message starts from correct line number:
+# RUN: rm -f %t.script
+# 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 1:
+
+## Change ":" to "+" at line 2 now, check correct error line number:
+# RUN: rm -f %t.script
+# 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 2:
+
+## Change ":" to "+" at line 5, after multiline comment,
+## check correct error line number:
+# RUN: rm -f %t.script
+# 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 5: