Page MenuHomePhabricator

[lld] [ELF] Support for warn-once option
AcceptedPublic

Authored by spetrovic on Jan 15 2019, 7:41 AM.

Details

Reviewers
ruiu
espindola
Summary

This patch adds warn-once option in LLD.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

spetrovic created this revision.Jan 15 2019, 7:41 AM
ruiu added a comment.Jan 15 2019, 10:05 AM

I personally did not find this option very useful. Could you explain in what situation you find this useful?

grimar added a subscriber: grimar.Jan 16 2019, 1:17 AM

I have something like this in many places:

extern void foo(int a);

int main() {

int i;
for (i=0;i<20;i++)
  foo(i);
return 0;

}

Without warn-once option on this example lld prints 20 error messages (-O2 optimization while making object file), with warn-once option it prints just one error message, so if we have similar code in many places this option might be useful.

Without warn-once option on this example lld prints 20 error messages (-O2 optimization while making object file), with warn-once option it prints just one error message, so if we have similar code in many places this option might be useful.

May we you just can use -error-limit=0 (no limit)?

Well yes, but it would be nicer not having to modify the scripts which we currently use in our build systems.

grimar added a comment.EditedJan 22 2019, 12:52 AM

In theory, spamming user with the same errors is not very friendly/useful indeed. Especially when we have 20 errors limit by default. Rui, what do you think if we would always enable -warn-once functionality?
(i.e. we would continue ignoring the option, because does not seem we would want to have -no-warn-once, but change the behavior to report an error only once for the symbol).

Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2019, 7:53 AM
ruiu added a comment.Feb 6 2019, 3:30 PM

I think I'm fine with this feature. I wouldn't personally use this that much, but I can imagine that some people would find this useful.

ELF/Config.h
183

Remove = false

ELF/Driver.cpp
867

This is the right place to set to Config->WarnOnce

1110–1112

instead of this place.

ELF/Options.td
376

per -> for each

ELF/Symbols.h
214

This comment should end with a full stop. Please also mention that this bit is used for --warn-once.

IsWarned -> Warned

test/ELF/warn-once.s
19

Please make sure that this file ends with a newline character.

grimar added inline comments.Feb 6 2019, 11:33 PM
test/ELF/warn-once.s
18

nit: remove ;

grimar added inline comments.Feb 10 2019, 11:46 PM
test/ELF/warn-once.s
11

BTW, this probably can be just the following for simplicity:

# CHECK:  error: undefined symbol: foo
# CHECK-NOT: error: undefined symbol: foo

(no need to check reporting of the location, we have a lot of other tests for that)

spetrovic marked 6 inline comments as done.Feb 11 2019, 7:53 AM
spetrovic updated this revision to Diff 186263.Feb 11 2019, 7:56 AM

Rui, should we make -warn-once ON by default?

ELF/Driver.cpp
870

I think this is not clang-formatted.
(Should be single line).

ruiu added a comment.Feb 11 2019, 9:55 AM

Rui, should we make -warn-once ON by default?

No, I don't think so. Either on or off could work as a default, but ON is currently the default, and I don't think we have a reason to change the default.

ELF/Options.td
375

You should follow the local convention of a file. Please add a blank line before each def line.

spetrovic updated this revision to Diff 186472.Feb 12 2019, 7:53 AM
spetrovic marked 2 inline comments as done.
ruiu accepted this revision.Feb 12 2019, 8:45 AM

LGTM

This revision is now accepted and ready to land.Feb 12 2019, 8:45 AM