lld: Add a warning limit, similar to the existing error limit

[thakis]

Summary

At least the COFF and ELF ports can produce arbitrary amounts of warnings (via /force, and via --orphan-handling=warn respectively).

To not produce arbitrary amounts of output by default, stop at 20 warnings by default.

(I'll add tests to this review if there are no concerns about adding this flag.)

[ruiu]

I'm not confident that this is what the user wants. This might be useful, but at the same time I can imagine that people want to see an entire log rather than the first part of it.

Does clang or other tools have a similar feature?

lld/COFF/Driver.cpp

68–69

I don't think "stopping" is a correct word to explain the feature. It stops showing more warnings, but the process continues.

[thakis]

In D61710#1495938, @ruiu wrote:

I'm not confident that this is what the user wants. This might be useful, but at the same time I can imagine that people want to see an entire log rather than the first part of it.

Does clang or other tools have a similar feature?

I'm not sure either this is the right approach. So far the only real use case I know of is in lld-link with /force. There are two use cases for it:

• The flag is useful when trying to reduce a bug from a large program to a small program. You can just keep deleting .cc input files and add /force and see if the bug stays around. For example, when reducing the recent /natvis: debug info bug from "build chromium" to PR41626 this was useful.

• Now that lld-link reports duplicate resources as errors and has /force:duplicateres to turn those into a warning so that people can incrementally fix their codebase as they update to the new lld, they might want to disable the warning for that for some target that takes a while to fix.

In both cases, I actually want to see 0 warnings – and this patch doesn't support -warninglimit:0 (or it does, but it means "all warnings", not "no warnings").

With link.exe, I can do /force /ignore:4006 and not get warnigns about duplicate symbols. Maybe we should instead add support for /ignore:4006 . link.exe with /force turns duplicate symbol errors into a warning and can /ignore them, but for undefined symbols it still prints "error:" and then exits with exit code 0 – and there is as far as I can tell no way to disable that diagnostic. So I don't know if we'd want to make /ignore:4006 disable all the /force: warnings if we go that route or what else to do for undefined symbols, and what to do for 'duplicate res' diagnostics (which link.exe can't /force at all, they're always hard errors there).

clang does have very granular warning flags, so this is a bit less of an issue there.

clang does have -w to disable _all_ warnings. I suppose that would be good enough for the /force use case as well – if you use force you're already a bit desperate and so disabling all warnings would be fine. Maybe we should add a flag to disable all warning output instead?

[MaskRay]

• clang has -ferror-limit= that defaults to 19.
• Since we've got error-limit=, it seems reasonable to have a parallel warning-limit=.
• I'm not sure the counterpart of clang -w in the linker world will be useful. Most linker warnings are real problems. In contract, many compiler warnings are just style issues and one may argue some are really arbitrary. Some ELF builds have --fatal-warnings, the probably just turn off that option if they don't want to see errors.

[ruiu]

• I'm not sure the counterpart of clang -w in the linker world will be useful. Most linker warnings are real problems. In contract, many compiler warnings are just style issues and one may argue some are really arbitrary. Some ELF builds have --fatal-warnings, the probably just turn off that option if they don't want to see errors.

That's a good point. When the linker prints out a warning message, that usually means there's a real problem in your program, even if your program would work fine. I don't think implementing /ignore:4006 to lld is a bad option. It may actually be a good option, as I can't think of any other warning messages that we want to suppress at the moment.

[arichardson]

I implemented the same thing for the CHERI fork of lld since I added some new warning that could in certain cases be emitted many times but are not necessarily a real problem.

Suppressing a single warning would also solve this problem but seems like a much more invasive change. I would be happy if this patch got committed since it would make our code closer to upstream and simplify future merges.

[ruiu]

In D61710#1496097, @arichardson wrote:

I implemented the same thing for the CHERI fork of lld since I added some new warning that could in certain cases be emitted many times but are not necessarily a real problem.

What would you do if something is not a real problem? I'd think "fix all warning" is generally a good idea, so I'd try to fix the problem (if it is not a real problem) or suppress the warning, and when suppressing a warning, I'd do one by one. So I wouldn't personally suppress warnings as a whole, as it could hide other warnings.

I just came up with another (maybe crazy) idea. When we get an overwhelming number of warnings, they are usually of the same kind of warning. So, it might make sense to set a limit per a warning. And there's an easy (and probably a bit hacky) way to do that -- we can identify who calls warn() by an address returned by __builtin_return_address(). If some line invokes warn() more than a certain threshold (e.g. 20), we can immediately return from warn().

[arichardson]

In D61710#1496112, @ruiu wrote:

In D61710#1496097, @arichardson wrote:

I implemented the same thing for the CHERI fork of lld since I added some new warning that could in certain cases be emitted many times but are not necessarily a real problem.

What would you do if something is not a real problem? I'd think "fix all warning" is generally a good idea, so I'd try to fix the problem (if it is not a real problem) or suppress the warning, and when suppressing a warning, I'd do one by one. So I wouldn't personally suppress warnings as a whole, as it could hide other warnings.

I agree and we do try to fix all of these. However, it may take a while to update all the cases where this is triggered in CheriBSD and I don't want to flood the build log with hundreds of almost identical warnings since it makes it harder to find the real issue. In the past this was a bigger problem since everything was very experimental and constantly changing. Now things are a lot more stable and we rarely add new warnings that cause this warning limit to be reached.

I just came up with another (maybe crazy) idea. When we get an overwhelming number of warnings, they are usually of the same kind of warning. So, it might make sense to set a limit per a warning. And there's an easy (and probably a bit hacky) way to do that -- we can identify who calls warn() by an address returned by __builtin_return_address(). If some line invokes warn() more than a certain threshold (e.g. 20), we can immediately return from warn().

I think this should work (as long as we don't end up with helper functions that call warn and therefore hide the real caller).
Having a limit per-warning rather than a global limit would allow retaining useful warnings while limititing the identical ones than can (at least temporarily) be ignored.
Another solution would be to change warnings that can be emitted many time times to use a warnLimited or similar function but that would require analyzing all warn calls.
I like the idea with the return address.

[MaskRay]

Note, if we go with __builtin_return_address(2), we may have to add __attribute__((noinline)) to warn(). An alternative is to add a macro for warnings, and let it call the underlying warn() with __func__ + __LINE__ (__FILE__ may leak the path name)

[ruiu]

Yes, a macro works too.

But what is your concern with "leaking" the pathname?

[MaskRay]

In D61710#1496187, @ruiu wrote:

Yes, a macro works too.

But what is your concern with "leaking" the pathname?

Not a big concern, just that __FILE__ affects reproducibility. __FILE__ means "a.c" in clang a.c but "/build/a.c" in clang /build/a.c.

[ruiu]

That is true, but I think that's not usually considered a problem of build reproducibility. Reproducible build guarantees the same result if you pass the exact same files and the exact same command line options, so if you change a way to pass a pathname to the compiler, it is OK to not create the same output.

[grimar]

lld/Common/ErrorHandler.cpp

121

Should we have sokething like the following?

 if (WarningLimit > WarningLimit)
  continue;

print("warning: ", raw_ostream::MAGENTA);
if (WarningCount < WarningLimit)
  *ErrorOS << Msg << "\n";
else
  *ErrorOS << WarningLimitExceededMsg << "\n";

[thakis]

I'll abandon this for now, since as said above it doesn't address the use case as I have, and I agree with Rui that it feels like the wrong approach. If I find time, I'll try to make a CL to make /force warnings ignorable.

I do think having a "disable all warnings" flag would be useful though, for the "warnings are either important and should either be errors (/WX), or are unimportant and should be silent" build philosophy.

lld/COFF/Driver.cpp

 
 bool link(ArrayRef<const char *> Args, bool CanExitEarly, raw_ostream &Diag) {
   errorHandler().LogName = args::getFilenameWithoutExe(Args[0]);
   errorHandler().ErrorOS = &Diag;
   errorHandler().ColorDiagnostics = Diag.has_colors();
   errorHandler().ErrorLimitExceededMsg =
       "too many errors emitted, stopping now"
       " (use /errorlimit:0 to see all errors)";
+  errorHandler().WarningLimitExceededMsg =
+      "too many warnings emitted, stopping now"
+      " (use /warninglimit:0 to see all warnings)";
   errorHandler().ExitEarly = CanExitEarly;
   Config = make<Configuration>();
 
   Symtab = make<SymbolTable>();
 
   Driver = make<LinkerDriver>();
   Driver->link(Args);
 
void LinkerDriver::link(ArrayRef<const char *> ArgsArr) {
   // Handle /errorlimit early, because error() depends on it.
   if (auto *Arg = Args.getLastArg(OPT_errorlimit)) {
     int N = 20;
     StringRef S = Arg->getValue();
     if (S.getAsInteger(10, N))
       error(Arg->getSpelling() + " number expected, but got " + S);
     errorHandler().ErrorLimit = N;
   }
+  if (auto *Arg = Args.getLastArg(OPT_warninglimit)) {
+    int N = 20;
+    StringRef S = Arg->getValue();
+    if (S.getAsInteger(10, N))
+      error(Arg->getSpelling() + " number expected, but got " + S);
+    errorHandler().WarningLimit = N;
+  }
 
   // Handle /help
   if (Args.hasArg(OPT_help)) {
     printHelp(ArgsArr[0]);
     return;
   }
 
   lld::ThreadsEnabled = Args.hasFlag(OPT_threads, OPT_threads_no, true);

lld/COFF/Options.td

 def base    : P<"base", "Base address of the program">;
 def color_diagnostics: Flag<["--"], "color-diagnostics">,
   HelpText<"Use colors in diagnostics">;
 def color_diagnostics_eq: Joined<["--"], "color-diagnostics=">,
   HelpText<"Use colors in diagnostics; one of 'always', 'never', 'auto'">;
 def defaultlib : P<"defaultlib", "Add the library to the list of input files">;
 def delayload : P<"delayload", "Delay loaded DLL name">;
 def entry   : P<"entry", "Name of entry point symbol">;
-def errorlimit : P<"errorlimit",
-    "Maximum number of errors to emit before stopping (0 = no limit)">;
 def export  : P<"export", "Export a function">;
 // No help text because /failifmismatch is not intended to be used by the user.
 def failifmismatch : P<"failifmismatch", "">;
 def functionpadmin : F<"functionpadmin">;
 def functionpadmin_opt : P<"functionpadmin", "Prepares an image for hotpatching">;
 def guard   : P<"guard", "Control flow guard">;
 def heap    : P<"heap", "Size of the heap">;
 def ignore : P<"ignore", "Specify warning codes to ignore">;
defm tsaware  : B<"tsaware",
                   "Create non-Terminal Server aware executable">;
 
 def help : F<"help">;
 
 // /?? and -?? must be before /? and -? to not confuse lib/Options.
 def help_q : Flag<["/??", "-??", "/?", "-?"], "">, Alias<help>;
 
 // LLD extensions
+def errorlimit : P<"errorlimit",
+    "Maximum number of errors to emit before stopping (0 = no limit)">;
+def warninglimit : P<"warninglimit",
+    "Maximum number of warnings to emit before stopping (0 = no limit)">;
 def exclude_all_symbols : F<"exclude-all-symbols">;
 def export_all_symbols : F<"export-all-symbols">;
 defm demangle : B<"demangle",
     "Demangle symbols in output (default)",
     "Do not demangle symbols in output">;
 def kill_at : F<"kill-at">;
 def lldmingw : F<"lldmingw">;
 def output_def : Joined<["/", "-", "/?", "-?"], "output-def:">;
 def pdb_source_path : P<"pdbsourcepath",
-                        "Base path used to make relative source file path absolute in PDB">;
 def rsp_quoting : Joined<["--"], "rsp-quoting=">,
   HelpText<"Quoting style for response files, 'windows' (default) or 'posix'">;
 def dash_dash_version : Flag<["--"], "version">,
   HelpText<"Print version information">;
 defm threads: B<"threads",
     "Run the linker multi-threaded (default)",
     "Do not run the linker multi-threaded">;
 

lld/Common/ErrorHandler.cpp

 void ErrorHandler::warn(const Twine &Msg) {
   if (FatalWarnings) {
     error(Msg);
     return;
   }
 
   std::lock_guard<std::mutex> Lock(Mu);
   newline(ErrorOS, Msg);
+
+
+  if (WarningLimit == 0 || WarningCount < WarningLimit) {
     print("warning: ", raw_ostream::MAGENTA);
     *ErrorOS << Msg << "\n";
+  } else if (WarningCount == WarningLimit) {
+    print("warning: ", raw_ostream::MAGENTA);
+    *ErrorOS << WarningLimitExceededMsg << "\n";
+  }
+
+  ++WarningCount;
 }
 
 void ErrorHandler::error(const Twine &Msg) {
   std::lock_guard<std::mutex> Lock(Mu);
   newline(ErrorOS, Msg);
 
   if (ErrorLimit == 0 || ErrorCount < ErrorLimit) {
     print("error: ", raw_ostream::RED);

lld/ELF/Driver.cpp

 static void setConfigs(opt::InputArgList &Args);
 
 bool elf::link(ArrayRef<const char *> Args, bool CanExitEarly,
                raw_ostream &Error) {
   errorHandler().LogName = args::getFilenameWithoutExe(Args[0]);
   errorHandler().ErrorLimitExceededMsg =
       "too many errors emitted, stopping now (use "
       "-error-limit=0 to see all errors)";
+  errorHandler().WarningLimitExceededMsg =
+      "too many warnings emitted, stopping now (use "
+      "-warning-limit=0 to see all warnings)";
   errorHandler().ErrorOS = &Error;
   errorHandler().ExitEarly = CanExitEarly;
   errorHandler().ColorDiagnostics = Error.has_colors();
 
   InputSections.clear();
   OutputSections.clear();
   BinaryFiles.clear();
   BitcodeFiles.clear();

 }
 
 void LinkerDriver::main(ArrayRef<const char *> ArgsArr) {
   ELFOptTable Parser;
   opt::InputArgList Args = Parser.parse(ArgsArr.slice(1));
 
   // Interpret this flag early because error() depends on them.
   errorHandler().ErrorLimit = args::getInteger(Args, OPT_error_limit, 20);
+  errorHandler().WarningLimit = args::getInteger(Args, OPT_warning_limit, 20);
   checkZOptions(Args);
 
   // Handle -help
   if (Args.hasArg(OPT_help)) {
     printHelp();
     return;
   }
 

lld/ELF/Options.td

def end_group: F<"end-group">,
   HelpText<"Ignored for compatibility with GNU unless you pass --warn-backrefs">;
 
 def end_lib: F<"end-lib">,
   HelpText<"End a grouping of objects that should be treated as if they were together in an archive">;
 
 defm entry: Eq<"entry", "Name of entry point symbol">,
   MetaVarName<"<entry>">;
 
-defm error_limit:
-  Eq<"error-limit", "Maximum number of errors to emit before stopping (0 = no limit)">;
+defm error_limit: Eq<"error-limit",
+  "Maximum number of errors to emit before stopping (0 = no limit)">;
+defm warning_limit: Eq<"warning-limit",
+  "Maximum number of warnings to emit before stopping (0 = no limit)">;
+
 
 def error_unresolved_symbols: F<"error-unresolved-symbols">,
   HelpText<"Report unresolved symbols as errors">;
 
 defm exclude_libs: Eq<"exclude-libs", "Exclude static libraries from automatic export">;
 
 defm execute_only: B<"execute-only",
     "Mark executable sections unreadable",

lld/include/lld/Common/ErrorHandler.h

 
 namespace lld {
 
 class ErrorHandler {
 public:
   uint64_t ErrorCount = 0;
   uint64_t ErrorLimit = 20;
   StringRef ErrorLimitExceededMsg = "too many errors emitted, stopping now";
+  uint64_t WarningCount = 0;
+  uint64_t WarningLimit = 20;
+  StringRef WarningLimitExceededMsg = "too many warnings emitted, stopping now";
   StringRef LogName = "lld";
   llvm::raw_ostream *ErrorOS = &llvm::errs();
   bool ColorDiagnostics = llvm::errs().has_colors();
   bool ExitEarly = true;
   bool FatalWarnings = false;
   bool Verbose = false;
 
   void error(const Twine &Msg);

lld/lib/Driver/DarwinLdDriver.cpp

bool parse(llvm::ArrayRef<const char *> args, MachOLinkingContext &ctx) {
 
   for (auto unknownArg : parsedArgs.filtered(OPT_UNKNOWN)) {
     warn("ignoring unknown argument: " +
          Twine(unknownArg->getAsString(parsedArgs)));
   }
 
   errorHandler().Verbose = parsedArgs.hasArg(OPT_v);
   errorHandler().ErrorLimit = args::getInteger(parsedArgs, OPT_error_limit, 20);
+  errorHandler().WarningLimit =
+      args::getInteger(parsedArgs, OPT_warning_limit, 20);
 
   // Figure out output kind ( -dylib, -r, -bundle, -preload, or -static )
   llvm::MachO::HeaderFileType fileType = llvm::MachO::MH_EXECUTE;
   bool isStaticExecutable = false;
   if (llvm::opt::Arg *kind = parsedArgs.getLastArg(
           OPT_dylib, OPT_relocatable, OPT_bundle, OPT_static, OPT_preload)) {
     switch (kind->getOption().getID()) {
     case OPT_dylib:

 
 /// This is where the link is actually performed.
 bool link(llvm::ArrayRef<const char *> args, bool CanExitEarly,
           raw_ostream &Error) {
   errorHandler().LogName = args::getFilenameWithoutExe(args[0]);
   errorHandler().ErrorLimitExceededMsg =
       "too many errors emitted, stopping now (use "
       "'-error-limit 0' to see all errors)";
+  errorHandler().ErrorLimitExceededMsg =
+      "too many warnings emitted, stopping now (use "
+      "'-warning-limit 0' to see all warnings)";
   errorHandler().ErrorOS = &Error;
   errorHandler().ExitEarly = CanExitEarly;
   errorHandler().ColorDiagnostics = Error.has_colors();
 
   MachOLinkingContext ctx;
   if (!parse(args, ctx))
     return false;
   if (ctx.doNothing())

lld/lib/Driver/DarwinLdOptions.td

def rpath : Separate<["-"], "rpath">,
      MetaVarName<"<path>">,
      HelpText<"Add path to the runpath search path list for image being created">;
 
 def t : Flag<["-"], "t">,
      HelpText<"Print the names of the input files as ld processes them">;
 def v : Flag<["-"], "v">,
      HelpText<"Print linker information">;
 def error_limit : Separate<["-", "--"], "error-limit">,
-     MetaVarName<"<number>">,
-     HelpText<"Maximum number of errors to emit before stopping (0 = no limit)">;
+def warning_limit : Separate<["-", "--"], "warning-limit">,
+  MetaVarName<"<number>">,
+  HelpText<"Maximum number of warnings to emit before stopping (0 = no limit)">;
 
 // Ignored options
 def lto_library : Separate<["-"], "lto_library">,
     MetaVarName<"<path>">,
     HelpText<"Ignored for compatibility with other linkers">;
 
 // Obsolete options
 def grp_obsolete : OptionGroup<"obsolete">, HelpText<"OBSOLETE OPTIONS">;

lld/test/ELF/linkerscript/orphan-report.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
 # RUN: echo "SECTIONS { .text : { *(.text.1) } }" > %t.script
 
 ## Check we do not report orphans by default even with -verbose.
 # RUN: ld.lld -shared -o %t.out --script %t.script %t.o 2>&1 -verbose \
 # RUN:   | FileCheck %s --check-prefix=DEFAULT
 # DEFAULT-NOT: placed
 
 ## Check --orphan-handling=place has the same behavior as default.
 # RUN: ld.lld -shared --orphan-handling=place -o %t.out --script %t.script \
 # RUN:   %t.o 2>&1 -verbose  -error-limit=0 | FileCheck %s --check-prefix=DEFAULT
 
 ## Check --orphan-handling=error reports errors about orphans.
 # RUN: not ld.lld -shared --orphan-handling=error -o %t.out --script %t.script \
-# RUN:   %t.o 2>&1 -verbose  -error-limit=0 | FileCheck %s --check-prefix=REPORT
+# RUN:   %t.o 2>&1 -verbose -error-limit=0 | FileCheck %s --check-prefix=REPORT
 # REPORT:      {{.*}}.o:(.text) is being placed in '.text'
 # REPORT-NEXT: {{.*}}.o:(.text.2) is being placed in '.text'
 # REPORT-NEXT: <internal>:(.comment) is being placed in '.comment'
 # REPORT-NEXT: <internal>:(.bss) is being placed in '.bss'
 # REPORT-NEXT: <internal>:(.bss.rel.ro) is being placed in '.bss.rel.ro'
 # REPORT-NEXT: <internal>:(.dynsym) is being placed in '.dynsym'
 # REPORT-NEXT: <internal>:(.gnu.version) is being placed in '.gnu.version'
 # REPORT-NEXT: <internal>:(.gnu.version_r) is being placed in '.gnu.version_r'

 # REPORT-NEXT: <internal>:(.eh_frame) is being placed in '.eh_frame'
 # REPORT-NEXT: <internal>:(.symtab) is being placed in '.symtab'
 # REPORT-NEXT: <internal>:(.symtab_shndx) is being placed in '.symtab_shndx'
 # REPORT-NEXT: <internal>:(.shstrtab) is being placed in '.shstrtab'
 # REPORT-NEXT: <internal>:(.strtab) is being placed in '.strtab'
 
 ## Check --orphan-handling=warn reports warnings about orphans.
 # RUN: ld.lld -shared --orphan-handling=warn -o %t.out --script %t.script \
-# RUN:   %t.o 2>&1 -verbose | FileCheck %s --check-prefix=REPORT
+# RUN:  %t.o 2>&1 -verbose -warning-limit=0 | FileCheck %s --check-prefix=REPORT
 
 # RUN: not ld.lld --orphan-handling=foo -o %t.out --script %t.script %t.o 2>&1 \
 # RUN:   | FileCheck %s --check-prefix=UNKNOWN
 # UNKNOWN: unknown --orphan-handling mode: foo
 
 .section .text.1,"a"
  nop
 
 .section .text.2,"a"
  nop

lld/wasm/Driver.cpp

 bool lld::wasm::link(ArrayRef<const char *> Args, bool CanExitEarly,
                      raw_ostream &Error) {
   errorHandler().LogName = args::getFilenameWithoutExe(Args[0]);
   errorHandler().ErrorOS = &Error;
   errorHandler().ColorDiagnostics = Error.has_colors();
   errorHandler().ErrorLimitExceededMsg =
       "too many errors emitted, stopping now (use "
       "-error-limit=0 to see all errors)";
+  errorHandler().WarningLimitExceededMsg =
+      "too many warnings emitted, stopping now (use "
+      "-warning-limit=0 to see all warnings)";
 
   Config = make<Configuration>();
   Symtab = make<SymbolTable>();
 
   initLLVM();
   LinkerDriver().link(Args);
 
   // Exit immediately if we don't need to return to the caller.
void LinkerDriver::link(ArrayRef<const char *> ArgsArr) {
   // Parse and evaluate -mllvm options.
   std::vector<const char *> V;
   V.push_back("wasm-ld (LLVM option parsing)");
   for (auto *Arg : Args.filtered(OPT_mllvm))
     V.push_back(Arg->getValue());
   cl::ParseCommandLineOptions(V.size(), V.data());
 
   errorHandler().ErrorLimit = args::getInteger(Args, OPT_error_limit, 20);
+  errorHandler().WarningLimit = args::getInteger(Args, OPT_warning_limit, 20);
 
   setConfigs(Args);
   checkOptions(Args);
 
   if (auto *Arg = Args.getLastArg(OPT_allow_undefined_file))
     readImportFile(Arg->getValue());
 
   if (!Args.hasArg(OPT_INPUT)) {

lld/wasm/Options.td

defm export_dynamic: B<"export-dynamic",
     "Put symbols in the dynamic symbol table",
     "Do not put symbols in the dynamic symbol table (default)">;
 
 def entry: S<"entry">, MetaVarName<"<entry>">,
   HelpText<"Name of entry point symbol">;
 
 def error_limit: J<"error-limit=">,
   HelpText<"Maximum number of errors to emit before stopping (0 = no limit)">;
+def warning_limit: J<"warning-limit=">,
+  HelpText<"Maximum number of warnings to emit before stopping (0 = no limit)">;
 
 def fatal_warnings: F<"fatal-warnings">,
   HelpText<"Treat warnings as errors">;
 
 defm gc_sections: B<"gc-sections",
     "Enable garbage collection of unused sections",
     "Disable garbage collection of unused sections">;