Page MenuHomePhabricator

[build] Make all warnings into errors with a whitelist
AbandonedPublic

Authored by majcosta on May 19 2020, 00:37.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Summary

Currently our ENABLE_WERROR build promotes a few warnings to errors,
this change enables us to use the more restrictive Werror options while
retaining granularity to preserve warnings as needed

Test Plan

remove whitelisted warnings from CMakeLists.txt file

ninja

see build fail due to warnings promoted to errors

Diff Detail

Event Timeline

majcosta created this revision.May 19 2020, 00:37
Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 19 2020, 00:37
majcosta requested review of this revision.May 19 2020, 00:37

Snippet of first build failure:

[00:38:21] :	 [Step 1/2] -- Check size of __int128
[00:38:21] :	 [Step 1/2] -- Check size of __int128 - done
[00:38:21] :	 [Step 1/2] -- Found OpenSSL: /usr/lib/x86_64-linux-gnu/libcrypto.so (found version "1.1.1d") found components:  Crypto 
[00:38:22] :	 [Step 1/2] -- Boost version: 1.67.0
[00:38:22] :	 [Step 1/2] -- Found the following Boost libraries:
[00:38:22] :	 [Step 1/2] --   chrono
[00:38:22] :	 [Step 1/2] --   filesystem
[00:38:22] :	 [Step 1/2] --   thread
[00:38:22] :	 [Step 1/2] --   system
[00:38:22] :	 [Step 1/2] --   date_time
[00:38:22] :	 [Step 1/2] --   atomic
[00:38:22] :	 [Step 1/2] -- Found PkgConfig: /usr/bin/pkg-config (found version "0.29") 
[00:38:22] :	 [Step 1/2] -- Found Event component event: /usr/lib/x86_64-linux-gnu/libevent.so
[00:38:22] :	 [Step 1/2] -- Found Event: /usr/include (found suitable version "2.1.8-stable", minimum required is "2.0.22") found components:  event 
[00:38:22] :	 [Step 1/2] -- Found Event component pthreads: /usr/lib/x86_64-linux-gnu/libevent_pthreads.so
[00:38:22] :	 [Step 1/2] -- Found Event: /usr/include (found suitable version "2.1.8-stable", minimum required is "2.0.22") found components:  pthreads 
[00:38:22] :	 [Step 1/2] -- Boost version: 1.67.0
[00:38:22] :	 [Step 1/2] -- Found the following Boost libraries:
[00:38:22] :	 [Step 1/2] --   unit_test_framework
[00:38:22] :	 [Step 1/2] -- Performing Test BOOST_TEST_DYN_LINK
[00:38:22] :	 [Step 1/2] -- Performing Test BOOST_TEST_DYN_LINK - Success
[00:38:22] :	 [Step 1/2] -- Boost version: 1.67.0
[00:38:22] :	 [Step 1/2] -- Found the following Boost libraries:
[00:38:22] :	 [Step 1/2] --   unit_test_framework
[00:38:22] :	 [Step 1/2] -- Configuring done
[00:38:23] :	 [Step 1/2] -- Generating done
[00:38:23] :	 [Step 1/2] -- Build files have been written to: /work/build/native
[00:38:23] :	 [Step 1/2] [6/455] Building CXX object src/crypto/CMakeFiles/crypto_shani.dir/sha256_shani.cpp.o
[00:38:23] :	 [Step 1/2] FAILED: src/crypto/CMakeFiles/crypto_shani.dir/sha256_shani.cpp.o 
[00:38:23] :	 [Step 1/2] /usr/bin/ccache /usr/bin/clang++  -DENABLE_SHANI -I../src/crypto/.. -g -O2 -fPIC -fvisibility=hidden   -fstack-protector-all -Wstack-protector -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wformat -Wvla -Wformat-security -Wcast-align -Wunused-parameter -Wmissing-braces -Wthread-safety-analysis -Wshadow -Wrange-loop-analysis -Wredundant-decls -Wredundant-move -Wno-unused-parameter -Wno-implicit-fallthrough -Werror -Wno-error=deprecated-declarations -msse4 -msha -std=gnu++14 -MD -MT src/crypto/CMakeFiles/crypto_shani.dir/sha256_shani.cpp.o -MF src/crypto/CMakeFiles/crypto_shani.dir/sha256_shani.cpp.o.d -o src/crypto/CMakeFiles/crypto_shani.dir/sha256_shani.cpp.o -c ../src/crypto/sha256_shani.cpp
[00:38:23] :	 [Step 1/2] ../src/crypto/sha256_shani.cpp:74:45: error: cast from 'const uint8_t *' (aka 'const unsigned char *') to 'const __m128i *' increases required alignment from 1 to 16 [-Werror,-Wcast-align]
[00:38:23] :	 [Step 1/2]     return _mm_shuffle_epi8(_mm_loadu_si128((const __m128i *)in), MASK);
[00:38:23] :	 [Step 1/2]                                             ^~~~~~~~~~~~~~~~~~~
[00:38:23] :	 [Step 1/2] ../src/crypto/sha256_shani.cpp:78:22: error: cast from 'uint8_t *' (aka 'unsigned char *') to '__m128i *' increases required alignment from 1 to 16 [-Werror,-Wcast-align]
[00:38:23] :	 [Step 1/2]     _mm_storeu_si128((__m128i *)out, _mm_shuffle_epi8(s, MASK));
[00:38:23] :	 [Step 1/2]                      ^~~~~~~~~~~~~~
[00:38:23] :	 [Step 1/2] ../src/crypto/sha256_shani.cpp:87:26: error: cast from 'uint32_t *' (aka 'unsigned int *') to 'const __m128i *' increases required alignment from 4 to 16 [-Werror,-Wcast-align]
[00:38:23] :	 [Step 1/2]     s0 = _mm_loadu_si128((const __m128i *)s);
[00:38:23] :	 [Step 1/2]                          ^~~~~~~~~~~~~~~~~~
[00:38:23] :	 [Step 1/2] ../src/crypto/sha256_shani.cpp:88:26: error: cast from 'uint32_t *' (aka 'unsigned int *') to 'const __m128i *' increases required alignment from 4 to 16 [-Werror,-Wcast-align]
[00:38:23] :	 [Step 1/2]     s1 = _mm_loadu_si128((const __m128i *)(s + 4));
[00:38:23] :	 [Step 1/2]                          ^~~~~~~~~~~~~~~~~~~~~~~~
[00:38:23] :	 [Step 1/2] ../src/crypto/sha256_shani.cpp:141:22: error: cast from 'uint32_t *' (aka 'unsigned int *') to '__m128i *' increases required alignment from 4 to 16 [-Werror,-Wcast-align]
[00:38:23] :	 [Step 1/2]     _mm_storeu_si128((__m128i *)s, s0);
[00:38:23] :	 [Step 1/2]                      ^~~~~~~~~~~~
[00:38:23]W:	 [Step 1/2] ++ print_sanitizers_log
[00:38:23] :	 [Step 1/2] ../src/crypto/sha256_shani.cpp:142:22: error: cast from 'uint32_t *' (aka 'unsigned int *') to '__m128i *' increases required alignment from 4 to 16 [-Werror,-Wcast-align]
[00:38:23]W:	 [Step 1/2] ++ for log in "${SAN_LOG_DIR}"/*.log.*
[00:38:23] :	 [Step 1/2]     _mm_storeu_si128((__m128i *)(s + 4), s1);
[00:38:23] :	 [Step 1/2]                      ^~~~~~~~~~~~~~~~~~
[00:38:23]W:	 [Step 1/2] ++ echo '*** Output of /tmp/sanitizer_logs/*.log.* ***'
[00:38:23] :	 [Step 1/2] 6 errors generated.
[00:38:23] :	 [Step 1/2] [18/455] Building CXX object src/crypto/CMakeFiles/crypto_sse4.1.dir/sha256_sse41.cpp.o
[00:38:23]W:	 [Step 1/2] ++ cat '/tmp/sanitizer_logs/*.log.*'
[00:38:23] :	 [Step 1/2] ninja: build stopped: subcommand failed.
[00:38:23]W:	 [Step 1/2] cat: '/tmp/sanitizer_logs/*.log.*': No such file or directory
[00:38:23] :	 [Step 1/2] *** Output of /tmp/sanitizer_logs/*.log.* ***
[00:38:23]W:	 [Step 1/2] cp: cannot stat '/work/ibd/debug.log': No such file or directory
[00:38:27]W:	 [Step 1/2] Process exited with code 1
[00:38:27]E:	 [Step 1/2] Process exited with code 1 (Step: Command Line)
majcosta planned changes to this revision.May 19 2020, 01:17

well that is embarrassing...

majcosta updated this revision to Diff 20292.May 19 2020, 17:06

fixed clang build, added conditional to prevent __FORTIFY_SOURCE warnings in '-O0' builds

deadalnix requested changes to this revision.May 19 2020, 17:15
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
src/CMakeLists.txt
138 ↗(On Diff #20292)

You will need to explain what the problem is an how that make sense, because from where I'm sitting, it doesn't.

This revision now requires changes to proceed.May 19 2020, 17:15
majcosta added a subscriber: Fabien.May 19 2020, 18:32
majcosta added inline comments.
src/CMakeLists.txt
138 ↗(On Diff #20292)

enabling these flags without optimization (as is the case of our Debug configuration that uses -O0 rather than -Og raises a warning, that gets promoted to error under WERROR_ENABLE.

so the intent of that change is to disable _FORTIFY_SOURCE only for Debug builds. however, @Fabien has brought to my attention that the method above isn't good since it doesn't work with all generators, so I'm working on something that's correct

majcosta updated this revision to Diff 20307.May 19 2020, 20:01

copying previous inline comment that I suspect is about to be deleted:

enabling these flags without optimization (as is the case of our Debug configuration that uses -O0 rather than -Og raises a warning, that gets promoted to error under WERROR_ENABLE.

so the intent of that change is to disable _FORTIFY_SOURCE only for Debug builds. however, @Fabien has brought to my attention that the method above isn't good since it doesn't work with all generators, so I'm working on something that's correct

this method should be robust to detecting build configuration and only exclude _FORTIFY_SOURCE=2 from the Debug build h/t @Fabien

majcosta updated this revision to Diff 20308.May 19 2020, 20:03

removed a spurious newline I added to CMakeLists.txt

majcosta planned changes to this revision.May 19 2020, 21:41
majcosta abandoned this revision.Jun 2 2020, 14:44