[ELF] - Teach linkerscript error handler to show full script line + column marker on error.

[grimar]

Summary

When error, this adds the text line of script to the output
and a marks exact incorrect token under it:

line 1: <error text here>
UNKNOWN_TAG {
      ^

[ruiu]

ELF/LinkerScript.cpp

176 ↗

(On Diff #52388)

The string manipulation in this function seem a bit too complicated. StringRef provides a good set of utility functions to make this kind of stuff easy. Can you rewrite it with them?

[grimar]

• Addressed review comment.

ELF/LinkerScript.cpp

176 ↗

(On Diff #52388)

Done.

[ruiu]

Let's define

StringRef getLine(StringRef S, size_t Pos)

which returns a line where the character S[Pos] belongs to. You can define that function using StringRef::rfind and StringRef::find (so you don't need a hand-written for loop.) That helper function should make things simpler.

[grimar]

Addressed review comments.

[ruiu]

ELF/LinkerScript.cpp

187–190 ↗

(On Diff #52469)

You can directly print out the error messages in this function.

189 ↗

(On Diff #52469)

"^" should point to the beginning of an erroneous word instead of middle.

[grimar]

test/ELF/linkerscript-diagnostic.s

66 ↗

(On Diff #52469)

I found that this test does not work. I had issues with counting spaces before also, then changed

# RUN: not ld.lld -shared %t -o %t1 --script %t.script  2>&1 | FileCheck ...

to current

# RUN: not ld.lld -shared %t -o %t1 --script %t.script > %t.log 2>&1
# FileCheck -check-prefix=ERR7 %s < %t.log

So added a file. And problems were gone. I thought it is ok now, but at fact I found now that it accepts any number in expression:

{{[ ]{7}\^}}
{{[ ]{70}\^}}
{{[ ]{700}\^}}

All above accepts with file.
I tried other variants that should be valid in real live:

{{[ ]{5}\^}}}
{{[\s]{5}\^}}}
{{[\x20]{5}\^}}}

Nothing works. So now I am searching for a way to check amount of spaces before '^' for testcase.

[ruiu]

test/ELF/linkerscript-diagnostic.s

66 ↗

(On Diff #52469)

I haven't tried, but does this work?

{{.......^}}

[grimar]

test/ELF/linkerscript-diagnostic.s

66 ↗

(On Diff #52469)

Always pass, no matter how many dots I put.

[ruiu]

test/ELF/linkerscript-diagnostic.s

66 ↗

(On Diff #52469)

It is because you forgot to write "RUN:" before FileCheck.

[grimar]

test/ELF/linkerscript-diagnostic.s

66 ↗

(On Diff #52469)

:( Thanks you, that was true.
And I still stuck on the counting whitespaces. When I replace space with "-", then

{{[-]{5}\^}}}

Works just perfect, but the same for spaces:

{{[ ]{5}\^}}}

not works. This seems to me ubnormal behavior of regexp in testsuite.
Continuing investigation how to fix..

[grimar]

• Addressed review comments.
• Fixed test (implemented workaround check for counting whitespaces as found no way to do the same with regexp in testsuite, it seems broken here).

[grimar]

Quickfix, forgot about one line.

[grimar]

• Moved rtrim() to getLine(), added comment.

[ruiu]

Looks much better.

ELF/LinkerScript.cpp

176 ↗

(On Diff #52492)

Add a comment.

// Returns the line that the character S[Pos] is in.

182 ↗

(On Diff #52492)

Do we have to care about DOS-style newline here?

186 ↗

(On Diff #52492)

Rename printErrorPos.

189–191 ↗

(On Diff #52492)

Remove +1 and -1.

test/ELF/linkerscript-diagnostic.s

70 ↗

(On Diff #52492)

This is probably too much. Please just use a regexp {{.....^}} instead.

[grimar]

• Addressed review comments.

ELF/LinkerScript.cpp

182 ↗

(On Diff #52492)

Why not ? I am observing these symbols when debugging files producced by testcase, so '\n' is stripped,
but '\r' is still in line and that does not look good at least in debugger, why not to remove that ?

189–191 ↗

(On Diff #52492)

Done. I was assumed that column enumeration starts with 1 and the code +1/-1 was done intentionally to keep the correct column calculation. That probably really not needed here.

test/ELF/linkerscript-diagnostic.s

57 ↗

(On Diff #52492)

This test I was able to replace with regexp. But test below - not.

70 ↗

(On Diff #52492)

I would be happy to do that. It just does not work.
I think correct variant could be {{.....\^}}, because when I change

error(std::string(Col - 1, ' ') + "^");

to

error(std::string(Col - 1, '-') + "^");

It works fine. And {{.....^}} not works here either. I think it is correct because ^ is special symbol "start of string" in regext and should be preceeded with "\" if exact "^" symbol needs to be matched. I used that fact for ERR6 check above.

But anyways both {{.....\^}} and {{.....^}} are not working for whitespaces for me. Test below fails (for both variants):

## 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 > %t.log 2>&1
# RUN: FileCheck -check-prefix=ERR7 %s < %t.log
# ERR7:      line 4: : expected, but got .temp
# ERR7-NEXT: boom .temp : { *(.temp) } }
# ERR7-NEXT: {{.....\^}}

[ruiu]

ELF/LinkerScript.cpp

176 ↗

(On Diff #52542)

Add "." at the end of the sentence.

182 ↗

(On Diff #52542)

Well, maybe the tone of this comment made me think of that. It is not "neeeded" on Unix, but it's good just in case. I'd update the comment.

// rtrim for DOS-style newlines.
return S.substr(Begin, End - Begin).rtrim();

test/ELF/linkerscript-diagnostic.s

68 ↗

(On Diff #52542)

I'd use grep instead of hexdump.

RUN: grep '^     ^' %t.log

[grimar]

• Addressed Rui's comments.

ELF/LinkerScript.cpp

176 ↗

(On Diff #52542)

Done.

182 ↗

(On Diff #52542)

I will be more careful with wording next time. Fixed.

test/ELF/linkerscript-diagnostic.s

68 ↗

(On Diff #52542)

Works like a charm ! Thanks for hint.

[ruiu]

LGTM with a nit.

test/ELF/linkerscript-diagnostic.s

55 ↗

(On Diff #52656)

RUN: grep '^^' %t.log

lld/trunk/ELF/LinkerScript.cpp

private:
   void readOutputFormat();
   void readSearchDir();
   void readSections();
 
   void readOutputSectionDescription();
   void readSectionPatterns(StringRef OutSec, bool Keep);
 
   size_t getPos();
+  void printErrorPos();
   std::vector<uint8_t> parseHex(StringRef S);
 
   StringSaver Saver;
   StringRef Input;
   std::vector<StringRef> Tokens;
   const static StringMap<Handler> Cmd;
   size_t Pos = 0;
   bool IsUnderSysroot;
while (!atEOF()) {
     StringRef Tok = next();
     if (Handler Fn = Cmd.lookup(Tok))
       (this->*Fn)();
     else
       setError("unknown directive: " + Tok);
   }
 }
 
+// Returns the line that the character S[Pos] is in.
+static StringRef getLine(StringRef S, size_t Pos) {
+  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 ScriptParser::printErrorPos() {
+  StringRef Tok = Tokens[Pos == 0 ? 0 : Pos - 1];
+  StringRef Line = getLine(Input, Tok.data() - Input.data());
+  size_t Col = Tok.data() - Line.data();
+  error(Line);
+  error(std::string(Col, ' ') + "^");
+}
+
 // We don't want to record cascading errors. Keep only the first one.
 void ScriptParser::setError(const Twine &Msg) {
   if (Error)
     return;
   error("line " + Twine(getPos()) + ": " + Msg);
+  printErrorPos();
   Error = true;
 }
 
 // Split S into linker script tokens.
 std::vector<StringRef> ScriptParser::tokenize(StringRef S) {
   std::vector<StringRef> Ret;
   for (;;) {
     S = skipSpace(S);

lld/trunk/test/ELF/linkerscript-diagnostic.s

 # 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:
+
+## 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 > %t.log 2>&1
+# RUN: FileCheck -check-prefix=ERR6 %s < %t.log
+# ERR6:      line 1:
+# ERR6-NEXT: UNKNOWN_TAG {
+# RUN: grep '^^' %t.log
+
+## 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 > %t.log 2>&1
+# RUN: FileCheck -check-prefix=ERR7 %s < %t.log
+# ERR7:      line 4: : expected, but got .temp
+# ERR7-NEXT: boom .temp : { *(.temp) } }
+# RUN: grep '^     ^' %t.log