cmake: enable more strict compiler warnings #508

axic · 2020-08-24T12:22:36Z

No description provided.

chfast · 2020-08-27T08:03:53Z

Add -Wmissing-declarations (may be GCC only)

axic · 2020-08-27T09:43:31Z

lib/fizzy/execute.cpp

@@ -1963,7 +1963,7 @@ ExecutionResult execute(
        }
        case Instr::f64_promote_f32:
        {
-            stack.top() = double{stack.top().f32};
+            stack.top() = static_cast<double>(stack.top().f32);


@chfast do you want this to go into master?

Do we have consensus here?

cmake/CableCompilerSettings.cmake

codecov · 2020-09-02T13:51:33Z

Codecov Report

Merging #508 into master will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #508   +/-   ##
=======================================
  Coverage   98.73%   98.73%           
=======================================
  Files          56       56           
  Lines        8704     8704           
=======================================
  Hits         8594     8594           
  Misses        110      110

chfast · 2020-09-02T14:15:04Z

cmake/CableCompilerSettings.cmake

@@ -87,6 +87,12 @@ macro(cable_configure_compiler)
            # Enable basing warnings set and treat them as errors.
            add_compile_options(-Werror -Wall -Wextra -Wshadow)

+            add_compile_options(-Wdouble-promotion -Wcast-qual -Wcast-align -Wdangling-else -Wlogical-not-parentheses -Wmissing-declarations)


I think this must be checked against documentation.
E.g. -Wlogical-not-parentheses

is enabled in Clang by default: https://clang.llvm.org/docs/DiagnosticsReference.html#wlogical-not-parentheses

is enabled in GCC with -Wall: https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wall

I added these based on documentation: first added a lot of the things I liked, then removed those which are included in -Wall (and other settings we have). Seems like this one slipped through the cracks.

I picked this one randomly.

The -Wdouble-promotion should be remove as I don't want the change #508 (comment).

I will check the rest.

It seems -Wcast-qual and -Wcast-align is only for C-style casts of pointers. Unlikely we will see such thing in our code, but the warning option can stay.

The -Wdangling-else is enabled by default/-Wall.

The -Wmissing-declarations is nice.

The -Wdouble-promotion should be remove as I don't want the change #508 (comment).

Do you have a reasoning? I'd be also interested to hear @gumb0's view.

The -Wdouble-promotion should be remove as I don't want the change #508 (comment).

Do you have a reasoning? I'd be also interested to hear @gumb0's view.

The double{x} is superior to static_cast<double>(x) and the -Wdouble-promotion forbids using the first syntax. It is rather a compiler bug due to fact that -Wdouble-promotion predates the double{x} syntax.

There is even Clang bug for this: https://bugs.llvm.org/show_bug.cgi?id=34062.

@chfast @gumb0 any other warnings we should/could enable?

Yes, try

-Weverything -Wno-c++98-compat -Wno-c++98-compat-pedantic -Wno-padded -Wno-switch-enum -Wno-float-equal -Wno-double-promotion

I found many easy fixes with these already.

-Wlogical-not-parentheses looks to be included in -Wall?

Move the changes to the main CMakeLists.txt, do not modify the Cable file.

This is cleaned up now.

test/utils/wasm_engine.cpp

test/utils/wasm3_engine.cpp

gumb0 · 2020-09-07T15:19:14Z

CMakeLists.txt

+
+option(WEVERYTHING "Enable almost all compiler warnings" OFF)
+if(WEVERYTHING)
+    add_compile_options(-Weverything)


Why do I not see these on CI?
Build Tidy (Debug) step outputs

---- Generator: Ninja Compiler: Clang Build Type: Debug LTO: OFF Flags: -Dd_m3HasTracer -Wall -Wextra -Wparentheses -Wundef -Wpointer-arith -Wstrict-aliasing=2 -Werror=implicit-function-declaration -Wno-unused-function -Wno-unused-variable -Wno-unused-parameter -Wno-missing-field-initializers -Werror=shadow Debug flags: -g -DDEBUG=1 -ggdb -O0 Release flags: -O3 -Wfatal-errors -fomit-frame-pointer -fno-stack-check -fno-stack-protector ----

are these not all of the flags?

These are wasm3 flags. We don't add ours to theirs.

I don't see any warning emitted in Build Tidy, do we not generate any currently? If we did, would CI fail?

All warnings are fixed.
But here is older build with the warning for #508 (comment): https://app.circleci.com/pipelines/github/wasmx/fizzy/4478/workflows/b12515da-c249-4903-aa9f-8d3d1d5e7cbb/jobs/31809

I mean, if it fails CI, what's the point of making them optional?

People can still build it locally without a struggle.

Do you mean they can build it locally without -Weverything for compilers that we don't test?

Yes. Clang discourages usage of -Weverything regularly for projects so I want to follow this rule for now. While Fizzy is actively maintained it is probably not big deal, but future versions of Clang may enable something we don't want.

gumb0 · 2020-09-07T15:37:37Z

test/bench_internal/parser_benchmarks.hpp

@@ -0,0 +1,15 @@
+// Fizzy: A fast WebAssembly interpreter


Maybe this file should be experimental.hpp, because two of its three functions are defined in experimental.cpp?

Also not sure if the third one - leb128u_decode_u64_noinline - needs to be in its own cpp.

I would go with something less specific than parser_benchmarks.cpp: experimental.cpp is fine.

This is not explicitly needed now as I discovered [[noinline]] attribute...

This is resolved now to use experimental.hpp for alternative LEB128 implementation. The other functions were moved to parser_benchmarks.cpp with [[gnu::noinline]] attribute.

gumb0 · 2020-09-07T15:38:07Z

test/unittests/execute_floating_point_test.cpp

@@ -137,8 +137,8 @@ class TestValues
            std::array<T, ps.size() * 2 + 1> a;

            auto it = std::begin(a);
-            it = std::transform(std::reverse_iterator{std::end(ps)},
-                std::reverse_iterator{std::begin(ps)}, it, std::negate<T>{});
+            it = std::transform(std::make_reverse_iterator(std::end(ps)),


Does this resolve some warning?

There is warning that reverse_iterator is may not to designed for CTAD (as it does not have any explicit CTAD hints). I think it is ok to change it here as this is the only case for this warning. And I'm also considering rewriting these test arrays to constexpr version where STL algorithms are going to be less useful.

Co-authored-by: Paweł Bylica <chfast@gmail.com>

axic force-pushed the cmake-warnings branch 2 times, most recently from ec2f408 to e57f3d1 Compare August 24, 2020 12:31

axic commented Aug 27, 2020

View reviewed changes

cmake/CableCompilerSettings.cmake Outdated Show resolved Hide resolved

axic force-pushed the cmake-warnings branch 7 times, most recently from 788f2eb to bece2a1 Compare September 2, 2020 13:17

chfast requested changes Sep 2, 2020

View reviewed changes

gumb0 reviewed Sep 3, 2020

View reviewed changes

test/utils/wasm_engine.cpp Outdated Show resolved Hide resolved

chfast force-pushed the cmake-warnings branch 2 times, most recently from 0fdc110 to 9b65842 Compare September 3, 2020 21:46

axic commented Sep 3, 2020

View reviewed changes

test/utils/wasm3_engine.cpp Outdated Show resolved Hide resolved

chfast force-pushed the cmake-warnings branch 3 times, most recently from 445273e to 1d5168c Compare September 3, 2020 22:02

This was referenced Sep 4, 2020

ci: Combine linter/style/tidy/portability checks #520

Merged

Emit vtables once #521

Merged

chfast force-pushed the cmake-warnings branch 4 times, most recently from cddddec to 5228ae2 Compare September 4, 2020 20:38

chfast changed the base branch from master to exception_vtables September 4, 2020 21:11

chfast force-pushed the cmake-warnings branch from fb2f85b to 5a5689e Compare September 4, 2020 21:13

chfast marked this pull request as ready for review September 4, 2020 21:13

chfast approved these changes Sep 4, 2020

View reviewed changes

Base automatically changed from exception_vtables to master September 4, 2020 21:20

chfast force-pushed the cmake-warnings branch from 5a5689e to 94b61f4 Compare September 7, 2020 08:53

gumb0 reviewed Sep 7, 2020

View reviewed changes

gumb0 approved these changes Sep 8, 2020

View reviewed changes

axic force-pushed the cmake-warnings branch from 22744f5 to e5d8491 Compare September 9, 2020 13:34

chfast force-pushed the cmake-warnings branch 2 times, most recently from a72268a to 66c480f Compare September 9, 2020 18:15

axic and others added 6 commits September 9, 2020 19:57

wasm3: use static_cast instead of C-style casts

1a230cf

Co-authored-by: Paweł Bylica <chfast@gmail.com>

Fix compiler warnings

0dcf084

test: Create proper declarations for benchmarked functions

3106104

Co-authored-by: Paweł Bylica <chfast@gmail.com>

test: Mark Engine implementations as final

a6225f6

cmake: enable more strict compiler warnings

3ed9a8d

ci: Add -Weverything job

d3e7206

axic force-pushed the cmake-warnings branch from 66c480f to d3e7206 Compare September 9, 2020 18:57

axic merged commit 2340104 into master Sep 9, 2020

axic deleted the cmake-warnings branch September 9, 2020 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmake: enable more strict compiler warnings #508

cmake: enable more strict compiler warnings #508

axic commented Aug 24, 2020

chfast commented Aug 27, 2020

axic Aug 27, 2020

chfast Aug 27, 2020

chfast Sep 4, 2020

codecov bot commented Sep 2, 2020 •

edited

Loading

chfast Sep 2, 2020

axic Sep 2, 2020

chfast Sep 2, 2020

chfast Sep 2, 2020 •

edited

Loading

axic Sep 2, 2020

chfast Sep 2, 2020

chfast Sep 2, 2020

gumb0 Sep 3, 2020

chfast Sep 3, 2020

chfast Sep 4, 2020

gumb0 Sep 7, 2020

chfast Sep 7, 2020

gumb0 Sep 7, 2020

chfast Sep 7, 2020

gumb0 Sep 7, 2020

chfast Sep 7, 2020

gumb0 Sep 8, 2020

chfast Sep 8, 2020

gumb0 Sep 7, 2020

chfast Sep 7, 2020

chfast Sep 9, 2020

gumb0 Sep 7, 2020

chfast Sep 7, 2020

cmake: enable more strict compiler warnings #508

cmake: enable more strict compiler warnings #508

Conversation

axic commented Aug 24, 2020

chfast commented Aug 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 2, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chfast Sep 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 2, 2020 •

edited

Loading

chfast Sep 2, 2020 •

edited

Loading