build: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set #25972
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Concept ACK. |
|
Looks like the CI failures (due to |
|
GUIX hashes x86: arm64: |
|
cc @theuni. See: https://github.com/bitcoin/bitcoin/pull/25972/checks?check_run_id=8137548994, for an example of unsuppressed sdt.h warnings. |
|
As @fanquake mentioned, some of these are only warnings in older compilers. I'm going through them to double-check that they are indeed non-issues, as opposed to missed warnings in newer compilers. (I'm ignoring the sdt.h warnings for now and focusing on what's in our code. I will look at that header next.) I've pushed commits that implement the following suggestions here: https://github.com/theuni/bitcoin/commits/use_cxxflags_with_depends For this one:
This was a warning in clang-9 but not clang-10. The warning was removed here: https://reviews.llvm.org/D72212 tl;dr: copies are allowed for POD types less than 64bytes. We can confirm this by adding a dummy That seems reasonable enough, but I think we may as well just fix it up to take a const ref to remove any ambiguity. for this one:
I can reproduce this with gcc9 but not gcc10. My best guess is that it's related to: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80635 Though I'm having trouble recreating a minimal test-case that fails with gcc9 and succeeds with gcc10. Changing these from For this one:
This one looks legitimate. Suggest fixing by initializing to For this one:
(This applies to x86_64 as well) Again, gcc10 is smart enough to figure this out, but gcc9 complains. Initializing to zero is harmless, so again I suggest just initializing to 0 to quiet the old compilers. |
|
I also pushed a patch for systemtap in depends that gets rid of those compiler warnings. From what I can tell, the variadic is completely unused there, but I'm not at all confident in that change yet. |
|
Concept ACK, btw :) |
0aa641f to
d5efac3
Compare
I had opened #26006, to see if anyone would jump in with the change, but at this point will just PR your commit separate to this. For the remaining of the changes, I've rebased on top of your branch I think the changes are fairly uncontreversial (aside from sdt.h), and are better than adding
Maybe @0xB10C can weigh in? |
|
With the /usr/bin/ccache x86_64-w64-mingw32-g++-posix -std=c++17 -DHAVE_CONFIG_H -I. -I../src/config -fmacro-prefix-map=/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-w64-mingw32=. -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -DHAVE_BUILD_INFO -D_MT -DWIN32 -D_WINDOWS -D_WIN32_WINNT=0x0601 -D_WIN32_IE=0x0501 -DWIN32_LEAN_AND_MEAN -DNOMINMAX -D_FILE_OFFSET_BITS=64 -DPROVIDE_FUZZ_MAIN_FUNCTION -I. -I./minisketch/include -I./secp256k1/include -I./univalue/include -I./leveldb/include -I/tmp/cirrus-ci-build/depends/x86_64-w64-mingw32/include/ -fdebug-prefix-map=/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-w64-mingw32=. -fstack-reuse=none -Wstack-protector -fstack-protector-all -fcf-protection=full -Wall -Wextra -Wformat -Wformat-security -Wvla -Wredundant-decls -Wdate-time -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wno-unused-parameter -Werror -Wno-error=return-type -fno-extended-identifiers -fvisibility=hidden -fPIE -pipe -std=c++17 -O2 -c -o script/libbitcoin_consensus_a-interpreter.o `test -f 'script/interpreter.cpp' || echo './'`script/interpreter.cpp
script/interpreter.cpp: In function ‘bool SignatureHashSchnorr(uint256&, ScriptExecutionData&, const T&, uint32_t, uint8_t, SigVersion, const PrecomputedTransactionData&, MissingDataBehavior) [with T = CTransaction]’:
script/interpreter.cpp:1530:42: error: ‘ext_flag’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
1530 | const uint8_t spend_type = (ext_flag << 1) + (have_annex ? 1 : 0); // The low bit indicates whether an annex is present.
| ~~~~~~~~~~^~~~~
script/interpreter.cpp: In function ‘bool SignatureHashSchnorr(uint256&, ScriptExecutionData&, const T&, uint32_t, uint8_t, SigVersion, const PrecomputedTransactionData&, MissingDataBehavior) [with T = CMutableTransaction]’:
script/interpreter.cpp:1530:42: error: ‘ext_flag’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
1530 | const uint8_t spend_type = (ext_flag << 1) + (have_annex ? 1 : 0); // The low bit indicates whether an annex is present.
| ~~~~~~~~~~^~~~~This looks like another false-positive, as bitcoin/src/script/interpreter.cpp Line 1480 in a8c3590 |
I too think that we don't use the variadic and it's Ok to drop it in the patch: We use, for example, the
|
|
I've opened an upstream issue: bitcoin-core/libmultiprocess#77, for the warnings in the multiprocess CI job: /usr/bin/ccache clang++ -m32 -std=c++17 -DHAVE_CONFIG_H -I. -I../src/config -DDEBUG -DDEBUG_LOCKORDER -DDEBUG_LOCKCONTENTION -DRPC_DOC_CHECK -DABORT_ON_FAILED_ASSUME -fmacro-prefix-map=/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu=. -DHAVE_BUILD_INFO -D_FILE_OFFSET_BITS=64 -DPROVIDE_FUZZ_MAIN_FUNCTION -I. -I./minisketch/include -I./secp256k1/include -I./univalue/include -I./leveldb/include -D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC -D_LIBCPP_DEBUG=1 -I/tmp/cirrus-ci-build/depends/i686-pc-linux-gnu/include/ -O0 -g3 -ftrapv -fdebug-prefix-map=/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu=. -Wstack-protector -fstack-protector-all -fcf-protection=full -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wvla -Wshadow-field -Wthread-safety -Wrange-loop-analysis -Wredundant-decls -Wunused-member-function -Wdate-time -Wconditional-uninitialized -Woverloaded-virtual -Wunreachable-code-loop-increment -Wimplicit-fallthrough -Wdocumentation -Wno-unused-parameter -Wno-self-assign -Werror -fPIE -std=c++17 -pthread -I/tmp/cirrus-ci-build/depends/i686-pc-linux-gnu/include -pipe -std=c++17 -O1 -c -o ipc/capnp/libbitcoin_ipc_a-protocol.o `test -f 'ipc/capnp/protocol.cpp' || echo './'`ipc/capnp/protocol.cpp
In file included from ipc/capnp/protocol.cpp:14:
/tmp/cirrus-ci-build/depends/i686-pc-linux-gnu/include/mp/proxy-io.h:33:37: error: parameter 'connection' shadows member inherited from type 'InvokeContext' [-Werror,-Wshadow-field]
ClientInvokeContext(Connection& connection, ThreadContext& thread_context)
^
/tmp/cirrus-ci-build/depends/i686-pc-linux-gnu/include/mp/proxy-io.h:27:17: note: declared here
Connection& connection;
^
/tmp/cirrus-ci-build/depends/i686-pc-linux-gnu/include/mp/proxy-io.h:160:16: error: moving a local object in a return statement prevents copy elision [-Werror,-Wpessimizing-move]
return std::move(logger);
^
/tmp/cirrus-ci-build/depends/i686-pc-linux-gnu/include/mp/proxy-io.h:160:16: note: remove std::move call here
return std::move(logger);
^~~~~~~~~~ ~
2 errors generated.as they are from the multiprocess code: |
Bumping #25065 (comment) again.
Long-term, I think we could either drop the
Short-term, the simple/obvious solution is just to initialize to zero, which wouldn't affect the meaning of the code. |
There is also Edit: Ok, maybe it does. Though annotating |
|
Here's a slightly more invasive change that eliminates the possibility of undefined behavior by filtering out script versions that can't be consumed by these functions: https://github.com/theuni/bitcoin/commits/explicit-v1-scriptver The key idea is here: theuni@5f26e4a#diff-da9625bc8e6928c0ec3239921e739c32b9d0abdfbca9773c550a3cdd818230b8R196 The second commit is just a small cleanup. I think it's a nice improvement. Will PR if there's any interest. |
Note that not only do we do not use the variadic, it's not even hooked up to anything. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
Strictly speaking, this will keep the UB. Recall that an |
Heh, I suppose I invited the pedantry there. Correct, it wouldn't eliminate UB potential entirely here, but it would put it on par with every other switch/case over an fully-handled enum class without a default in our codebase. I don't think you're suggesting adding As I see it it'd be strictly an improvement (though admittedly not bulletproof) as the intent is better communicated to callers, and would cleanly eliminate this warning. Would you NACK it as a PR? |
fanquake
added a commit
that referenced
this pull request
Sep 15, 2022
02c9e56 fs: fully initialize _OVERLAPPED for win32 (Cory Fields) Pull request description: ```bash fs.cpp: In member function ‘bool fsbridge::FileLock::TryLock()’: fs.cpp:129:32: error: missing initializer for member ‘_OVERLAPPED::InternalHigh’ [-Werror=missing-field-initializers] 129 | _OVERLAPPED overlapped = {0}; | ^ fs.cpp:129:32: error: missing initializer for member ‘_OVERLAPPED::<anonymous>’ [-Werror=missing-field-initializers] fs.cpp:129:32: error: missing initializer for member ‘_OVERLAPPED::hEvent’ [-Werror=missing-field-initializers] ``` Came up in #25972. That PR is now rebased on this change. Closes: #26006 ACKs for top commit: sipsorcery: tACK 02c9e56. hebasto: ACK 02c9e56, tested on Linux x86_64: Tree-SHA512: 6a0495c34bd952b2bb8c994a1450da7d3eee61225bb4ff0ce009c013f5e29dba94bb1c3ecef9989dc18c939909fdc8eba690a38f96da431ae9d64c23656de7d0
No, but generally we still put an assert in the fallthrough case. Putting the assert after the bitcoin/src/kernel/coinstats.cpp Lines 169 to 173 in f332c4f |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Sep 15, 2022
02c9e56 fs: fully initialize _OVERLAPPED for win32 (Cory Fields) Pull request description: ```bash fs.cpp: In member function ‘bool fsbridge::FileLock::TryLock()’: fs.cpp:129:32: error: missing initializer for member ‘_OVERLAPPED::InternalHigh’ [-Werror=missing-field-initializers] 129 | _OVERLAPPED overlapped = {0}; | ^ fs.cpp:129:32: error: missing initializer for member ‘_OVERLAPPED::<anonymous>’ [-Werror=missing-field-initializers] fs.cpp:129:32: error: missing initializer for member ‘_OVERLAPPED::hEvent’ [-Werror=missing-field-initializers] ``` Came up in bitcoin#25972. That PR is now rebased on this change. Closes: bitcoin#26006 ACKs for top commit: sipsorcery: tACK 02c9e56. hebasto: ACK 02c9e56, tested on Linux x86_64: Tree-SHA512: 6a0495c34bd952b2bb8c994a1450da7d3eee61225bb4ff0ce009c013f5e29dba94bb1c3ecef9989dc18c939909fdc8eba690a38f96da431ae9d64c23656de7d0
Maybe into the CI or dev docs, given we'll never use -Werror by default for compilation? |
Oh I see. |
|
Ported to the CMake-based build system in hebasto#188. |
hebasto
added a commit
to hebasto/bitcoin
that referenced
this pull request
May 8, 2024
1689b48 fixup! ci: Test CMake edge cases (Hennadii Stepanov) b72aeba fixup! ci: Test CMake edge cases (Hennadii Stepanov) f38ec56 fixup! cmake: Add compiler diagnostic flags (Hennadii Stepanov) Pull request description: This PR ports bitcoin#25972 after the recent sync/rebase [PR](#186). ACKs for top commit: vasild: ACK 1689b48 Tree-SHA512: 0cc7a6a1ad0c9e99e0dddcd69bf00ae182119b33ecc0f21d22446e207ce756b8fa7d865585dfd2fb43458c44db00194e9088f7ea01a1c7cc226f3117a20374d7
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Sep 27, 2024
1986f12 build: Update `libmultiprocess` library (Hennadii Stepanov) Pull request description: This update in particular includes: - bitcoin-core/libmultiprocess#78 which is [required](bitcoin#25972 (comment)) for bitcoin#25972 - bitcoin-core/libmultiprocess#74 - bitcoin-core/libmultiprocess#70 ACKs for top commit: ryanofsky: Code review ACK 1986f12 Tree-SHA512: 2d9fa72df5de7d5be37d77d479702cba36c45e9fa9d9fc27e58aac0437c39e4a1054c0ac53b612cb43e830982e444d98c7d3e651d093ac68344e66f4734227bb
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Sep 27, 2024
1986f12 build: Update `libmultiprocess` library (Hennadii Stepanov) Pull request description: This update in particular includes: - bitcoin-core/libmultiprocess#78 which is [required](bitcoin#25972 (comment)) for bitcoin#25972 - bitcoin-core/libmultiprocess#74 - bitcoin-core/libmultiprocess#70 ACKs for top commit: ryanofsky: Code review ACK 1986f12 Tree-SHA512: 2d9fa72df5de7d5be37d77d479702cba36c45e9fa9d9fc27e58aac0437c39e4a1054c0ac53b612cb43e830982e444d98c7d3e651d093ac68344e66f4734227bb
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Sep 27, 2024
1986f12 build: Update `libmultiprocess` library (Hennadii Stepanov) Pull request description: This update in particular includes: - bitcoin-core/libmultiprocess#78 which is [required](bitcoin#25972 (comment)) for bitcoin#25972 - bitcoin-core/libmultiprocess#74 - bitcoin-core/libmultiprocess#70 ACKs for top commit: ryanofsky: Code review ACK 1986f12 Tree-SHA512: 2d9fa72df5de7d5be37d77d479702cba36c45e9fa9d9fc27e58aac0437c39e4a1054c0ac53b612cb43e830982e444d98c7d3e651d093ac68344e66f4734227bb
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Sep 28, 2024
1986f12 build: Update `libmultiprocess` library (Hennadii Stepanov) Pull request description: This update in particular includes: - bitcoin-core/libmultiprocess#78 which is [required](bitcoin#25972 (comment)) for bitcoin#25972 - bitcoin-core/libmultiprocess#74 - bitcoin-core/libmultiprocess#70 ACKs for top commit: ryanofsky: Code review ACK 1986f12 Tree-SHA512: 2d9fa72df5de7d5be37d77d479702cba36c45e9fa9d9fc27e58aac0437c39e4a1054c0ac53b612cb43e830982e444d98c7d3e651d093ac68344e66f4734227bb
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Oct 1, 2024
1986f12 build: Update `libmultiprocess` library (Hennadii Stepanov) Pull request description: This update in particular includes: - bitcoin-core/libmultiprocess#78 which is [required](bitcoin#25972 (comment)) for bitcoin#25972 - bitcoin-core/libmultiprocess#74 - bitcoin-core/libmultiprocess#70 ACKs for top commit: ryanofsky: Code review ACK 1986f12 Tree-SHA512: 2d9fa72df5de7d5be37d77d479702cba36c45e9fa9d9fc27e58aac0437c39e4a1054c0ac53b612cb43e830982e444d98c7d3e651d093ac68344e66f4734227bb
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Oct 24, 2024
befb42f qt: Silence `-Wcast-function-type` warning (Hennadii Stepanov) Pull request description: On Fedora 38 @ 8f7b9eb: ``` $ x86_64-w64-mingw32-g++ --version | head -1 x86_64-w64-mingw32-g++ (GCC) 12.2.1 20221121 (Fedora MinGW 12.2.1-8.fc38) $ ./configure CONFIG_SITE=$PWD/depends/x86_64-w64-mingw32/share/config.site CXXFLAGS="-Wno-return-type -Wcast-function-type" $ make > /dev/null qt/winshutdownmonitor.cpp: In static member function 'static void WinShutdownMonitor::registerShutdownBlockReason(const QString&, HWND__* const&)': qt/winshutdownmonitor.cpp:46:42: warning: cast between incompatible function types from 'FARPROC' {aka 'long long int (*)()'} to 'PSHUTDOWNBRCREATE' {aka 'int (*)(HWND__*, const wchar_t*)'} [-Wcast-function-type] 46 | PSHUTDOWNBRCREATE shutdownBRCreate = (PSHUTDOWNBRCREATE)GetProcAddress(GetModuleHandleA("User32.dll"), "ShutdownBlockReasonCreate"); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ``` [Required](bitcoin#25972 (comment)) for bitcoin#25972. Picked from https://trac.nginx.org/nginx/ticket/1865. ACKs for top commit: MarcoFalke: review ACK befb42f Tree-SHA512: b37b2c5dd8702caf84d1833c3511bc46ee14f23b84678b8de0fd04e24e5ecc5fd4d27ba38be0d0b08de91299369f70d4924c895a71fd8e0b6feffcfb7407574a
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Oct 24, 2024
befb42f qt: Silence `-Wcast-function-type` warning (Hennadii Stepanov) Pull request description: On Fedora 38 @ 8f7b9eb: ``` $ x86_64-w64-mingw32-g++ --version | head -1 x86_64-w64-mingw32-g++ (GCC) 12.2.1 20221121 (Fedora MinGW 12.2.1-8.fc38) $ ./configure CONFIG_SITE=$PWD/depends/x86_64-w64-mingw32/share/config.site CXXFLAGS="-Wno-return-type -Wcast-function-type" $ make > /dev/null qt/winshutdownmonitor.cpp: In static member function 'static void WinShutdownMonitor::registerShutdownBlockReason(const QString&, HWND__* const&)': qt/winshutdownmonitor.cpp:46:42: warning: cast between incompatible function types from 'FARPROC' {aka 'long long int (*)()'} to 'PSHUTDOWNBRCREATE' {aka 'int (*)(HWND__*, const wchar_t*)'} [-Wcast-function-type] 46 | PSHUTDOWNBRCREATE shutdownBRCreate = (PSHUTDOWNBRCREATE)GetProcAddress(GetModuleHandleA("User32.dll"), "ShutdownBlockReasonCreate"); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ``` [Required](bitcoin#25972 (comment)) for bitcoin#25972. Picked from https://trac.nginx.org/nginx/ticket/1865. ACKs for top commit: MarcoFalke: review ACK befb42f Tree-SHA512: b37b2c5dd8702caf84d1833c3511bc46ee14f23b84678b8de0fd04e24e5ecc5fd4d27ba38be0d0b08de91299369f70d4924c895a71fd8e0b6feffcfb7407574a
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Oct 24, 2024
befb42f qt: Silence `-Wcast-function-type` warning (Hennadii Stepanov) Pull request description: On Fedora 38 @ 8f7b9eb: ``` $ x86_64-w64-mingw32-g++ --version | head -1 x86_64-w64-mingw32-g++ (GCC) 12.2.1 20221121 (Fedora MinGW 12.2.1-8.fc38) $ ./configure CONFIG_SITE=$PWD/depends/x86_64-w64-mingw32/share/config.site CXXFLAGS="-Wno-return-type -Wcast-function-type" $ make > /dev/null qt/winshutdownmonitor.cpp: In static member function 'static void WinShutdownMonitor::registerShutdownBlockReason(const QString&, HWND__* const&)': qt/winshutdownmonitor.cpp:46:42: warning: cast between incompatible function types from 'FARPROC' {aka 'long long int (*)()'} to 'PSHUTDOWNBRCREATE' {aka 'int (*)(HWND__*, const wchar_t*)'} [-Wcast-function-type] 46 | PSHUTDOWNBRCREATE shutdownBRCreate = (PSHUTDOWNBRCREATE)GetProcAddress(GetModuleHandleA("User32.dll"), "ShutdownBlockReasonCreate"); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ``` [Required](bitcoin#25972 (comment)) for bitcoin#25972. Picked from https://trac.nginx.org/nginx/ticket/1865. ACKs for top commit: MarcoFalke: review ACK befb42f Tree-SHA512: b37b2c5dd8702caf84d1833c3511bc46ee14f23b84678b8de0fd04e24e5ecc5fd4d27ba38be0d0b08de91299369f70d4924c895a71fd8e0b6feffcfb7407574a
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Oct 24, 2024
befb42f qt: Silence `-Wcast-function-type` warning (Hennadii Stepanov) Pull request description: On Fedora 38 @ 8f7b9eb: ``` $ x86_64-w64-mingw32-g++ --version | head -1 x86_64-w64-mingw32-g++ (GCC) 12.2.1 20221121 (Fedora MinGW 12.2.1-8.fc38) $ ./configure CONFIG_SITE=$PWD/depends/x86_64-w64-mingw32/share/config.site CXXFLAGS="-Wno-return-type -Wcast-function-type" $ make > /dev/null qt/winshutdownmonitor.cpp: In static member function 'static void WinShutdownMonitor::registerShutdownBlockReason(const QString&, HWND__* const&)': qt/winshutdownmonitor.cpp:46:42: warning: cast between incompatible function types from 'FARPROC' {aka 'long long int (*)()'} to 'PSHUTDOWNBRCREATE' {aka 'int (*)(HWND__*, const wchar_t*)'} [-Wcast-function-type] 46 | PSHUTDOWNBRCREATE shutdownBRCreate = (PSHUTDOWNBRCREATE)GetProcAddress(GetModuleHandleA("User32.dll"), "ShutdownBlockReasonCreate"); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ``` [Required](bitcoin#25972 (comment)) for bitcoin#25972. Picked from https://trac.nginx.org/nginx/ticket/1865. ACKs for top commit: MarcoFalke: review ACK befb42f Tree-SHA512: b37b2c5dd8702caf84d1833c3511bc46ee14f23b84678b8de0fd04e24e5ecc5fd4d27ba38be0d0b08de91299369f70d4924c895a71fd8e0b6feffcfb7407574a
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Apr 20, 2025
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Apr 20, 2025
kwvg
added a commit
to kwvg/dash
that referenced
this pull request
Apr 20, 2025
5 tasks
PastaPastaPasta
added a commit
to dashpay/dash
that referenced
this pull request
Apr 22, 2025
…bitcoin#29486, bitcoin#25972 (ensure `WARN_CXXFLAGS` are populated to ensure expected `--enable-werror` behavior) af14f23 ci: don't build using `-Werror` when using Clang (Kittywhiskers Van Gogh) 3b837c8 ci: don't build using `-Werror` when using GCC (Kittywhiskers Van Gogh) 04e036e revert: make fuzzing builds stricter by enabling -Werror by default (Kittywhiskers Van Gogh) 29090a0 merge bitcoin#25972: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set (Kittywhiskers Van Gogh) d0548f8 merge bitcoin#29486: remove -Wdocumentation conditional (Kittywhiskers Van Gogh) 0f4812f merge bitcoin#27872: suppress external warnings by default (Kittywhiskers Van Gogh) 7100780 merge bitcoin#28999: Enable -Wunreachable-code (Kittywhiskers Van Gogh) 5471c58 merge bitcoin#28092: document that -Wreturn-type has been fixed upstream (mingw-w64) (Kittywhiskers Van Gogh) 9098c9c build: always enable -Wsuggest-override (Kittywhiskers Van Gogh) Pull request description: ## Motivation While working on [dash#6633](#6633), I had built `develop` (5e4a892) but the build _failed_ locally due to a `-Wthread-safety` warning (see below) that was introduced by [bitcoin#25337](bitcoin#25337) ([commit](a7d4127)). It was caught because I use additional `CXXFLAGS` on my local system but it _should_ have been caught by CI, especially since [bitcoin#20182](bitcoin#20182) ([commit](14a67ee)) made `--enable-werror` the default for CI and the sole exception (the Windows build) was remedied with [bitcoin#20586](bitcoin#20586) ([commit](750447e)), so every build variant should've run with `-Werror`. <details> <summary>Compile error:</summary> ``` CXX wallet/libbitcoin_wallet_a-sqlite.o wallet/rpcwallet.cpp:1038:36: error: calling function 'IsMine' requires holding mutex 'wallet.cs_wallet' exclusively [-Werror,-Wthread-safety-analysis] 1038 | isminefilter mine = wallet.IsMine(address); | ^ ``` </details> But that didn't happen. Till [bitcoin#23149](70ed6b4), there were a separate set of warnings (overridable by `CXXFLAGS`) and errors (overridable by `CXXFLAG_WERROR`). _Before_ the backport, coverage was as expected ([build](https://gitlab.com/dashpay/dash/-/jobs/9221165750#L786), search for `-Werror=thread-safety`) but _after_ the backport, `thread-safety` (and other expected warnings) were no longer being evaluated ([build](https://gitlab.com/dashpay/dash/-/jobs/9238308844#L740), search for `-Werror` and `-Wthread-safety`, only the former is present). Expected `CXXFLAGS`: ``` CXXFLAGS = -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_top_srcdir)=. -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wreorder -Wvla -Wshadow-field -Wthread-safety -Wloop-analysis -Wredundant-decls -Wunused-member-function -Wdate-time -Wconditional-uninitialized -Woverloaded-virtual -Wsuggest-override -Wunreachable-code-loop-increment -Wimplicit-fallthrough -Wno-unused-parameter -Wno-self-assign -Werror -pipe -std=c++20 -O2 -O0 -g0 ``` Actual `CXXFLAGS`: ``` CXXFLAGS = -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_top_srcdir)=. -Werror -pipe -std=c++20 -O2 -O0 -g0 ``` This happened because `CXXFLAGS` are overridden by default which results in none of the warnings making it to the final `CXXFLAGS`, which reduced the effectiveness of `-Werror` substantially (while `CXXFLAG_WERROR` was left undisturbed, which allowed pre-backport builds to include coverage). This is remedied by backporting [bitcoin#25972](bitcoin#25972) (done by this PR), which will ensure that `WARN_CXXFLAGS` are included _even if `CXXFLAGS` are overridden_ and is possible because [bitcoin#24391](bitcoin#24391) ([commit](11323c3)) is already in `develop`. ## Additional Information * Dependency for #6638 * Dependency for #6639 * Because the warnings (converted to errors with `-Werror`) cast a wider net than the older set of error flags, `develop` in its current state does not compile. To allow CI to bless this PR, `-Werror` for both Clang and GCC-based builds have been **tentatively** disabled and will be re-enabled by the dependent PRs listed above. It is recommended to read the pull request description for both dependents while reviewing this PR. * `-Wsuggest-override` was made unconditional in [bitcoin#28348](bitcoin#28348) ([commit](c71e3df)) **but** there were two such conditional checks, they were deduplicated in [bitcoin#23149](bitcoin#23149) but the former was merged before the latter (i.e. out-of-order) and one conditional check lingered around. This lingering check has been removed as we don't support GCC 9.2. * `CXXFLAGS` set for the fuzz build ([commit](184bd60)) that enabled `-Werror` are made redundant with [bitcoin#20182](bitcoin#20182) and therefore, have been removed. ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK af14f23 Tree-SHA512: ccdaf71cf79eb3aec2468c4c1eaa696cd120c03e9665a3c4b56da6ef17cca9585ef8c66ac1625f2ba243c7f80f15e92a336c0bd90b5f11969fabb3adde3c8125
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Apr 22, 2025
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Apr 22, 2025
…n#27872, bitcoin#29486, bitcoin#25972 (ensure `WARN_CXXFLAGS` are populated to ensure expected `--enable-werror` behavior) af14f23 ci: don't build using `-Werror` when using Clang (Kittywhiskers Van Gogh) 3b837c8 ci: don't build using `-Werror` when using GCC (Kittywhiskers Van Gogh) 04e036e revert: make fuzzing builds stricter by enabling -Werror by default (Kittywhiskers Van Gogh) 29090a0 merge bitcoin#25972: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set (Kittywhiskers Van Gogh) d0548f8 merge bitcoin#29486: remove -Wdocumentation conditional (Kittywhiskers Van Gogh) 0f4812f merge bitcoin#27872: suppress external warnings by default (Kittywhiskers Van Gogh) 7100780 merge bitcoin#28999: Enable -Wunreachable-code (Kittywhiskers Van Gogh) 5471c58 merge bitcoin#28092: document that -Wreturn-type has been fixed upstream (mingw-w64) (Kittywhiskers Van Gogh) 9098c9c build: always enable -Wsuggest-override (Kittywhiskers Van Gogh) Pull request description: ## Motivation While working on [dash#6633](dashpay#6633), I had built `develop` (5e4a892) but the build _failed_ locally due to a `-Wthread-safety` warning (see below) that was introduced by [bitcoin#25337](bitcoin#25337) ([commit](dashpay@a7d4127)). It was caught because I use additional `CXXFLAGS` on my local system but it _should_ have been caught by CI, especially since [bitcoin#20182](bitcoin#20182) ([commit](dashpay@14a67ee)) made `--enable-werror` the default for CI and the sole exception (the Windows build) was remedied with [bitcoin#20586](bitcoin#20586) ([commit](dashpay@750447e)), so every build variant should've run with `-Werror`. <details> <summary>Compile error:</summary> ``` CXX wallet/libbitcoin_wallet_a-sqlite.o wallet/rpcwallet.cpp:1038:36: error: calling function 'IsMine' requires holding mutex 'wallet.cs_wallet' exclusively [-Werror,-Wthread-safety-analysis] 1038 | isminefilter mine = wallet.IsMine(address); | ^ ``` </details> But that didn't happen. Till [bitcoin#23149](dashpay@70ed6b4), there were a separate set of warnings (overridable by `CXXFLAGS`) and errors (overridable by `CXXFLAG_WERROR`). _Before_ the backport, coverage was as expected ([build](https://gitlab.com/dashpay/dash/-/jobs/9221165750#L786), search for `-Werror=thread-safety`) but _after_ the backport, `thread-safety` (and other expected warnings) were no longer being evaluated ([build](https://gitlab.com/dashpay/dash/-/jobs/9238308844#L740), search for `-Werror` and `-Wthread-safety`, only the former is present). Expected `CXXFLAGS`: ``` CXXFLAGS = -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_top_srcdir)=. -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wreorder -Wvla -Wshadow-field -Wthread-safety -Wloop-analysis -Wredundant-decls -Wunused-member-function -Wdate-time -Wconditional-uninitialized -Woverloaded-virtual -Wsuggest-override -Wunreachable-code-loop-increment -Wimplicit-fallthrough -Wno-unused-parameter -Wno-self-assign -Werror -pipe -std=c++20 -O2 -O0 -g0 ``` Actual `CXXFLAGS`: ``` CXXFLAGS = -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_top_srcdir)=. -Werror -pipe -std=c++20 -O2 -O0 -g0 ``` This happened because `CXXFLAGS` are overridden by default which results in none of the warnings making it to the final `CXXFLAGS`, which reduced the effectiveness of `-Werror` substantially (while `CXXFLAG_WERROR` was left undisturbed, which allowed pre-backport builds to include coverage). This is remedied by backporting [bitcoin#25972](bitcoin#25972) (done by this PR), which will ensure that `WARN_CXXFLAGS` are included _even if `CXXFLAGS` are overridden_ and is possible because [bitcoin#24391](bitcoin#24391) ([commit](dashpay@11323c3)) is already in `develop`. ## Additional Information * Dependency for dashpay#6638 * Dependency for dashpay#6639 * Because the warnings (converted to errors with `-Werror`) cast a wider net than the older set of error flags, `develop` in its current state does not compile. To allow CI to bless this PR, `-Werror` for both Clang and GCC-based builds have been **tentatively** disabled and will be re-enabled by the dependent PRs listed above. It is recommended to read the pull request description for both dependents while reviewing this PR. * `-Wsuggest-override` was made unconditional in [bitcoin#28348](bitcoin#28348) ([commit](dashpay@c71e3df)) **but** there were two such conditional checks, they were deduplicated in [bitcoin#23149](bitcoin#23149) but the former was merged before the latter (i.e. out-of-order) and one conditional check lingered around. This lingering check has been removed as we don't support GCC 9.2. * `CXXFLAGS` set for the fuzz build ([commit](dashpay@184bd60)) that enabled `-Werror` are made redundant with [bitcoin#20182](bitcoin#20182) and therefore, have been removed. ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK af14f23 Tree-SHA512: ccdaf71cf79eb3aec2468c4c1eaa696cd120c03e9665a3c4b56da6ef17cca9585ef8c66ac1625f2ba243c7f80f15e92a336c0bd90b5f11969fabb3adde3c8125
knst
added a commit
to knst/dash
that referenced
this pull request
Apr 23, 2025
…bitcoin#27872, bitcoin#29486, bitcoin#25972 (ensure `WARN_CXXFLAGS` are populated to ensure expected `--enable-werror` behavior)" This reverts commit 26f438f, reversing changes made to e053c8b.
knst
added a commit
to knst/dash
that referenced
this pull request
Apr 25, 2025
…when CXXFLAGS is set" This reverts commit 29090a0.
5 tasks
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Now that
CXXFLAGSare back in user control, I don't think there's areason to no-longer use our warning flags when
CXXFLAGShas beenoverriden (this includes, by default, when building from depends).
Anyone can suppress warnings from third-party code by
passing the relevant
-Wno-options inCXXFLAGS.Closes: #18092.