[llvm-dev] Miscellaneous warnings in code using Visual Studio

[Paul C. Anagnostopoulos via llvm-dev]

When I build on Windows 7 with Visual Studio, I get various warnings that were ignored until I recently set

LLVM_ENABLE_WERROR:BOOL=ON

One example is shown below. Should I take the time to hunt these down and fix them?

**********************************************************************
** Visual Studio 2019 Developer Command Prompt v16.8.1
** Copyright (c) 2020 Microsoft Corporation
**********************************************************************

D:\LLVM\build>build-targets.bat
1 file(s) copied.
[363/1578] Building CXX object lib\CodeGen\...LLVMGlobalISel.dir\MachineIRBuilder.cpp.obj
FAILED: lib/CodeGen/GlobalISel/CMakeFiles/LLVMGlobalISel.dir/MachineIRBuilder.cpp.obj
C:\PROGRA~2\MIB055~1\2019\COMMUN~1\VC\Tools\MSVC\1428~1.293\bin\Hostx86\x86\cl.exe /nolog
o /TP -DGTEST_HAS_RTTI=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS
-D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_FILE_OFFSET_BITS=64 -D_HAS_EXCEPTI
ONS=0 -D_LARGEFILE_SOURCE -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE
-D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib\CodeGen\Global
ISel -IC:\LLVM\llvm-project\llvm\lib\CodeGen\GlobalISel -Iinclude -IC:\LLVM\llvm-project\l
lvm\include /DWIN32 /D_WINDOWS /WX /Zc:inline /Zc:__cplusplus /Zc:strictStrings /Oi /Zc:
rvalueCast /bigobj /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd
4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd47
02 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091
-wd4592 -wd4319 -wd4709 -wd4324 -w14062 -we4238 /MDd /Zi /Ob0 /Od /RTC1 /EHs-c- /GR- -st
d:c++14 /showIncludes /Folib\CodeGen\GlobalISel\CMakeFiles\LLVMGlobalISel.dir\MachineIRBui
lder.cpp.obj /Fdlib\CodeGen\GlobalISel\CMakeFiles\LLVMGlobalISel.dir\LLVMGlobalISel.pdb /F
S -c C:\LLVM\llvm-project\llvm\lib\CodeGen\GlobalISel\MachineIRBuilder.cpp
C:\LLVM\llvm-project\llvm\lib\CodeGen\GlobalISel\MachineIRBuilder.cpp(657): error C2220: t
he following warning is treated as an error
C:\LLVM\llvm-project\llvm\lib\CodeGen\GlobalISel\MachineIRBuilder.cpp(657): warning C4018:
'>=': signed/unsigned mismatch
[366/1578] Building CXX object lib\CodeGen\...CMakeFiles\LLVMGlobalISel.dir\Utils.cpp.obj
ninja: build stopped: subcommand failed.

_______________________________________________
LLVM Developers mailing list
llvm...@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

[David Blaikie via llvm-dev]

If they prove to be good cleanup/improvements to the code in general - but we don't try too hard to be -Werror clean on every compiler, mostly just self-hosted clang. The line is fuzzy - if a warning isn't /too/ bad (ie: doesn't require extreme contortions to the code to address the warning) then fixing instances seems ok. If it's unhelpful (eg: a warning has a high false positive rate and/or clang's equivalent warning is more nuanced and avoids the need to touch false positives) we may disable warnings in LLVM. (this doesn't address the issue of warnings in LLVM's public headers - not every downstream consumer of LLVM is going to disable the same set of warnings, so sometimes it's necessary to do some warning cleanup even for undesirable warnings)

[Paul C. Anagnostopoulos via llvm-dev]

The problem with not fixing them is that I cannot enable WERROR. So warnings in my own code might zoom by unseen and then bust the build, as happened the other day. Of course, it's possible that warning was one produced by the build's compiler but not by Visual Studio's.

I could always enable it just for a build of my code, then disable it again.

At 1/12/2021 12:17 PM, David Blaikie wrote:
>If they prove to be good cleanup/improvements to the code in general - but we don't try too hard to be -Werror clean on every compiler, mostly just self-hosted clang. The line is fuzzy - if a warning isn't /too/ bad (ie: doesn't require extreme contortions to the code to address the warning) then fixing instances seems ok. If it's unhelpful (eg: a warning has a high false positive rate and/or clang's equivalent warning is more nuanced and avoids the need to touch false positives) we may disable warnings in LLVM. (this doesn't address the issue of warnings in LLVM's public headers - not every downstream consumer of LLVM is going to disable the same set of warnings, so sometimes it's necessary to do some warning cleanup even for undesirable warnings)

_______________________________________________

[Aaron Ballman via llvm-dev]

On Tue, Jan 12, 2021 at 12:17 PM David Blaikie via llvm-dev
<llvm...@lists.llvm.org> wrote:
>
> If they prove to be good cleanup/improvements to the code in general - but we don't try too hard to be -Werror clean on every compiler, mostly just self-hosted clang.

This doesn't match my understanding -- we have a documented CMake
option that controls -Werror and I think we've always tried to be
-Werror clean with Clang, GCC, and MSVC builds before to support that
build configuration.

> The line is fuzzy - if a warning isn't /too/ bad (ie: doesn't require extreme contortions to the code to address the warning) then fixing instances seems ok. If it's unhelpful (eg: a warning has a high false positive rate and/or clang's equivalent warning is more nuanced and avoids the need to touch false positives) we may disable warnings in LLVM. (this doesn't address the issue of warnings in LLVM's public headers - not every downstream consumer of LLVM is going to disable the same set of warnings, so sometimes it's necessary to do some warning cleanup even for undesirable warnings)

I agree that we don't want large amounts of code churn for warnings
that are typically off by default in our default cmake configuration
(or could be made off by default if they're sufficiently low quality),
but I think this situation is different. I think these new warnings
were recently introduced and are ones we would typically fix with an
NFC patch.

~Aaron

[David Blaikie via llvm-dev]

Yep, it's a tradeoff to be sure. I'd recommend, if possible, self-hosting (using clang to build your development clang) - but otherwise, yes, fixes to other compiler warnings are welcome but in some cases it may be desirable to disable  a warning instead (& realize this kind of thing will regress easily due to other people's contributions since they aren't using MSVC).

[David Blaikie via llvm-dev]

On Tue, Jan 12, 2021 at 9:34 AM Aaron Ballman <aa...@aaronballman.com> wrote:

On Tue, Jan 12, 2021 at 12:17 PM David Blaikie via llvm-dev
<llvm...@lists.llvm.org> wrote:
>
> If they prove to be good cleanup/improvements to the code in general - but we don't try too hard to be -Werror clean on every compiler, mostly just self-hosted clang.

This doesn't match my understanding -- we have a documented CMake
option that controls -Werror and I think we've always tried to be
-Werror clean with Clang, GCC, and MSVC builds before to support that
build configuration.

Certainly I don't think we've ever outright rejected any path forward for avoiding a warning from any supported compiler (either by changing the code or suppressing the warning in the LLVM build in general) - though I don't think practically speaking we try very hard to keep all the supported compiler versions (not just brands, but the full supported version range of each) -Werror clean - about the cleanest would be a selfhost build, there are many buildbots/etc that check that is -Werror clean.
 

[Aaron Ballman via llvm-dev]

On Tue, Jan 12, 2021 at 12:37 PM David Blaikie <dbla...@gmail.com> wrote:
>
>
>
> On Tue, Jan 12, 2021 at 9:34 AM Aaron Ballman <aa...@aaronballman.com> wrote:
>>
>> On Tue, Jan 12, 2021 at 12:17 PM David Blaikie via llvm-dev
>> <llvm...@lists.llvm.org> wrote:
>> >
>> > If they prove to be good cleanup/improvements to the code in general - but we don't try too hard to be -Werror clean on every compiler, mostly just self-hosted clang.
>>
>> This doesn't match my understanding -- we have a documented CMake
>> option that controls -Werror and I think we've always tried to be
>> -Werror clean with Clang, GCC, and MSVC builds before to support that
>> build configuration.
>
>
> Certainly I don't think we've ever outright rejected any path forward for avoiding a warning from any supported compiler (either by changing the code or suppressing the warning in the LLVM build in general) - though I don't think practically speaking we try very hard to keep all the supported compiler versions (not just brands, but the full supported version range of each) -Werror clean - about the cleanest would be a selfhost build, there are many buildbots/etc that check that is -Werror clean.

My recollection is that we only require the selfhost build to be
-Werror clean (with a bot configured to test that) but that we strive
to be -Werror clean when it won't introduce unwanted changes to the
code (in which case, we're happier to disable the warning within
CMake). So I think we may be saying the same things with different
words, but maybe we have a different definition of "try very hard"
here. :-)

~Aaron

[David Blaikie via llvm-dev]

On Tue, Jan 12, 2021 at 9:42 AM Aaron Ballman <aa...@aaronballman.com> wrote:

On Tue, Jan 12, 2021 at 12:37 PM David Blaikie <dbla...@gmail.com> wrote:
>
>
>
> On Tue, Jan 12, 2021 at 9:34 AM Aaron Ballman <aa...@aaronballman.com> wrote:
>>
>> On Tue, Jan 12, 2021 at 12:17 PM David Blaikie via llvm-dev
>> <llvm...@lists.llvm.org> wrote:
>> >
>> > If they prove to be good cleanup/improvements to the code in general - but we don't try too hard to be -Werror clean on every compiler, mostly just self-hosted clang.
>>
>> This doesn't match my understanding -- we have a documented CMake
>> option that controls -Werror and I think we've always tried to be
>> -Werror clean with Clang, GCC, and MSVC builds before to support that
>> build configuration.
>
>
> Certainly I don't think we've ever outright rejected any path forward for avoiding a warning from any supported compiler (either by changing the code or suppressing the warning in the LLVM build in general) - though I don't think practically speaking we try very hard to keep all the supported compiler versions (not just brands, but the full supported version range of each) -Werror clean - about the cleanest would be a selfhost build, there are many buildbots/etc that check that is -Werror clean.

My recollection is that we only require the selfhost build to be
-Werror clean (with a bot configured to test that) but that we strive
to be -Werror clean when it won't introduce unwanted changes to the
code (in which case, we're happier to disable the warning within
CMake). So I think we may be saying the same things with different
words, but maybe we have a different definition of "try very hard"
here. :-)

Yep yep. Sounds roughly similar
 

[Chris Tetreault via llvm-dev]

As a fellow Visual Studio user, I feel your pain. However, trying to fix these warnings is a losing battle. Most LLVM developers do not use Visual Studio, so my experience tells me that new warnings will be added faster than you can fix them. This is not a indictment of the community; it's just that there are so many compilers out there and they all have different warnings. Being werror clean across compilers is just not manageable.

As the code owner of tablegen, it may be worth it to get your bit of the codebase close to being werror clean just for your personal productivity. I don't know how hard that would be, some parts of the codebase are pretty gross in MSVC.

Thanks,
Christopher Tetreault

[via llvm-dev]

Yeah, periodically I contemplate doing some patches to reduce the noise
from MSVC, but the annoyance has never risen to the level of deciding
to act on it.
There is one module whose contents are completely conditionalized out
on Windows, but I think that produces a linker warning rather than a
compiler warning, so I wouldn't expect Werror to turn that into a hard
error.
--paulr

> https://urldefense.com/v3/__https://lists.llvm.org/cgi-
> bin/mailman/listinfo/llvm-dev__;!!JmoZiZGBv3RvKRSx!v9R-
> rlLFi_Ghr5_kcghnnBpBD15R43Nl8Gkw6cXT5D0796AN5sKYlukrjHDP-Q5dyw$

> _______________________________________________
> LLVM Developers mailing list
> llvm...@lists.llvm.org

> https://urldefense.com/v3/__https://lists.llvm.org/cgi-
> bin/mailman/listinfo/llvm-dev__;!!JmoZiZGBv3RvKRSx!v9R-
> rlLFi_Ghr5_kcghnnBpBD15R43Nl8Gkw6cXT5D0796AN5sKYlukrjHDP-Q5dyw$

[Adrian McCarthy via llvm-dev]

In LLDB, we've been moderately aggressive at fixing warnings for MSVC.

The particular example given (`assert(uint16_t + uint16_t >= size_t)`) is almost certainly harmless, especially since it's in an assertion.

I have, however, spent a lot of time chasing bugs (in other projects) that could have been found immediately by enabling signed/unsigned comparison warnings.  As a result, I'm particularly uncomfortable ignoring those.  I've developed fluency in the integral conversion rules, but I'm not sure every C++ programmer would immediately recognize where the signed value arises given that the expression involves three unsigned values.  And it's exactly those kinds of implicit conversions that lead to surprising bugs, and it's why it's a reasonable warning to pay attention to.

[Machiel van Hooren via llvm-dev]

I've had similar issues with warnings from LLVM headers with Visual Studio.
The way I've dealt with this is this to use the experimental /external
compiler flags within Visual Studio which allow control of warning
levels from external headers.

For example adding these flags will set warning level to W0 for all
include files that are included with angle brackets:

/experimental:external /external:anglebrackets /external:W0

This is a bit crude because you can't target specific libraries, but it
might still be helpful depending on your application.

Regards,

Machiel van Hooren