llvm dev - Mar 2015 - [LLVMdev] Enabling stricter warnings for Windows builds

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
-------------------------------------------------------------
warning C4100: unreferenced formal parameter (263666 times)
warning C4127: conditional expression is constant (101120 times)
warning C4512: assignment operator could not be generated (40472 times)
warning C4505: unreferenced local function has been removed (6606 times)
warning C4610: [...] can never be instantiated (1714 times)
warning C4510: default constructor could not be generated (1714 times)
warning C4702: unreachable code (1343 times)
warning C4706: assignment within conditional expression (296 times)


Warnings that seem like they should be fixed but will take a little time to
correct
--------------------------------------------------------------------------------------------------
warning C4245: signed/unsigned mismatch (962 times)
warning C4310: cast truncates constant value (216 times)
warning C4701: potentially uninitialized local variable (123 times)
warning C4703: potentially uninitialized local pointer variable (40 times)
warning C4389: signed/unsigned mismatch (28 times)


Warnings that should probably be fixed
-------------------------------------------------
warning C4189: local variable is initialized but not referenced (6 times)
warning C4204: nonstandard extension used : non-constant aggregate initializer
(4 times)
warning C4611: interaction between '_setjmp' and C++ object destruction
is non-portable (2 times)


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 also
don't know why there are two different "signed/unsigned mismatch"
warnings (though I'm guessing one is assignments and the other is
comparisons).

I have a local build going right now with the first two groups of warnings
disabled.  If it goes as expected, I'd like to commit this change along with
fixes for the very small last group.  Then, after a reasonable adjustment period
to let subprojects see how this looks, I'd like to turn on the "all
warnings" option by default for Windows builds.

Does anyone see any problems with the way I have categorized these warnings?

Thanks,
Andy

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150320/935264c5/attachment.html>

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. =/

On Fri, Mar 20, 2015 at 4:26 PM, Kaylor, Andrew <andrew.kaylor at
intel.com>
wrote:
>  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
>
> -------------------------------------------------------------
>
> warning C4100: unreferenced formal parameter (263666 times)
>
> warning C4127: conditional expression is constant (101120 times)
>
> warning C4512: assignment operator could not be generated (40472 times)
>
> warning C4505: unreferenced local function has been removed (6606 times)
>
> warning C4610: [...] can never be instantiated (1714 times)
>
> warning C4510: default constructor could not be generated (1714 times)
>
> warning C4702: unreachable code (1343 times)
>
> warning C4706: assignment within conditional expression (296 times)
>
>
>
>
>
> Warnings that seem like they should be fixed but will take a little time
> to correct
>
>
>
--------------------------------------------------------------------------------------------------
>
> warning C4245: signed/unsigned mismatch (962 times)
>
> warning C4310: cast truncates constant value (216 times)
>
> warning C4701: potentially uninitialized local variable (123 times)
>
> warning C4703: potentially uninitialized local pointer variable (40 times)
>
> warning C4389: signed/unsigned mismatch (28 times)
>
>
>
>
>
> Warnings that should probably be fixed
>
> -------------------------------------------------
>
> warning C4189: local variable is initialized but not referenced (6 times)
>
> warning C4204: nonstandard extension used : non-constant aggregate
> initializer (4 times)
>
> warning C4611: interaction between '_setjmp' and C++ object
destruction is
> non-portable (2 times)
>
>
>
>
>
> 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
> also don’t know why there are two different “signed/unsigned mismatch”
> warnings (though I’m guessing one is assignments and the other is
> comparisons).
>
>
>
> I have a local build going right now with the first two groups of warnings
> disabled.  If it goes as expected, I’d like to commit this change along
> with fixes for the very small last group.  Then, after a reasonable
> adjustment period to let subprojects see how this looks, I’d like to turn
> on the “all warnings” option by default for Windows builds.
>
>
>
> Does anyone see any problems with the way I have categorized these
> warnings?
>
>
>
> Thanks,
>
> Andy
>
>
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150320/eb3987cf/attachment.html>

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

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

On Fri, Mar 20, 2015 at 4:26 PM, Kaylor, Andrew <andrew.kaylor at
intel.com>
wrote:
>  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
>
> -------------------------------------------------------------
>
> warning C4100: unreferenced formal parameter (263666 times)
>
> warning C4127: conditional expression is constant (101120 times)
>
> warning C4512: assignment operator could not be generated (40472 times)
>
> warning C4505: unreferenced local function has been removed (6606 times)
>
This ^ is curious - I guess there's some misfeature in MSVC's
implementation of this warning? Clang & GCC have a -Wunused-function which
we have turned on and LLVM/Clang/etc build clean of this warning.

>  warning C4610: [...] can never be instantiated (1714 times)
>
Curious (again, given the high number of instances, I assume the warning is
overlooking something) - wonder what the deal is there. But just mild
curiosity, nothing important.

>  warning C4510: default constructor could not be generated (1714 times)
>
> warning C4702: unreachable code (1343 times)
>
Even Clang's improved -Wunreachable-code isn't on by default yet. Maybe
one
day - it needs more cleanup.

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

>
>  Warnings that seem like they should be fixed but will take a little time
> to correct
>
>
>
--------------------------------------------------------------------------------------------------
>
> warning C4245: signed/unsigned mismatch (962 times)
>
If we want to hold this bar, we should consider enabling Clang/GCC's
-Wsign-compare?

>  warning C4310: cast truncates constant value (216 times)
>
Again, could be improvements to Clang -Wconstant-conversion to catch these.

>  warning C4701: potentially uninitialized local variable (123 times)
>
> warning C4703: potentially uninitialized local pointer variable (40 times)
>
These two are /probably/ not worth it (but worth checking a sample to see)
- Clang's -Wsometimes-uninitialized is designed to have a low/zero false
positive rate while diagnosing cases like this.

>  warning C4389: signed/unsigned mismatch (28 times)
>
Wonder how this is different from C4245?

>
>  Warnings that should probably be fixed
>
> -------------------------------------------------
>
> 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.

>  warning C4204: nonstandard extension used : non-constant aggregate
> initializer (4 times)
>
If this is a nonstandard extension we want to avoid then maybe breaking it
out of Clang's -Wpedantic (assuming it's there) into a separate flag
(assuming it's not already under a separate flag) & enabling both MSVC
and
Clang's would be fine.

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

>
>
>
>
> 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
> also don’t know why there are two different “signed/unsigned mismatch”
> warnings (though I’m guessing one is assignments and the other is
> comparisons).
>
Perhaps?

>
>
> I have a local build going right now with the first two groups of warnings
> disabled.  If it goes as expected, I’d like to commit this change along
> with fixes for the very small last group.  Then, after a reasonable
> adjustment period to let subprojects see how this looks, I’d like to turn
> on the “all warnings” option by default for Windows builds.
>
>
>
> Does anyone see any problems with the way I have categorized these
> warnings?
>
Just to summarize from the other thread:

My understanding and preference from past discussions is essentially: If it
isn't a diagnostic that's sufficiently valuable/correct/etc to implement
in
Clang, it's not worth holding the LLVM codebase to that standard and we
should just disable whatever warning it is. I think that's been the general
philosophy in the past, to the best of my understanding.



- David

>
>
> Thanks,
>
> Andy
>
>
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150323/8d63c931/attachment.html>

>> 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150324/d453f76e/attachment.html>

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


On Monday, March 23, 2015, David Blaikie <dblaikie at gmail.com> wrote:
> (thanks for pointing me to this thread - sorry I missed it)
>
> On Fri, Mar 20, 2015 at 4:26 PM, Kaylor, Andrew <andrew.kaylor at
intel.com
> <javascript:_e(%7B%7D,'cvml','andrew.kaylor at
intel.com');>> wrote:
>
>>  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
>>
>> -------------------------------------------------------------
>>
>> warning C4100: unreferenced formal parameter (263666 times)
>>
>> warning C4127: conditional expression is constant (101120 times)
>>
>> warning C4512: assignment operator could not be generated (40472 times)
>>
>> warning C4505: unreferenced local function has been removed (6606
times)
>>
>
> This ^ is curious - I guess there's some misfeature in MSVC's
> implementation of this warning? Clang & GCC have a -Wunused-function
which
> we have turned on and LLVM/Clang/etc build clean of this warning.
>
>
>>  warning C4610: [...] can never be instantiated (1714 times)
>>
>
> Curious (again, given the high number of instances, I assume the warning
> is overlooking something) - wonder what the deal is there. But just mild
> curiosity, nothing important.
>
>
>>  warning C4510: default constructor could not be generated (1714 times)
>>
>> warning C4702: unreachable code (1343 times)
>>
>
> Even Clang's improved -Wunreachable-code isn't on by default yet.
Maybe
> one day - it needs more cleanup.
>
>
>>  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)
>
>
>>
>>  Warnings that seem like they should be fixed but will take a little
>> time to correct
>>
>>
>>
--------------------------------------------------------------------------------------------------
>>
>> warning C4245: signed/unsigned mismatch (962 times)
>>
>
> If we want to hold this bar, we should consider enabling Clang/GCC's
> -Wsign-compare?
>
>
>>  warning C4310: cast truncates constant value (216 times)
>>
>
> Again, could be improvements to Clang -Wconstant-conversion to catch these.
>
>
>>  warning C4701: potentially uninitialized local variable (123 times)
>>
>> warning C4703: potentially uninitialized local pointer variable (40
times)
>>
>
> These two are /probably/ not worth it (but worth checking a sample to see)
> - Clang's -Wsometimes-uninitialized is designed to have a low/zero
false
> positive rate while diagnosing cases like this.
>
>
>>  warning C4389: signed/unsigned mismatch (28 times)
>>
>
> Wonder how this is different from C4245?
>
>
>>
>>  Warnings that should probably be fixed
>>
>> -------------------------------------------------
>>
>> 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.
>
>
>>  warning C4204: nonstandard extension used : non-constant aggregate
>> initializer (4 times)
>>
>
> If this is a nonstandard extension we want to avoid then maybe breaking it
> out of Clang's -Wpedantic (assuming it's there) into a separate
flag
> (assuming it's not already under a separate flag) & enabling both
MSVC and
> Clang's would be fine.
>
>
>>  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.
>
>
>>
>>
>>
>>
>> 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
>> also don’t know why there are two different “signed/unsigned mismatch”
>> warnings (though I’m guessing one is assignments and the other is
>> comparisons).
>>
>
> Perhaps?
>
>
>>
>>
>> I have a local build going right now with the first two groups of
>> warnings disabled.  If it goes as expected, I’d like to commit this
change
>> along with fixes for the very small last group.  Then, after a
reasonable
>> adjustment period to let subprojects see how this looks, I’d like to
turn
>> on the “all warnings” option by default for Windows builds.
>>
>>
>>
>> Does anyone see any problems with the way I have categorized these
>> warnings?
>>
>
> Just to summarize from the other thread:
>
> My understanding and preference from past discussions is essentially: If
> it isn't a diagnostic that's sufficiently valuable/correct/etc to
implement
> in Clang, it's not worth holding the LLVM codebase to that standard and
we
> should just disable whatever warning it is. I think that's been the
general
> philosophy in the past, to the best of my understanding.
>
>
>
> - David
>
>
>>
>>
>> Thanks,
>>
>> Andy
>>
>>
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> LLVMdev at cs.uiuc.edu
<javascript:_e(%7B%7D,'cvml','LLVMdev at cs.uiuc.edu');>
>>        http://llvm.cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150407/9e567c79/attachment.html>