Page MenuHomePhabricator

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

Authored by grimar on Apr 1 2016, 9:58 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar updated this revision to Diff 52388.Apr 1 2016, 9:58 AM
grimar retitled this revision from to [ELF] - Teach linkerscript error handler to show full script line + column marker on error..
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar.
ruiu added inline comments.Apr 1 2016, 10:44 AM
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 updated this revision to Diff 52407.Apr 1 2016, 12:15 PM
  • Addressed review comment.
ELF/LinkerScript.cpp
176 ↗(On Diff #52388)

Done.

ruiu edited edge metadata.Apr 1 2016, 2:22 PM

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 updated this revision to Diff 52469.Apr 2 2016, 11:22 AM
grimar edited edge metadata.

Addressed review comments.

ruiu added inline comments.Apr 2 2016, 11:25 AM
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 added inline comments.Apr 2 2016, 11:43 AM
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 added inline comments.Apr 2 2016, 11:48 AM
test/ELF/linkerscript-diagnostic.s
66 ↗(On Diff #52469)

I haven't tried, but does this work?

{{.......^}}
grimar added inline comments.Apr 2 2016, 11:51 AM
test/ELF/linkerscript-diagnostic.s
66 ↗(On Diff #52469)

Always pass, no matter how many dots I put.

ruiu added inline comments.Apr 2 2016, 11:54 AM
test/ELF/linkerscript-diagnostic.s
66 ↗(On Diff #52469)

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

grimar added inline comments.Apr 2 2016, 1:20 PM
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 updated this revision to Diff 52474.Apr 2 2016, 1:40 PM
grimar marked 2 inline comments as done.
  • 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 updated this revision to Diff 52475.Apr 2 2016, 1:45 PM

Quickfix, forgot about one line.

grimar updated this revision to Diff 52492.Apr 3 2016, 1:26 AM
  • Moved rtrim() to getLine(), added comment.
ruiu added a comment.Apr 3 2016, 11:44 AM

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 updated this revision to Diff 52542.Apr 4 2016, 4:06 AM
grimar marked 2 inline comments as done.
  • 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 added inline comments.Apr 4 2016, 1:10 PM
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 updated this revision to Diff 52656.Apr 4 2016, 10:59 PM
grimar marked 2 inline comments as done.
  • 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 accepted this revision.Apr 5 2016, 7:53 AM
ruiu edited edge metadata.

LGTM with a nit.

test/ELF/linkerscript-diagnostic.s
55 ↗(On Diff #52656)

RUN: grep '^^' %t.log

This revision is now accepted and ready to land.Apr 5 2016, 7:53 AM
This revision was automatically updated to reflect the committed changes.