Enabling stricter warnings for Windows builds

Project Infrastructure LLVM Dev List Archives
#1

I’ve been guilty several times recently of committing code that introduced build warnings. It’s a poor excuse, but my excuse is that I’ve been working on code for Windows and LLVM doesn’t enable strict warnings by default on Windows and produces nearly half a million warnings (literally) if you manually turn them on. As such, I thought I’d make an effort to see what it would take to clean things up on Windows.

Looking just at the main llvm project I found that despite the multitude of warnings, there were only 16 unique warnings being produced. I see in HandleLLVMOptions.cmake that there are already 17 warnings being explicitly disabled, so I thought it would be reasonable to start by adding most of the warnings that are currently being produced to that list.

I triaged the warnings that I was seeing according to how certain I was that disabling them was a reasonable thing to do. Here’s what I came up with (with a glaring bias toward just disabling common warnings and fixing uncommon ones):

Warnings that should almost certainly be disabled

#2

Enabling /W4 in MSVC and disabling things as necessary seems reasonable.

You can also self-host with clang-cl, and that will give you the same set of warnings as clang on other platforms. You can select the platform toolset in the VS IDE, as described at http://llvm.org/builds/. I think we are currently slower to compile than MSVC, though. =/

#3

As a caveat, I didn’t look at the code for any of these warnings -- not one
-- so I don’t know anything about how benign any of them might be.

I reckon you should be able to do about 12 per hour. I'm eagerly
awaiting your full report in 2020!

Tim.

#4

(thanks for pointing me to this thread - sorry I missed it)

#5

warning C4706: assignment within conditional expression (296 times)

Only 300? I was going to say that I assume this warning doesn’t support the double-paren suppression that GCC/Clang’s -Wparentheses dictate, but with such a low hit rate I wonder if it’s some other sub-set of cases MSVC is seeing here. (or maybe we don’t use even parentheses-bound assignment in conditionals very often at all)

Yeah, I expected more too. I haven’t looked at any of these to see what might explain it.

warning C4389: signed/unsigned mismatch (28 times)

Wonder how this is different from C4245?

C4245 seems to apply specifically to const values that the compiler knows to have a negative value.

See examples: https://msdn.microsoft.com/en-us/library/e9s7thk1.aspx

warning C4189: local variable is initialized but not referenced (6 times)

Clang diagnoses cases like this already - I wonder what subset MSVC is catching that Clang is not. If it’s types with non-trivial construction/destruction, then this seems like a warning we might not want to enable - it might false-positive on scoped devices, etc.

Some of these were cases where the code was getting an object pointer and then using it to call a static function. The other cause was places where a template parameter in a compound condition had the potential to short circuit evaluation of the condition and thus skip the only use of the local variable.

In the former case, fixing the code to avoid the warning seems like an improvement.

I don’t know if MSVC only warns in the second case if the template is actually instantiated with a value that short circuits the condition. If not, that one definitely seems like a nuisance but it was easy enough to fix in the places where it was being reported.

warning C4611: interaction between ‘_setjmp’ and C++ object destruction is non-portable (2 times)

Curious - again, worth comparing whatever false positives to whatever holes clang might have, etc.

This one looks potentially serious to me, but it isn’t obvious what to do about it so I ended up adding this to the list of warnings I’m going to just disable. It’s a design issue.

Both instances of the warning occurred in the same place, CrashRecoveryContext::runSafely().

It may be that the best thing to do here is to just document the potentially platform-dependent behavior if it really isn’t consistent across the platforms we support.

-Andy

#6

>> warning C4706: assignment within conditional expression (296 times)

> Only 300? I was going to say that I assume this warning doesn't support
the double-paren suppression that GCC/Clang's -Wparentheses dictate, but
with such a low hit rate I wonder if it's some other sub-set of cases MSVC
is seeing here. (or maybe we don't use even parentheses-bound assignment in
conditionals very often at all)

Yeah, I expected more too. I haven’t looked at any of these to see what
might explain it.

>> warning C4389: signed/unsigned mismatch (28 times)

> Wonder how this is different from C4245?

C4245 seems to apply specifically to const values that the compiler knows
to have a negative value.

See examples: https://msdn.microsoft.com/en-us/library/e9s7thk1.aspx

>> warning C4189: local variable is initialized but not referenced (6
times)

> Clang diagnoses cases like this already - I wonder what subset MSVC is
catching that Clang is not. If it's types with non-trivial
construction/destruction, then this seems like a warning we might not want
to enable - it might false-positive on scoped devices, etc.

Some of these were cases where the code was getting an object pointer and
then using it to call a static function. The other cause was places where
a template parameter in a compound condition had the potential to short
circuit evaluation of the condition and thus skip the only use of the local
variable.

In the former case, fixing the code to avoid the warning seems like an
improvement.

Yeah, sounds fine - but also doesn't sound like it's worth holding as a bar
for LLVM, so I'd still be sort of inclined to disable the warning rather
than produce noise for MSVC-using developers that the rest of the community
isn't really going to worry about (we'd never implement such a warning in
Clang, for example - I doubt it'd even be worth a clang-tidy warning, but
it'd be on the edge)

  I don’t know if MSVC only warns in the second case if the template is
actually instantiated with a value that short circuits the condition. If
not, that one definitely seems like a nuisance but it was easy enough to
fix in the places where it was being reported.

*nod*

>> warning C4611: interaction between '_setjmp' and C++ object destruction
is non-portable (2 times)

> Curious - again, worth comparing whatever false positives to whatever
holes clang might have, etc.

This one looks potentially serious to me, but it isn’t obvious what to do
about it so I ended up adding this to the list of warnings I’m going to
just disable. It’s a design issue.

Both instances of the warning occurred in the same place,
CrashRecoveryContext::runSafely().

It may be that the best thing to do here is to just document the
potentially platform-dependent behavior if it really isn’t consistent
across the platforms we support.

I wonder if the warning is only about the non-trivial function_ref object,
or whether it'd fire there regardless? It'd be interesting to know the
limitations/point of the warning in more detail. But yeah, given the small
number of cases it comes up in & it's not likely to get any worse - we're
hardly going to litter the codebase with setjmp/longjmp - so if the current
use case is safe (I've no idea), probably just disable the warning & move
on.

- David

#7

It would great to report the defective warnings to the VC folks so they fix them.