This is an archive of the discontinued LLVM Phabricator instance.

Limit maximum number of errors to 1000.
ClosedPublic

Authored by ruiu on Nov 22 2016, 10:09 AM.

Details

Summary

This is in the context of https://llvm.org/bugs/show_bug.cgi?id=31109.
When LLD prints out errors for relocations, it tends to print out
extremely large number of errors (like millions) because it would
print out one error per relocation.

This patch makes LLD bail out if it found too many errors.
What do you think?

Event Timeline

ruiu updated this revision to Diff 78890.Nov 22 2016, 10:09 AM
ruiu retitled this revision from to Limit maximum number of errors to 1000..
ruiu updated this object.
ruiu added reviewers: rafael, davide, inglorion.
ruiu added a subscriber: llvm-commits.
inglorion edited edge metadata.Nov 22 2016, 10:20 AM

I like the idea of limiting errors, particularly for errors like the message that prompted this, which end up just printing out an identical message over and over again. However, I think ELF is probably not the right place for this to live. Can we make it a top-level lld feature with a command line parameter to control the threshold and perhaps even turn it off?

filcab added a subscriber: filcab.Nov 22 2016, 10:27 AM

Test case?
It seems having a --full-shutdown will make lld not "bail out" after N errors, but stop printing errors anyway. In that case, no use going on with the link.

ELF/Error.cpp
44

Nit: The <= condition and ErrorCount++ below this check make lld emit N+1 errors.

46

I don't think this should really be tied to --full-shutdown. I see those features as orthogonal.

47

You probably want to tell the user why lld exited. Something like "Too many errors emitted, halting link" should do it.
(No need to go overboard with messages like "go fix your program" like valgrind had :-) )

ruiu added a comment.Nov 22 2016, 10:28 AM

This makes sense to the COFF linker indeed. But the ELF linker and COFF linkers are different (for example, the COFF linker doesn't support library use, so it can always exit() on too many error), I'd implement it into COFF as a separate patch rather than trying to integrate.

We'll probably make change to the relocation errors, so that they'll include filenames and offsets. It is just a negligence that we are printing out the exact same error.

I wonder in what situation you want to make the maximum number configurable. Isn't 1000 enough?

ruiu added a comment.Nov 22 2016, 10:39 AM

I'll add a test once we get an agreement on what we should do.

ELF/Error.cpp
44

Will fix.

46

It correlates to CanExitEarly argument given to lld::elf::link. If it's true, we can't exit. Since --full-shutdown is for tests, I think using Config->ExitEarly is okay.

47

Agreed.

davide edited edge metadata.Nov 22 2016, 10:51 AM

I think the idea is OK, but I have some questions about the implementation. Please see inline comment.

ELF/Error.h
34–36

I'm not sure why you want to say ErrorLimit == 0 means no limit and not make this configurable. Either make it configurable or remove the special meaning for 0 here (and below).

I'd pretty much prefer having a limit fixed instead of configurable. I also think that 1000 is too big of a number. IIRC clang sets -ferror-limit to 20 by default, maybe something nearer to that threshold?

ruiu updated this revision to Diff 78898.Nov 22 2016, 10:52 AM
ruiu edited edge metadata.
  • Updated as per comments.
ruiu added a comment.Nov 22 2016, 10:56 AM

If you make this configurable, what's the flag name? gcc uses -fmax-errors=n, while clang uses -ferror-limit=n. (Why they are different in the first place?)

emaste added a subscriber: emaste.Nov 22 2016, 10:59 AM
emaste added inline comments.
ELF/Error.h
34–36

Even if the user's not inspecting each reported error, it is sometimes useful to have all of them available: for example, comparing the absolute number over time while fixing some sort of broad issue, or in cases where hundreds of identical errors, and then a small number of unique ones.

IMO if it's set to 20 (which seems like a reasonable default to me), it absolutely must be configurable.

I think the idea is OK, but I have some questions about the implementation. Please see inline comment.

In D26981#602919, @ruiu wrote:

If you make this configurable, what's the flag name? gcc uses -fmax-errors=n, while clang uses -ferror-limit=n. (Why they are different in the first place?)

I'm not sure why LLVM and GCC use two different flags, but at this point, if gold doesn't have a similar flag, I'd just stick with whatever clang uses (i.e. -ferror-limit=patatino).

ruiu updated this revision to Diff 78900.Nov 22 2016, 11:14 AM
  • Add tests
  • Add -error-limit flag
davide added inline comments.Nov 22 2016, 11:21 AM
ELF/Driver.cpp
45–47

Any reason to initialize it here instead of inside struct Configuration ?

ruiu added inline comments.Nov 22 2016, 11:25 AM
ELF/Driver.cpp
45–47

This is not a limit but the current number of errors. The limit is in Config.

davide accepted this revision.Nov 22 2016, 11:29 AM
davide edited edge metadata.

LGTM. I misread ErrorLimit as ErrorCount.

This revision is now accepted and ready to land.Nov 22 2016, 11:29 AM

Just an additional question: Can errors be signalled in different threads?

ELF/Error.cpp
46

Fair enough. Thanks!

ruiu added a comment.Nov 23 2016, 8:34 AM

That's a very good point. Errors could be emitted from multiple threads at the same time, so we need a lock.

This revision was automatically updated to reflect the committed changes.
ruiu added a comment.Nov 23 2016, 10:45 AM

FYI: Filipe, I added a mutex to protect outs() and errs() in r287794.