Page MenuHomePhabricator

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

Authored by thakis on May 8 2019, 6:15 PM.

Details

Reviewers
ruiu
espindola
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.)

Diff Detail

Event Timeline

thakis created this revision.May 8 2019, 6:15 PM
Herald added a project: Restricted Project. · View Herald Transcript
ruiu added a comment.May 8 2019, 6:29 PM

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.

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?

  • 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 added a comment.May 8 2019, 10:46 PM
  • 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.

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 added a comment.May 9 2019, 1:16 AM

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().

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.

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 added a comment.May 9 2019, 2:49 AM

Yes, a macro works too.

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

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 added a comment.May 9 2019, 3:16 AM

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 added a subscriber: grimar.May 13 2019, 6:05 AM
grimar added inline comments.
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 abandoned this revision.May 13 2019, 10:00 AM

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.