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

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/LinkerScript.cpp:182
@@ +181,3 @@
+    End = S.size();
+  // rtrim() needed here because string can have '\r\n' at end.
+  return S.substr(Begin, End - Begin).rtrim();
----------------
ruiu wrote:
> Do we have to care about DOS-style newline here?
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 ?

================
Comment at: ELF/LinkerScript.cpp:189-191
@@ +188,5 @@
+  StringRef Line = getLine(Input, Tok.data() - Input.data());
+  size_t Col = Tok.data() - Line.data() + 1;
+  error(Line);
+  error(std::string(Col - 1, ' ') + "^");
+}
----------------
ruiu wrote:
> Remove +1 and -1.
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.

================
Comment at: test/ELF/linkerscript-diagnostic.s:57
@@ +56,3 @@
+## Check that last line is "^"
+# ERR6DUMP: 00000030  54 41 47 20 7b 0d 0a 5e
+
----------------
This test I was able to replace with regexp. But test below - not.

================
Comment at: test/ELF/linkerscript-diagnostic.s:70
@@ +69,2 @@
+## Check that last line is "     ^"
+# ERR7DUMP: 00000040 20 20 20 20 20 5e
----------------
ruiu wrote:
> This is probably too much. Please just use a regexp {{.....^}} instead.
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: {{.....\^}}
```


http://reviews.llvm.org/D18699