This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Linkerscript: better diagnostic for INPUT/GROUP commands.
AbandonedPublic

Authored by grimar on Jul 27 2017, 8:39 AM.

Details

Reviewers
ruiu
rafael
Summary

Imagine we have script with following line:

INPUT(no_such_file)

Previously we would report next error: "cannot open no_such_file: No such file
or directory". It provides no information that file was tried to be opened from
linker script. And it is confusing, because some times we have regular inputs that
looks like libraries, for example libm.so, but they are linker scripts inside.
It is hard to find where error is coming from without debugging.

Patch adds reporting of script file name.

Diff Detail

Event Timeline

grimar created this revision.Jul 27 2017, 8:39 AM
ruiu edited edge metadata.Jul 27 2017, 3:29 PM

I think it makes more sense to print out the error message if Error becomes true.

In D35945#823283, @ruiu wrote:

I think it makes more sense to print out the error message if Error becomes true.

That will be less intrusive but probably less straightforward as well. Will update the diff.

grimar updated this revision to Diff 108606.Jul 28 2017, 2:17 AM
  • Addressed review comment.
ruiu added inline comments.Aug 2 2017, 8:33 PM
ELF/ScriptParser.cpp
285–289

I think you can only check ErrorCount. Remove Error.

grimar updated this revision to Diff 109511.Aug 3 2017, 4:10 AM
grimar retitled this revision from [ELF] - Linkerscript: better disgnostic for INPUT/GROUP commands. to [ELF] - Linkerscript: better diagnostic for INPUT/GROUP commands..
  • Addressed review comment.
ruiu added inline comments.Aug 3 2017, 4:37 AM
ELF/ScriptParser.cpp
311

Ditto

grimar added inline comments.Aug 3 2017, 4:41 AM
ELF/ScriptParser.cpp
311

Why ? I think readAsNeeded() can set Error.

ruiu added inline comments.Aug 3 2017, 4:42 AM
ELF/ScriptParser.cpp
311

Whenever error is true, ErrorCount should be greater than 0.

grimar added inline comments.Aug 3 2017, 4:53 AM
ELF/ScriptParser.cpp
311

I do not think so. What if we will have:

AS_NEEDED#valid_file_name)

'readAsNeeded()' expects '(', but see # and triggers Error, then adds file correctly and we have Error set, but ErrorCount == 0.
So looks readGroup() is fine.

Honestly I think watching these flags separatelly starting to be complicated. May be better to leave initial version, where both 'Error' and 'ErrorCount' were checked in readAsNeeded() ? There is no need to perform attemp to open file in case I showed above.

grimar added inline comments.Aug 3 2017, 5:01 AM
ELF/ScriptParser.cpp
311

Ah, I missed that setError also calls error() which sets ErrorCount to be > 0. Please ignore this comment.

grimar added inline comments.Aug 3 2017, 5:23 AM
ELF/ScriptParser.cpp
311

I suggest to switch to always use ErrorCount: D36266

grimar updated this revision to Diff 109708.Aug 4 2017, 4:42 AM
  • Reimplemented in a more generic way.
ruiu added inline comments.Aug 4 2017, 10:37 PM
ELF/ScriptParser.cpp
269

I don't think this is enough. You want to check if ErrorCount is incremented in this function.

grimar added inline comments.Aug 7 2017, 7:31 AM
ELF/ScriptParser.cpp
269

I think it is not possible to call this function if we have ErrorCount > 0.
(we call it only from readAsNeeded() and readGroup() and both check the ErrorCount before call)

And also it is not recursive.

I do not know why I would want to check it is incremented. Am I missing something ?

grimar added inline comments.Aug 22 2017, 11:34 AM
ELF/ScriptParser.cpp
251–255

Will it be better if I add following check here ?

if (ErrorCount)
  return;

It still will be excessive check currently I believe (like I pointed in my previous comment),
but if code outside will change so that this function be called with ErrorCount > 0,
such check will protect error reporting logic this patch introduces.

ruiu added inline comments.Aug 22 2017, 11:44 AM
ELF/ScriptParser.cpp
251–255

If that shouldn't happen, add it as an assert.

grimar updated this revision to Diff 112313.Aug 23 2017, 2:02 AM
  • Added assert.
ruiu added inline comments.Aug 23 2017, 8:21 AM
ELF/ScriptParser.cpp
270

Do not call error twice for one error message.

Okay, so I think we need some better infrastructure for diagnostics if we really want to improve linker script diagnostics. That's not that high priority, but it is worth to try. I'll try to create one. Until then, please hold on with and other changes for diagnostics.

grimar abandoned this revision.Dec 1 2017, 4:14 AM