RESOLVED WONTFIX 246246
[CMake] Selectively treat some warnings as errors 
https://bugs.webkit.org/show_bug.cgi?id=246246
Summary [CMake] Selectively treat some warnings as errors
Adrian Perez
Reported 2022-10-08 04:16:08 PDT
There are certain compiler warnings that we would like to have always treated as errors, but at the same time treating *all* warnings as errors with -Werror can be complicated (see #155047 for discussion on this topic). Nevertheless, there may be certain warnings that we would prefer to *always* have as errors in developer builds, even in those cases where DEVELOPER_MODE_FATAL_WARNINGS is disabled. With -Werror=name we can selectively opt-in to those we want to keep at bay no matter what. This idea crossed my mind when I discovered Clang has now some handy warnings: -Wundefined-internal -Wundefined-inline -Wundefined-func-template These will warn if a static function, an inline function, or a function template have been forward-declared, but the definition has not been brought into scope in the compilation unit where they are used. This currently continues building, but results in linker errors which are most of the time cryptic and do not directly point to the issue at hand. Coincidentally: a reason why I used LLD first for years, and a while ago I imported the Mold linker in the Flatpak SDK is because they tend to provide slightly better diagnostics for this kind of thing--but the compiler erroring earlier makes *so much easier* to fix things! So at first I would want to add the above three with -Werror=name to CMake developer builds, see how that goes, and maybe later start adding more opt-ins for treating warnings as errors. Hopefully slowly boiling the frog this way can get steer us towards DEVELOPER_MODE_FATAL_WARNINGS being always enabled :)
Attachments
Add attachment proposed patch, testcase, etc.
Adrian Perez
Comment 1 2022-10-08 04:23:16 PDT
(In reply to Adrian Perez from comment #0) > -Wundefined-internal > -Wundefined-inline > -Wundefined-func-template Ah, actually the last one I am going to drop from the proposal, because often template specializations are defined in .cpp files and used from other compilation units. We would get too many unsolvable errors from it.
Michael Catanzaro
Comment 2 2022-10-08 15:05:21 PDT
I agree that incremental progress is better than no progress. That said, I don't think the warnings you've selected will really make any difference since I've never seen those warnings occur in WebKit ever. The most important warnings that I would start with are -Wredundant-move, -Wunused-variable, and -Wreturn-type. Instead of enabling them always, I would just use them as a temporary implementation of DEVELOPER_MODE_FATAL_WARNINGS until we're able to really make everything fatal.
Michael Catanzaro
Comment 3 2022-10-08 15:12:04 PDT
(Also I'm not particularly fond of the approach of cherry-picking a bunch of particular warnings to be permanently fatal, since deciding which warnings to add to the list is quite arbitrary. Temporarily targeting the few that occur commonly in WebKit makes more sense to me.)
Adrian Perez
Comment 4 2022-10-10 04:01:59 PDT
OTOH, I think we may want to have these enabled: -Wabstract-final-clas(In reply to Michael Catanzaro from comment #2) > I agree that incremental progress is better than no progress. That said, I > don't think the warnings you've selected will really make any difference > since I've never seen those warnings occur in WebKit ever. The most > important warnings that I would start with are -Wredundant-move, > -Wunused-variable, and -Wreturn-type. > > Instead of enabling them always, I would just use them as a temporary > implementation of DEVELOPER_MODE_FATAL_WARNINGS until we're able to really > make everything fatal. The two I started with are supported only by Clang, and you are most likely to see them happen with non-unified builds. They greatly help figure out where a FooInlines.h header is missing, which normally would cause the linker to produce errors of varying degrees of cryptic-ness. I am slowly adding a few more, probably to end up including the ones you also suggest. I want the ones we add this way to be 100% non-controversial so we can have them always on, no exceptions.
Michael Catanzaro
Comment 5 2022-10-19 13:15:41 PDT
(In reply to Adrian Perez from comment #4) > I want the ones we add this way to be 100% non-controversial so we can > have them always on, no exceptions. It complicates things a bit: for them to be always on, we really cannot ever use a warning that has any chance of becoming stricter or more sensitive in the future. We can enable fatal warnings more aggressively if we don't have to worry about that, so my preference is to gate all the fatal warnings behind that DEVELOPER_MODE_FATAL_WARNINGS flag.
Michael Catanzaro
Comment 6 2022-10-20 14:52:58 PDT
So I looked at the EWS to see how bad things are, https://github.com/WebKit/WebKit/pull/5559. We do have some warnings that need fixed: /app/webkit/Source/WebCore/crypto/openssl/OpenSSLCryptoUniquePtr.h:57:64: warning: ‘void RSA_free(RSA*)’ is deprecated: Since OpenSSL 3.0 [-Wdeprecated-declarations] /app/webkit/Source/WebCore/crypto/openssl/OpenSSLCryptoUniquePtr.h:57:64: warning: ‘void EC_KEY_free(EC_KEY*)’ is deprecated: Since OpenSSL 3.0 [-Wdeprecated-declarations] /app/webkit/Source/WebCore/crypto/openssl/OpenSSLCryptoUniquePtr.h:57:64: warning: ‘void HMAC_CTX_free(HMAC_CTX*)’ is deprecated: Since OpenSSL 3.0 [-Wdeprecated-declarations] /app/webkit/Source/WebCore/Modules/mediastream/SFrameUtils.cpp:139:16: warning: ‘void* memcpy(void*, const void*, size_t)’ accessing 18446744073709551614 bytes at offsets 0 and 0 overlaps 9223372036854775805 bytes at offset -9223372036854775807 [-Wrestrict] /app/webkit/Source/WebCore/platform/VideoPixelFormat.cpp:36:69: warning: unused parameter ‘shouldDiscardAlpha’ [-Wunused-parameter] /home/buildbot/igalia-jsc32-armv7-ews/JSC-ARMv7-32bits-Build-EWS/build/Source/ThirdParty/capstone/Source/cs.c:1015:26: warning: cast increases required alignment of target type [-Wcast-align] /home/buildbot/igalia-jsc32-armv7-ews/JSC-ARMv7-32bits-Build-EWS/build/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:9745:21: warning: unused variable ‘globalObject’ [-Wunused-variable] /home/buildbot/igalia-jsc32-armv7-ews/JSC-ARMv7-32bits-Build-EWS/build/Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:598:21: warning: unused variable ‘globalObject’ [-Wunused-variable] /home/buildbot/igalia-jsc32-armv7-ews/JSC-ARMv7-32bits-Build-EWS/build/Source/JavaScriptCore/jit/GCAwareJITStubRoutine.cpp:61:20: warning: operation on ‘*(JSC::GCAwareJITStubRoutine*)this’ may be undefined [-Wsequence-point] /home/buildbot/igalia-jsc32-armv7-ews/JSC-ARMv7-32bits-Build-EWS/build/Source/JavaScriptCore/jit/GCAwareJITStubRoutine.cpp:76:16: warning: operation on ‘*(JSC::GCAwareJITStubRoutine*)this’ may be undefined [-Wsequence-point] /home/buildbot/igalia-jsc32-armv7-ews/JSC-ARMv7-32bits-Build-EWS/build/Source/JavaScriptCore/parser/Lexer.cpp:1845:11: warning: variable ‘mergedCharacterBits’ set but not used [-Wunused-but-set-variable] But this is much fewer than I was expecting. I don't think we'll need to pick and choose here: we should be able to use full -Werror. The post-commit bots may need additional attention, though, since there are many more supported configurations there. I have reported bug #246760 for that.
Michael Catanzaro
Comment 7 2022-10-21 11:22:58 PDT
Since there are way fewer warnings to fix than I had expected, let's jump straight to using full using -Werror instead of using -Werror= for particular warnings. I have reported bug #246871 for this. Unless something goes unexpectedly wrong, we shouldn't need this bug anymore, so I'll close it for now. But of course, it's OK to change our minds....
Note You need to log in before you can comment on or make changes to this bug.