Improve ClangCL support by disabling, fixing warnings #5876

alexreinking · 2021-04-02T19:59:42Z

Supersedes @emmenlau's #5858, which I guess didn't provide maintainers with write permissions to the PR branch.

Improves ClangCL support by disabling (or fixing) warnings its -Wall mode emits.

Removed a bunch of extra semicolons
Stopped passing linker script to ClangCL (which throws an unknown flag warning)
Be smarter about disabling introspection
Various code fixes (see my comments in review)
A bit of CMake clean-up near my edits.

src/CMakeLists.txt

steven-johnson · 2021-04-02T20:45:14Z

Skipping buildbots because the changes have already been tested in the other PR and we don't test ClangCL on CI, anyway.

This will certainly break if we don't test it, but I guess that's ok for now ("best effort"). If we get better Windows testing capability it would be worth adding.

alexreinking · 2021-04-05T21:09:02Z

I'm wondering if a different approach here is appropriate... the only ClangCL-specific changes are:

Disabling -Wall, which is smelly to me
Disabling WITH_INTROSPECTION which could be solved outside of the buildsystem by sniffing _MSC_VER.

I wonder if a better approach is to fix/selectively disable warnings and disable introspection on MSVC-like compilers using the usual macros.

@steven-johnson -- any thoughts?

steven-johnson · 2021-04-05T21:17:17Z

Disabling -Wall should never be necessary and is definitely a smell. (I overlooked this when I approved it.) Does ClangCL nto support it at all?

alexreinking · 2021-04-05T23:28:07Z

Disabling -Wall should never be necessary and is definitely a smell. (I overlooked this when I approved it.) Does ClangCL nto support it at all?

It does support -Wall, but it enables very, very many warnings. This is the new list of warnings to disable (some of these might be real or be easy to fix, I haven't dug deep):

-Wno-c++98-compat-pedantic
-Wno-c++98-compat
-Wno-cast-align
-Wno-comma
-Wno-deprecated-declarations
-Wno-documentation-unknown-command
-Wno-documentation
-Wno-double-promotion
-Wno-exit-time-destructors
-Wno-extra-semi-stmt
-Wno-extra-semi
-Wno-float-conversion
-Wno-float-equal
-Wno-global-constructors
-Wno-implicit-float-conversion
-Wno-implicit-int-conversion
-Wno-implicit-int-float-conversion
-Wno-missing-field-initializers
-Wno-missing-prototypes
-Wno-nonportable-system-include-path
-Wno-old-style-cast
-Wno-reserved-id-macro
-Wno-return-std-move-in-c++11
-Wno-shadow-field-in-constructor
-Wno-shadow-field
-Wno-shadow
-Wno-shorten-64-to-32
-Wno-sign-conversion
-Wno-switch-enum
-Wno-undef
-Wno-undefined-func-template
-Wno-unused-function
-Wno-unused-member-function
-Wno-unused-parameter
-Wno-unused-template

I wonder if clang-cl's -Wall is more of a -Weverything or even -Weverything -pedantic -Wextra -Weven_more_I_didnt_think_of.

Another wrinkle: ClangCL doesn't have exceptions enabled by default and it doesn't honor -fexceptions as a way of enabling them.

alexreinking · 2021-04-06T00:08:46Z

src/CodeGen_Hexagon.cpp

-
-            return load;


Ouch. I wonder if this was a bad merge. Probably worth turning on this warning explicitly.

Yow. I'm genuinely surprised this is merely a warning.

alexreinking · 2021-04-06T00:10:41Z

src/LowerWarpShuffles.cpp

@@ -571,7 +571,7 @@ class LowerWarpShuffles : public IRMutator {

        Expr wild = Variable::make(Int(32), "*");
        vector<Expr> result;
-        int bits;
+        int bits = 0;


The reason this isn't needed is extremely subtle: the function is_const_power_of_two_integer returns true iff it initializes bits. The first read of bits is guarded by short-circuit && below. ClangCL isn't clever enough to see this, and I don't blame it.

There's no downside to this (this is not performance-critical) and disabling uninitialized use warnings seems significantly worse.

alexreinking · 2021-04-06T00:11:12Z

src/Util.cpp

    return "";
+#endif
 }


This emits an unreachable code warning.

alexreinking · 2021-04-06T00:11:47Z

src/Util.cpp

@@ -529,7 +528,7 @@ struct TickStackEntry {
    int line;
 };

-vector<TickStackEntry> tick_stack;


Throws a warning about missing extern and that it should be static if module-local, which it is. No need to export this.

alexreinking · 2021-04-06T00:12:15Z

src/WasmExecutor.cpp

-#endif
-
+#else
    WasmModule wasm_module;
    wasm_module.contents = new WasmModuleContents(module, arguments, fn_name, jit_externs, extern_deps);
    return wasm_module;
+#endif
 }


Similar to above: unreachable code.

alexreinking · 2021-04-06T00:12:29Z

src/VectorizeLoops.cpp

@@ -1672,4 +1672,4 @@ Stmt vectorize_loops(const Stmt &stmt, const map<string, Function> &env, const T
 }

 }  // namespace Internal
-}  // namespace Halide


This was the only file without a trailing newline.

steven-johnson · 2021-04-06T00:31:57Z

I wonder if clang-cl's -Wall is more of a -Weverything or even -Weverything -pedantic -Wextra -Weven_more_I_didnt_think_of.

Ouch -- yeah, maybe it's intended to be equivalent of MSVC's warning-level-4, which is pretty insanely high IIRC.

Another wrinkle: ClangCL doesn't have exceptions enabled by default and it doesn't honor -fexceptions as a way of enabling them.

uh, what? how on earth would a clang-compatible(-ish) compiler omit this?

steven-johnson · 2021-04-06T16:31:09Z

src/Buffer.h

@@ -568,12 +568,12 @@ class Buffer {
    Expr operator()(const Expr &first, Args... rest) const {
        std::vector<Expr> args = {first, rest...};
        return (*this)(args);
-    };
+    }


Ooh, I'd actually love to enable whatever warnings complains about this, it's a minor pet peeve of mine

steven-johnson · 2021-04-06T20:15:06Z

Failures are unrelated (error in nightly LLVM13), ok to land

alexreinking · 2021-04-07T00:53:53Z

Landing. Still need a way to reliably enable exceptions on ClangCL, but that can be done in another PR. This one's scope has crept far enough.

alexreinking requested a review from steven-johnson April 2, 2021 19:59

alexreinking added the build Issues related to building Halide and with CI label Apr 2, 2021

alexreinking mentioned this pull request Apr 2, 2021

Added support for ClangCL #5858

Closed

alexreinking self-assigned this Apr 2, 2021

alexreinking added this to the v12.0.0 milestone Apr 2, 2021

alexreinking added the skip_buildbots Synonym for buildbot_test_nothing label Apr 2, 2021

steven-johnson approved these changes Apr 2, 2021

View reviewed changes

src/CMakeLists.txt Outdated Show resolved Hide resolved

alexreinking marked this pull request as draft April 5, 2021 21:09

emmenlau and others added 3 commits April 5, 2021 14:19

Added support for ClangCL on MSVC

5aad444

Remove stack-size workarounds

a7d4e8c

Add ClangCL/lld-link.exe's warning to TargetExportScript.cmake

014afc3

alexreinking force-pushed the bda_clang_build branch from d2ef021 to 014afc3 Compare April 5, 2021 21:21

Clean up TargetExportScript.cmake

cf946dd

alexreinking added 4 commits April 5, 2021 16:43

Disable tons of warnings from ClangCL's -Wall.

41e8bf8

Fix problems discovered by legitimate warnings.

f7ddd48

Remove one of the long variables.

4265af5

Disable introspection if MSVC or execinfo.h not available.

b19a7b3

alexreinking commented Apr 6, 2021

View reviewed changes

alexreinking added 2 commits April 5, 2021 17:34

Remove empty statements and unnecessary semicolons

3a60c98

Don't pass /MP except in MSBuild (Visual Studio)

b502cae

alexreinking changed the title ~~Fix ClangCL~~ Improve ClangCL support by disabling, fixing warnings Apr 6, 2021

alexreinking removed the skip_buildbots Synonym for buildbot_test_nothing label Apr 6, 2021

alexreinking marked this pull request as ready for review April 6, 2021 01:20

tickle buildbots

808c1e7

steven-johnson reviewed Apr 6, 2021

View reviewed changes

steven-johnson approved these changes Apr 6, 2021

View reviewed changes

alexreinking merged commit ea76214 into master Apr 7, 2021

alexreinking deleted the bda_clang_build branch April 7, 2021 00:53

alexreinking mentioned this pull request Apr 7, 2021

[cmake] Fix and reorganize warnings for building Halide #5885

Merged

emmenlau mentioned this pull request Apr 28, 2021

Support for ClangCl (clang-cl) on MSVC #5852

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Improve ClangCL support by disabling, fixing warnings #5876

Improve ClangCL support by disabling, fixing warnings #5876

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Improve ClangCL support by disabling, fixing warnings #5876

Improve ClangCL support by disabling, fixing warnings #5876

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!