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 ↗

(On Diff #78890)

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

46 ↗

(On Diff #78890)

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

47 ↗

(On Diff #78890)

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 ↗

(On Diff #78890)

Will fix.

46 ↗

(On Diff #78890)

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 ↗

(On Diff #78890)

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 ↗

(On Diff #78890)

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 ↗

(On Diff #78890)

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–46 ↗

(On Diff #78900)

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

[ruiu]

ELF/Driver.cpp

45–46 ↗

(On Diff #78900)

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 ↗

(On Diff #78890)

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.

lld/trunk/ELF/Config.h

struct Configuration {
   StripPolicy Strip = StripPolicy::None;
   UnresolvedPolicy UnresolvedSymbols;
   Target2Policy Target2 = Target2Policy::GotRel;
   BuildIdKind BuildId = BuildIdKind::None;
   ELFKind EKind = ELFNoneKind;
   uint16_t DefaultSymbolVersion = llvm::ELF::VER_NDX_GLOBAL;
   uint16_t EMachine = llvm::ELF::EM_NONE;
   uint64_t EntryAddr = 0;
+  uint64_t ErrorLimit;
   uint64_t ImageBase;
   uint64_t MaxPageSize;
   uint64_t ZStackSize;
   unsigned LtoPartitions;
   unsigned LtoO;
   unsigned Optimize;
   unsigned ThinLtoJobs;
 };
 
 // The only instance of Configuration struct.
 extern Configuration *Config;
 
 } // namespace elf
 } // namespace lld
 
 #endif

lld/trunk/ELF/Driver.cpp

 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;
void LinkerDriver::readConfigs(opt::InputArgList &Args) {
   Config->AllowMultipleDefinition = Args.hasArg(OPT_allow_multiple_definition);
   Config->Bsymbolic = Args.hasArg(OPT_Bsymbolic);
   Config->BsymbolicFunctions = Args.hasArg(OPT_Bsymbolic_functions);
   Config->Demangle = getArg(Args, OPT_demangle, OPT_no_demangle, true);
   Config->DisableVerify = Args.hasArg(OPT_disable_verify);
   Config->Discard = getDiscardOption(Args);
   Config->EhFrameHdr = Args.hasArg(OPT_eh_frame_hdr);
   Config->EnableNewDtags = !Args.hasArg(OPT_disable_new_dtags);
+  Config->ErrorLimit = getInteger(Args, OPT_error_limit, 20);
   Config->ExportDynamic = Args.hasArg(OPT_export_dynamic);
   Config->FatalWarnings = Args.hasArg(OPT_fatal_warnings);
   Config->GcSections = getArg(Args, OPT_gc_sections, OPT_no_gc_sections, false);
   Config->GdbIndex = Args.hasArg(OPT_gdb_index);
   Config->ICF = Args.hasArg(OPT_icf);
   Config->NoGnuUnique = Args.hasArg(OPT_no_gnu_unique);
   Config->NoUndefinedVersion = Args.hasArg(OPT_no_undefined_version);
   Config->Nostdlib = Args.hasArg(OPT_nostdlib);

lld/trunk/ELF/Error.h

 #include "lld/Core/LLVM.h"
 
 #include "llvm/Support/Error.h"
 
 namespace lld {
 namespace elf {
 
 extern bool HasError;
+extern uint64_t 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);

lld/trunk/ELF/Error.cpp

 #include <unistd.h>
 #endif
 
 using namespace llvm;
 
 namespace lld {
 
 bool elf::HasError;
+uint64_t 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 (Config->ErrorLimit == 0 || ErrorCount < Config->ErrorLimit) {
     *ErrorOS << Argv0 << ": error: " << Msg << "\n";
+  } else if (ErrorCount == Config->ErrorLimit) {
+    *ErrorOS << Argv0 << ": error: too many errors emitted, stopping now\n";
+    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

lld/trunk/ELF/Options.td

def enable_new_dtags: F<"enable-new-dtags">,
   HelpText<"Enable new dynamic tags">;
 
 def end_lib: F<"end-lib">,
   HelpText<"End a grouping of objects that should be treated as if they were together in an archive">;
 
 def entry: S<"entry">, MetaVarName<"<entry>">,
   HelpText<"Name of entry point symbol">;
 
+def error_limit: S<"error-limit">,
+  HelpText<"Maximum number of errors to emit before stopping (0 = no limit)">;
+
 def export_dynamic: F<"export-dynamic">,
   HelpText<"Put symbols in the dynamic symbol table">;
 
 def export_dynamic_symbol: S<"export-dynamic-symbol">,
   HelpText<"Put a symbol in the dynamic symbol table">;
 
 def fatal_warnings: F<"fatal-warnings">,
   HelpText<"Treat warnings as errors">;

 def alias_Bstatic_non_shared: F<"non_shared">, Alias<Bstatic>;
 def alias_Bstatic_static: F<"static">, Alias<Bstatic>;
 def alias_L__library_path: J<"library-path=">, Alias<L>;
 def alias_discard_all_x: Flag<["-"], "x">, Alias<discard_all>;
 def alias_discard_locals_X: Flag<["-"], "X">, Alias<discard_locals>;
 def alias_dynamic_list: J<"dynamic-list=">, Alias<dynamic_list>;
 def alias_entry_e: JoinedOrSeparate<["-"], "e">, Alias<entry>;
 def alias_entry_entry: J<"entry=">, Alias<entry>;
+def alias_error_limit: J<"error-limit=">, Alias<error_limit>;
 def alias_export_dynamic_E: Flag<["-"], "E">, Alias<export_dynamic>;
 def alias_export_dynamic_symbol: J<"export-dynamic-symbol=">,
   Alias<export_dynamic_symbol>;
 def alias_fini_fini: J<"fini=">, Alias<fini>;
 def alias_format_b: S<"b">, Alias<format>;
 def alias_hash_style_hash_style: J<"hash-style=">, Alias<hash_style>;
 def alias_init_init: J<"init=">, Alias<init>;
 def alias_l__library: J<"library=">, Alias<l>;

lld/trunk/test/ELF/error-limit.test

+RUN: not ld.lld 01 02 03 04 05 06 07 08 09 10 11 12 13 14 15 16 17 18 19 20 \
+RUN:   21 22 2>&1 | FileCheck -check-prefix=DEFAULT %s
+
+DEFAULT:      cannot open 01
+DEFAULT:      cannot open 20
+DEFAULT-NEXT: too many errors emitted, stopping now
+DEFAULT-NOT:  cannot open 21
+
+RUN: not ld.lld -error-limit=5 01 02 03 04 05 06 07 08 09 10 2>&1 \
+RUN:   | FileCheck -check-prefix=LIMIT5 %s
+RUN: not ld.lld -error-limit 5 01 02 03 04 05 06 07 08 09 10 2>&1 \
+RUN:   | FileCheck -check-prefix=LIMIT5 %s
+
+LIMIT5:      cannot open 01
+LIMIT5:      cannot open 05
+LIMIT5-NEXT: too many errors emitted, stopping now
+LIMIT5-NOT:  cannot open 06
+
+RUN: not ld.lld -error-limit=0 01 02 03 04 05 06 07 08 09 10 11 12 13 14 15 \
+RUN:   16 17 18 19 20 21 22 2>&1 | FileCheck -check-prefix=UNLIMITED %s
+
+UNLIMITED:     cannot open 01
+UNLIMITED:     cannot open 20
+UNLIMITED:     cannot open 21
+UNLIMITED:     cannot open 22
+UNLIMITED-NOT: too many errors emitted, stopping now