Limit maximum number of errors to 1000.

[ruiu]

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?

[inglorion]

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]

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]

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]

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]

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 as per comments.

[ruiu]

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]

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.

[davide]

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]

• Add tests
• Add -error-limit flag

[davide]

ELF/Driver.cpp

45–47

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

[ruiu]

ELF/Driver.cpp

45–47

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

[davide]

LGTM. I misread ErrorLimit as ErrorCount.

[filcab]

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

ELF/Error.cpp

46

Fair enough. Thanks!

[ruiu]

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

[ruiu]

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

ELF/Driver.cpp

 using namespace lld;
 using namespace lld::elf;
 
 Configuration *elf::Config;
 LinkerDriver *elf::Driver;
 
 bool elf::link(ArrayRef<const char *> Args, bool CanExitEarly,
                raw_ostream &Error) {
   HasError = false;
+  ErrorCount = 0;
   ErrorOS = &Error;
   Argv0 = Args[0];
 
   Configuration C;
   LinkerDriver D;
   ScriptConfiguration SC;
   Config = &C;
   Driver = &D;
   ScriptConfig = &SC;

ELF/Error.h

 
 #include "lld/Core/LLVM.h"
 
 #include "llvm/Support/Error.h"
 
 namespace lld {
 namespace elf {
 
+// The maximum number of errors. If error() is called more than
+// this number, LLD bails out. 0 means no limit.
+const unsigned ErrorLimit = 1000;
+
 extern bool HasError;
+extern unsigned ErrorCount;
 extern llvm::raw_ostream *ErrorOS;
 extern llvm::StringRef Argv0;
 
 void log(const Twine &Msg);
 void warn(const Twine &Msg);
 
 void error(const Twine &Msg);
 void error(std::error_code EC, const Twine &Prefix);

ELF/Error.cpp

 #include <unistd.h>
 #endif
 
 using namespace llvm;
 
 namespace lld {
 
 bool elf::HasError;
+unsigned elf::ErrorCount;
 raw_ostream *elf::ErrorOS;
 StringRef elf::Argv0;
 
 void elf::log(const Twine &Msg) {
   if (Config->Verbose)
     outs() << Argv0 << ": " << Msg << "\n";
 }
 
 void elf::warn(const Twine &Msg) {
   if (Config->FatalWarnings)
     error(Msg);
   else
     *ErrorOS << Argv0 << ": warning: " << Msg << "\n";
 }
 
 void elf::error(const Twine &Msg) {
+  if (ErrorLimit == 0 || ErrorCount <= ErrorLimit)
     *ErrorOS << Argv0 << ": error: " << Msg << "\n";
+  else if (Config->ExitEarly)
+    exitLld(1);
+
   HasError = true;
+  ErrorCount++;
 }
 
 void elf::error(std::error_code EC, const Twine &Prefix) {
   error(Prefix + ": " + EC.message());
 }
 
 void elf::exitLld(int Val) {
   // Dealloc/destroy ManagedStatic variables before calling