Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

very loud warnings output from cmake VS2019+Clang builds #4983

Closed
sezero opened this issue Nov 20, 2021 · 13 comments
Closed

very loud warnings output from cmake VS2019+Clang builds #4983

sezero opened this issue Nov 20, 2021 · 13 comments

Comments

@sezero
Copy link
Contributor

sezero commented Nov 20, 2021

See e.g. recent Github CI build logs.

Cmake seems to enable both gcc -Wall and also MSVC /W3, the latter
possibly is the culprit?

@sezero
Copy link
Contributor Author

sezero commented Nov 20, 2021

Tried the following, no change

diff --git a/CMakeLists.txt b/CMakeLists.txt
index e3d3b14..f25d33c 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -257,6 +257,14 @@ if(MSVC)
     endforeach(flag_var)
   endif()
 endif()
+if(MSVC_CLANG)
+  # silence clang-cl builds a bit
+  foreach(flag_var
+      CMAKE_C_FLAGS CMAKE_C_FLAGS_DEBUG CMAKE_C_FLAGS_RELEASE
+      CMAKE_C_FLAGS_MINSIZEREL CMAKE_C_FLAGS_RELWITHDEBINFO)
+    string(REGEX REPLACE "/W(1|2|3|4)" "" ${flag_var} "${${flag_var}}")
+  endforeach(flag_var)
+endif()
 
 # Those are used for pkg-config and friends, so that the SDL2.pc, sdl2-config,
 # etc. are created correctly.

@icculus
Copy link
Collaborator

icculus commented Nov 20, 2021

Maybe we should force -Wno-undef and -Wno-nonportable-system-include-path ?

(the first to solve all the "you used #if __GNUC__ without checking if it's defined first" and the second to stop complaints about #include <Windows.h> with a capital 'W'.)

@sezero
Copy link
Contributor Author

sezero commented Nov 20, 2021

Maybe we should force -Wno-undef and -Wno-nonportable-system-include-path ?

Maybe also -Wno-reserved-id-macro (we do a lot of those)

<Windows.h>` with a capital 'W'.)

It's the other way around.. (Off-topic: the winrt code does windows includes with capital letters, though.)

@sezero
Copy link
Contributor Author

sezero commented Nov 20, 2021

See https://github.com/sezero/SDL/tree/clang-cl
https://github.com/sezero/SDL/blob/clang-cl/CMakeLists.txt#L504

This reduces log size from about 55k lines to 33k lines - still not good.

What I wonder is why these warning options are enabled - is it
done internally by injecting them or how...

@slouken
Copy link
Collaborator

slouken commented Nov 20, 2021

Are there code changes that we should be doing to fix the warnings?

@sezero
Copy link
Contributor Author

sezero commented Nov 20, 2021

Some of the -Wundef warnings may be fixed, but as far as I can see the bulk of the rest would require very intrusive work to silence.

@sezero
Copy link
Contributor Author

sezero commented Nov 20, 2021

Adding -Wno-everything seems to silence it: https://github.com/sezero/SDL/tree/clang-cl2

diff --git a/CMakeLists.txt b/CMakeLists.txt
index e3d3b14..ed39d25 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -563,6 +563,14 @@ if(USE_GCC OR USE_CLANG)
     list(APPEND EXTRA_CFLAGS "-Wshadow")
   endif()
 
+  if(MSVC_CLANG)
+  # silence the clang-cl builds
+    check_c_compiler_flag(-Weverything HAVE_GCC_WEVERYTHING)
+    if(HAVE_GCC_WEVERYTHING)
+      list(APPEND EXTRA_CFLAGS "-Wno-everything")
+    endif()
+  endif()
+
   if(APPLE)
     list(APPEND EXTRA_LDFLAGS "-Wl,-undefined,error")
     list(APPEND EXTRA_LDFLAGS "-Wl,-compatibility_version,${DYLIB_COMPATIBILITY_VERSION}")

The order of addition in CMakeLists.txt seems important: I had to add it as the last, so when CMakeLists.txt inverts the order -Wno-everything will come before all others. Otherwise, other -Wxx (e.g. -Wshadow) seems to lose effect.

2021-11-20T20:28:09.7587638Z -- 
2021-11-20T20:28:09.7588028Z --  CFLAGS:        /DWIN32 /D_WINDOWS /W3
2021-11-20T20:28:09.7589341Z --  EXTRA_CFLAGS:  -msse3 -msse2 -msse -m3dnow -fcolor-diagnostics -Wno-everything -Wshadow -Wdeclaration-after-statement -Werror=declaration-after-statement -fno-strict-aliasing -Wall 
2021-11-20T20:28:09.7590472Z --  EXTRA_LDFLAGS: 
2021-11-20T20:28:09.7591085Z --  EXTRA_LIBS:    user32;gdi32;winmm;imm32;ole32;oleaut32;version;uuid;advapi32;setupapi;shell32;dinput8

Anyone has a better solution?

@icculus
Copy link
Collaborator

icculus commented Nov 21, 2021

Adding -Wno-everything seems to silence it:

I definitely wouldn't turn off all warnings. :)

Are there code changes that we should be doing to fix the warnings?

These two warnings are overreaches by Clang, imho; they don't actually affect clang or any other compiler we currently use, so they're just trying to enforce some hypothetical code hygiene on others, as far as I can tell.

@sezero
Copy link
Contributor Author

sezero commented Nov 21, 2021

I definitely wouldn't turn off all warnings. :)

It also adds the other -Wall & co, but as I stated, the order is important - with
the placement I did above, -Wshadow is still in effect. And other solutions are
welcome too :)

@sezero
Copy link
Contributor Author

sezero commented Nov 21, 2021

Also of real interest is where the hell these warnings are actually enabled.. Looked at lib/Driver/ToolChains/MSVC.cpp of clang, couldn't see anything relevant - cmake is doing something??

@hgs3
Copy link
Contributor

hgs3 commented Nov 23, 2021

@sezero According to the clang-cl manual, /Wall is treated as -Weverything:

  /W0                     Disable all warnings
  /W1                     Enable -Wall
  /W2                     Enable -Wall
  /W3                     Enable -Wall
  /W4                     Enable -Wall and -Wextra
  /Wall                   Enable -Weverything

A solution to this problem would be to use /W4 instead of /Wall for MSVC Clang builds. That brings down the total warning count from 13180 to 842. I tested this locally by replacing this line of the CMakeLists.txt with the following conditional:

if(MSVC_CLANG)
  list(APPEND EXTRA_CFLAGS "/W4")
else ()
  list(APPEND EXTRA_CFLAGS "-Wall")
endif ()

Alternatively, if you want -Weverything enabled, then you probably want to disable the most obtrusive warnings. I've collected all unique warnings and sorted them by occurrence in descending order below. There are only 37 unique warnings in total, but some are occurring in the thousands.

[-Wcast-align] 2526
[-Wreserved-id-macro] 2461
[-Wsign-conversion] 1758
[-Wlanguage-extension-token] 1548
[-Wimplicit-int-conversion] 1049
[-Wundef] 956
[-Wextra-semi-stmt] 830
[-Wunused-parameter] 637
[-Wimplicit-int-float-conversion] 277
[-Wnonportable-system-include-path] 170
[-Wsign-compare] 158
[-Wpedantic] 154
[-Wdouble-promotion] 122
[-Wcast-qual] 108
[-Wbad-function-cast] 80
[-Wswitch-enum] 76
[-Wunused-macros] 63
[-Wmissing-prototypes] 41
[-Wfloat-equal] 30
[-Wnull-pointer-arithmetic] 30
[-Wtautological-value-range-compare] 24
[-Wstrict-prototypes] 17
[-Wmissing-variable-declarations] 12
[-Wincompatible-pointer-types] 9
[-Wcovered-switch-default] 8
[-Wgnu-folding-constant] 8
[-Wstring-conversion] 6
[-Wimplicit-fallthrough] 5
[-Wunused-const-variable] 4
[-Wfloat-conversion] 3
[-Watomic-implicit-seq-cst] 2
[-Wunknown-pragmas] 2
[-Wdisabled-macro-expansion] 2
[-Wdocumentation-deprecated-sync] 1
[-Wunreachable-code-break] 1
[-Wunused-local-typedef] 1
[-Wignored-qualifiers] 1

@sezero
Copy link
Contributor Author

sezero commented Nov 23, 2021

@sezero According to the clang-cl manual, /Wall is treated as -Weverything:

Ew.. That explains it, thanks. Will try patching cmake soon.

@sezero sezero closed this as completed in b7f9c20 Nov 23, 2021
@sezero
Copy link
Contributor Author

sezero commented Nov 23, 2021

OK, pushed b7f9c20 -- closing as fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants