Bug 246246

Summary:	[CMake] Selectively treat some warnings as errors
Product:	WebKit	Reporter:	Adrian Perez <aperez>
Component:	Tools / Tests	Assignee:	Adrian Perez <aperez>
Status:	RESOLVED WONTFIX
Severity:	Normal	CC:	don.olmstead, mcatanzaro
Priority:	P2
Version:	WebKit Nightly Build
Hardware:	Unspecified
OS:	Unspecified
See Also:	https://bugs.webkit.org/show_bug.cgi?id=155047 https://bugs.webkit.org/show_bug.cgi?id=246760 https://bugs.webkit.org/show_bug.cgi?id=246861 https://bugs.webkit.org/show_bug.cgi?id=246871

Description Adrian Perez 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 :)

Comment 1 Adrian Perez 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.

Comment 2 Michael Catanzaro 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.

Comment 3 Michael Catanzaro 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.)

Comment 4 Adrian Perez 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.

Comment 5 Michael Catanzaro 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.

Comment 6 Michael Catanzaro 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.

Comment 7 Michael Catanzaro 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....