build: merge bitcoin#28092, #28999, #27872, #29486, #25972 (ensure WARN_CXXFLAGS are populated to ensure expected --enable-werror behavior)
#6637
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
5 tasks
…eam (mingw-w64)
Merged
5 tasks
Now that our set of warnings has expanded, strict enforcement results in build failure. Those warnings will be resolved and this flag reverted in a subsequent pull request.
Now that our set of warnings has expanded, strict enforcement results in build failure. Those warnings will be resolved and this flag reverted in a subsequent pull request.
5 tasks
WalkthroughThis set of changes primarily updates build configuration scripts and environment setup files used in continuous integration (CI) workflows. The Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
configure.ac (2)
214-218: Default flip may confuse future maintainers – add an explicit comment or rename the knob
AC_ARG_ENABLE([suppress-external-warnings], …, [suppress_external_warnings=$enableval], [suppress_external_warnings=yes])
now makes suppression enabled by default (yes).
Almost every follow‑up check still uses
if test "$suppress_external_warnings" != "no" …which happens to keep the
old behaviour working, but the mental mapping “yes ⇒ suppress” /
“no ⇒ don’t suppress” is no longer obvious from the variable name.Consider one of the following so the intent stays crystal‑clear:
+# Suppress warnings from external headers by default. “yes” means +# suppression is on, “no” means show external warnings.or rename the variable to
suppress_external_warnings_default_on(or similar)
and adjust tests accordingly.This tiny clarification can save hours when somebody revisits this file months
from now.
460-482: Large flat list ofAX_CHECK_COMPILE_FLAGcalls – consider a helper macroThe 20‑odd consecutive
AX_CHECK_COMPILE_FLAGinvocations are mechanically
repetitive and make the section hard to scan. A thin wrapper macro (e.g.
ADD_WARN_FLAG(flag)) would keep the list declarative and reduce boiler‑plate
while preserving the compile‑time probes.Not blocking, just readability food‑for‑thought.
doc/developer-notes.md (1)
221-221: Clarify behavior of clang-tidy output denoising
Now that suppression is enabled by default, the note “output is denoised of errors from external dependencies” may no longer require additional flags. Consider rephrasing to explain that denoising happens automatically.ci/test/00_setup_env_native_multiprocess.sh (1)
18-18: Redundant-Wno-error=documentationwithNO_WERROR=1
SinceNO_WERROR=1disables treating warnings as errors globally, the-Wno-error=documentationis no longer needed. You may remove it for clarity.-export BITCOIN_CONFIG="${BITCOIN_CONFIG} --disable-hardening CFLAGS='-O0 -g0' CXXFLAGS='-O0 -g0 -Wno-error=documentation'" +export BITCOIN_CONFIG="${BITCOIN_CONFIG} --disable-hardening CFLAGS='-O0 -g0' CXXFLAGS='-O0 -g0'"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
ci/dash/build_src.sh(1 hunks)ci/test/00_setup_env_arm.sh(1 hunks)ci/test/00_setup_env_mac.sh(1 hunks)ci/test/00_setup_env_native_fuzz.sh(1 hunks)ci/test/00_setup_env_native_fuzz_with_valgrind.sh(1 hunks)ci/test/00_setup_env_native_multiprocess.sh(1 hunks)ci/test/00_setup_env_native_nowallet.sh(1 hunks)ci/test/00_setup_env_native_qt5.sh(1 hunks)ci/test/00_setup_env_native_sqlite.sh(1 hunks)ci/test/00_setup_env_native_tsan.sh(1 hunks)ci/test/00_setup_env_native_ubsan.sh(1 hunks)ci/test/00_setup_env_native_valgrind.sh(1 hunks)ci/test/00_setup_env_win64.sh(1 hunks)configure.ac(2 hunks)doc/developer-notes.md(1 hunks)src/wallet/walletdb.cpp(1 hunks)
🔇 Additional comments (19)
configure.ac (1)
483-490: Inconsistent condition for-Wno-deprecated-copy– double‑check intentHere the guard was changed to
if test "$suppress_external_warnings" != "yes"
while most other places keep!= "no". The asymmetry is intentional if the
flag should only be added when external warnings are not suppressed.
If that is the case, great – maybe drop a one‑liner comment to make it
explicit; otherwise this may be an overlooked logic inversion.ci/test/00_setup_env_mac.sh (1)
18-18: Environment hand‑shake: confirmNO_WERRORis consumed downstream
export NO_WERROR=1looks good and is consistent with the other CI presets.
Just ensureci/dash/build_src.sh(or any delegated script) actually checks
for this variable; otherwise the flag will be inert and may give a false sense
of safety in warning‑heavy builds.ci/test/00_setup_env_native_nowallet.sh (1)
15-15: Consistency with other CI jobsSame remark as for macOS: the addition is fine, but please make sure all build
entry points (native + cross) honourNO_WERRORso the behaviour is uniform
across matrices.ci/test/00_setup_env_native_sqlite.sh (1)
14-14: Introduce NO_WERROR to disable warnings-as-errors
The addition ofexport NO_WERROR=1ensures that warnings are not treated as errors in this CI environment, aligning with the temporary relaxation of strict-Werrorenforcement across all setups.ci/dash/build_src.sh (1)
36-36: Remove deprecated suppress-external-warnings flag
UpdatingBITCOIN_CONFIG_ALLto drop--enable-suppress-external-warningsaligns withconfigure.acnow enabling external warnings suppression by default, reducing redundancy in the CI build configuration.ci/test/00_setup_env_native_ubsan.sh (1)
17-17: Add NO_WERROR to UBSan environment
ExportingNO_WERROR=1here disables treating warnings as errors when building with the undefined sanitizer, consistent with other CI environments.ci/test/00_setup_env_native_tsan.sh (1)
18-18: Add NO_WERROR to TSan environment
Addingexport NO_WERROR=1prevents warnings from being escalated to errors during ThreadSanitizer builds, matching the unified CI strategy.ci/test/00_setup_env_native_qt5.sh (1)
19-19: Disable warnings-as-errors in Qt5 CI setup
The newexport NO_WERROR=1ensures that the Qt5 CI container does not treat compiler warnings as errors, in line with the temporary CI-wide override.doc/developer-notes.md (1)
217-217: Remove--enable-suppress-external-warningsfrom clang-tidy invocation
The change correctly aligns the documentation with the now-default behavior of suppressing external warnings. Ensure that any scripts or CI docs referencing this flag are also updated.ci/test/00_setup_env_native_multiprocess.sh (1)
20-20: Disable warnings-as-errors for multiprocess build
SettingNO_WERROR=1is consistent with other CI configurations to avoid unexpected failures.ci/test/00_setup_env_arm.sh (2)
28-28: Remove suppression flag in ARM build config
The removal of--enable-suppress-external-warningsis correct. Ensure this aligns withconfigure.acdefaults.
29-29: Disable warnings-as-errors for ARM build
AddingNO_WERROR=1is consistent and prevents local build failures due to new warnings.ci/test/00_setup_env_native_fuzz.sh (2)
18-18: Update native fuzz configuration flags
Removing--enable-suppress-external-warningsand adjusting sanitizer flags matches upstream logic. Confirm thatfuzzer,address,undefined,integersanitizers run as expected without error enforcement.
19-19: Disable warnings-as-errors for native fuzz build
AddingNO_WERROR=1prevents warnings from being treated as errors, aligning with other CI environments.ci/test/00_setup_env_native_valgrind.sh (1)
14-15: Build configuration modified to address warning issues.The removal of
--enable-suppress-external-warningsand addition ofNO_WERROR=1align with the PR's goal to address issues with compiler warning flags. These changes temporarily disable treating warnings as errors while maintaining the full set of warning flags, which will help identify warnings without failing builds.ci/test/00_setup_env_win64.sh (2)
16-19: Well-documented compiler flag exceptions for mingw-w64.The added comments clearly explain why specific warning suppressions are needed for Windows builds. This improves code maintainability by documenting the reason for these compiler flags, which are necessary to handle missing noreturn attributes in mingw-w64 headers prior to version 11.0.0.
21-21: Temporary disabling of -Werror consistent with other CI scripts.Adding
NO_WERROR=1is consistent with the overall PR approach to temporarily disable treating warnings as errors while the codebase is updated to address all warnings.src/wallet/walletdb.cpp (2)
1162-1168: Fixed conditional compilation structure for proper error handling.The changed code correctly structures the conditional compilation logic to ensure proper error handling when SQLite support is not compiled in. The original code structure might have led to improper error reporting or return logic execution.
1171-1177: Fixed conditional compilation structure for Berkeley DB.Similar to the SQLite case, this change properly structures the conditional compilation logic for Berkeley DB support, ensuring the correct error handling path is followed when Berkeley DB support is not compiled in.
knst
reviewed
Apr 22, 2025
| @@ -13,5 +13,8 @@ export PACKAGES="python3 nsis g++-mingw-w64-x86-64-posix wine-binfmt wine64 wine | |||
| export RUN_FUNCTIONAL_TESTS=false | |||
| export RUN_SECURITY_TESTS="false" | |||
| export GOAL="deploy" | |||
| export BITCOIN_CONFIG="--enable-gui --enable-reduce-exports --disable-miner --without-boost-process" | |||
| # Prior to 11.0.0, the mingw-w64 headers were missing noreturn attributes, causing warnings when | |||
There was a problem hiding this comment.
extra input for reviewers: we require at least gcc 11.1
| [GCC](https://gcc.gnu.org) | [11.1](https://github.com/bitcoin/bitcoin/pull/29091) |
WARN_CXXFLAGS is populated to ensure expected --enable-werror behavior)WARN_CXXFLAGS are populated to ensure expected --enable-werror behavior)
UdjinM6
approved these changes
Apr 22, 2025
PastaPastaPasta
requested changes
Apr 22, 2025
There was a problem hiding this comment.
doesn't build locally due to below and others:
node/coinstats.cpp:94:13: error: unused function 'ApplyStats' [-Werror,-Wunused-function]
94 | static void ApplyStats(CCoinsStats& stats, std::nullptr_t, const uint256& hash, const std::map<uint32_t, Coin>& outputs)
| ^~~~~~~~~~
1 error generated.
|
@PastaPastaPasta, it won't build locally if you have |
PastaPastaPasta
approved these changes
Apr 22, 2025
PastaPastaPasta
added a commit
that referenced
this pull request
Apr 22, 2025
…Werror` for Clang, merge bitcoin#13633 c7748cd revert: don't build using `-Werror` when using Clang (Kittywhiskers Van Gogh) 1015f9b fix: resolve `-Wunreachable-code` warnings, add comment for explanation (Kittywhiskers Van Gogh) 10cb3e4 fix: resolve `-Wlogical-op-parentheses` warnings (Kittywhiskers Van Gogh) 7459b1e fix: resolve `-Wunused-private-field` warnings (Kittywhiskers Van Gogh) 7a52083 fix: resolve `-Wmissing-field-initializers` warnings (Kittywhiskers Van Gogh) 184b463 fix: resolve `-Wdelete-non-abstract-non-virtual-dtor` warnings (Kittywhiskers Van Gogh) 53d29e9 fix: sidestep `-Wunused-function` warnings with `[[maybe_unused]]` (Kittywhiskers Van Gogh) 9315e69 move-only: keep functions used in wallet-enabled segments within macro (Kittywhiskers Van Gogh) 6729d69 fix: add missing lock annotation in lambda expression (Kittywhiskers Van Gogh) f6d1eb9 chore: drop extraneous unused `ApplyStats()` definition (Kittywhiskers Van Gogh) df9c926 chore: drop unused `EraseOldDBData()` (Kittywhiskers Van Gogh) c9cb9a6 merge bitcoin#13633: Drop dead code from Stacks (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Depends on #6637 * Dependency for #6639 * While [bitcoin#13633](bitcoin#13633) was originally marked as DNM in the backports spreadsheet for being SegWit-related, it's not SegWit-_dependent_ and is for dead code removal, which is necessary due to the enforcement of `-Wunused-function`. It has therefore been included. * `EraseOldDBData()` fell into disuse after [dash#6579](#6579) but wasn't removed then. It has been removed now due to `-Wunused-function` enforcement. * An extraneous `ApplyStats()` definition was left over when backporting [bitcoin#19145](bitcoin#19145). It has been removed now due to `-Wunused-function` enforcement. * [bitcoin#25337](bitcoin#25337) ([commit](a7d4127)) did not include a lock annotation for the lambda expression in `ListReceived()`. It has been added alongside `AssertLockHeld`s for both the lambda expression and the function. This was done to pass `-Wthread-safety` enforcement. * `ParsePubKeyIDFromAddress()`, `ParseBLSPubKey()` and `ValidatePlatformPort()` have been moved inside the `ENABLE_WALLET` macro conditional to avoid `-Wunused-function` warnings if building with wallets disabled. * Some dead code cannot be removed because they are either part of partial backports that may be completed in the future or complicate backports in the future that may expect their presence. To keep `-Wunused-function` happy, they have been marked with `[[maybe_unused]]` and this annotation should be removed when they are used. ## 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 - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK c7748cd PastaPastaPasta: utACK c7748cd Tree-SHA512: 5d746773a1f49c33f373250ba459a5da4301fea0cb5202d26717808d8a4985920cbdfec0903ed884f20561f8d40082b4ea796e78912bacff00e0645d355ec9f2
|
Obviously, |
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.
PastaPastaPasta
added a commit
that referenced
this pull request
Apr 27, 2025
…ray-bounds` and `-Wdangling-reference`, re-enable `-Werror` for GCC 40caa7d revert: don't build using `-Werror` when using GCC (Kittywhiskers Van Gogh) 96e494c build: constrain `-Wstringop-over{read,flow}` suppression to GCC only (Kittywhiskers Van Gogh) 6bff03b build: don't error on `-Wattributes` with GCC <14 (Kittywhiskers Van Gogh) 9aab60b build: don't error on `-Warray-bounds` and `-Wdangling-reference` with GCC (Kittywhiskers Van Gogh) 18fcb26 fix: resolve `-Wunused-function` warning (Kittywhiskers Van Gogh) 7a65315 fix: resolve `-Wmaybe-uninitialized` warnings (Kittywhiskers Van Gogh) b208018 fix: resolve `-Wignored-qualifiers` warning (Kittywhiskers Van Gogh) 7dd0daa fix: resolve `-Woverloaded-virtual` warning (Kittywhiskers Van Gogh) c587838 refactor: use `GetTransactionBlock()` as narrow `GetTransaction()` proxy (Kittywhiskers Van Gogh) d9828a9 refactor: move `ToJson` definitions relied by `TxToUniv` to source file (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Depends on #6637 * Depends on #6638 * `TxToUniv()` is a function defined in `core_write.cpp`, which is a part of `libbitcoin_common` that relies on `ToJson()` definitions in various classes (`CCbTx`, `CProRegTx` and others) to print out of contents of transaction payload. The problem is, those various classes have their source files a part of `libbitcoin_server`, so when binaries like `dash-tx` (which do not use `libbitcoin_server`) need to be linked, the necessary `ToJson()` implementations, were they defined in source files, wouldn't be found and this would result in a linking error. This is worked around by defining them in the header instead but this creates a new problem where these headers are included by `core_write` but constructing the UniValue objects returned by `ToJson()` needs something from `core_write` (i.e. there is a circular dependency). This was worked around in [dash#6229](#6229) ([commit](b330318)) with redefinition, which was fine but is now verboten with `-Wredundant-decls` enforcement. * To work around this, all `ToJson()` definitions from those headers relied on by `TxToUniv()` have been moved to their own source file (`evo/core_write.cpp`) and included in `libbitcoin_common`. This takes care of the linking issue and avoids circular dependencies. * `GetTransaction()` is extensively used in Dash-specific code and a redefinition in `validation.cpp` is used to reduce the amount of reported circular dependencies, removing that circular dependency in order to satisfy `-Wredundant-decls` results in the following (see below). <details> <summary>Circular dependencies:</summary> Removed: ``` "evo/deterministicmns -> validation -> txmempool -> evo/deterministicmns" ``` Added: ``` "node/transaction -> validation -> node/transaction" "evo/deterministicmns -> node/transaction -> net_processing -> evo/deterministicmns" "evo/deterministicmns -> node/transaction -> node/blockstorage -> evo/deterministicmns" "evo/deterministicmns -> node/transaction -> node/context -> evo/deterministicmns" "evo/deterministicmns -> node/transaction -> txmempool -> evo/deterministicmns" "evo/chainhelper -> llmq/chainlocks -> node/transaction -> node/context -> evo/chainhelper" "evo/deterministicmns -> node/transaction -> net_processing -> evo/mnauth -> evo/deterministicmns" "evo/deterministicmns -> node/transaction -> node/context -> governance/governance -> evo/deterministicmns" "evo/deterministicmns -> node/transaction -> net_processing -> llmq/dkgsession -> evo/deterministicmns" "evo/deterministicmns -> node/transaction -> net_processing -> llmq/dkgsessionmgr -> evo/deterministicmns" "evo/deterministicmns -> node/transaction -> net_processing -> llmq/quorums -> evo/deterministicmns" "evo/deterministicmns -> node/transaction -> node/blockstorage -> masternode/node -> evo/deterministicmns" "governance/governance -> governance/object -> node/transaction -> node/context -> governance/governance" "llmq/chainlocks -> node/transaction -> node/context -> llmq/context -> llmq/chainlocks" "llmq/context -> llmq/instantsend -> node/transaction -> node/context -> llmq/context" "coinjoin/context -> coinjoin/server -> evo/deterministicmns -> node/transaction -> node/context -> coinjoin/context" "evo/cbtx -> evo/deterministicmns -> node/transaction -> node/context -> evo/creditpool -> evo/cbtx" "evo/deterministicmns -> node/transaction -> net_processing -> llmq/dkgsession -> llmq/debug -> evo/deterministicmns" "evo/deterministicmns -> node/transaction -> node/context -> evo/mnhftx -> llmq/signing -> llmq/signing_shares -> evo/deterministicmns" ``` Diff: **+18** </details> To avoid this, alongside some header adjustments, a proxy to `GetTransaction()` has been introduced in `evo/chainhelper.cpp`, `GetTransactionBlock()`. This _reduces_ the number of circular dependencies (see below). <details> <summary>Circular dependencies:</summary> Removed: ``` "consensus/tx_verify -> evo/assetlocktx -> validation -> consensus/tx_verify" "consensus/tx_verify -> evo/assetlocktx -> validation -> txmempool -> consensus/tx_verify" "evo/assetlocktx -> validation -> txmempool -> evo/assetlocktx" "evo/deterministicmns -> validation -> evo/deterministicmns" "evo/deterministicmns -> validation -> txmempool -> evo/deterministicmns" ``` Added: ``` "evo/chainhelper -> llmq/chainlocks -> evo/chainhelper" "evo/chainhelper -> llmq/instantsend -> evo/chainhelper" "evo/chainhelper -> evo/specialtxman -> evo/deterministicmns -> evo/chainhelper" "evo/chainhelper -> node/transaction -> node/context -> evo/chainhelper" "evo/deterministicmns -> llmq/commitment -> validation -> evo/deterministicmns" "consensus/tx_verify -> evo/assetlocktx -> llmq/commitment -> validation -> consensus/tx_verify" "evo/assetlocktx -> llmq/signing -> net_processing -> txmempool -> evo/assetlocktx" "evo/chainhelper -> masternode/payments -> governance/classes -> governance/object -> evo/chainhelper" "evo/deterministicmns -> llmq/commitment -> validation -> txmempool -> evo/deterministicmns" "consensus/tx_verify -> evo/assetlocktx -> llmq/signing -> net_processing -> txmempool -> consensus/tx_verify" ``` Diff: **+5** </details> To differentiate it from `GetTransaction()`, since all Dash-specific invocations either rely on the transaction index or the mempool, we can safely trim down the number of arguments and add a fast-fail. * Even with `-Werror` enabled, we have to downgrade `-Warray-bounds` to warnings (i.e. use `-Wno-error=array-bounds`) as there are two sources of the warning, `immer` (see [arximboldi/immer#223](arximboldi/immer#223)) and serialization code (fixed with [bitcoin#30765](bitcoin#30765)). As due to the former, we need to suppress it anyways, the latter has not been backported in the interest of brevity. * Similar to the above, we have to downgrade `-Wdangling-reference` as it is caused by UniValue-adjacent code that is fixed by [bitcoin#27605](bitcoin#27605) but would require an out-of-order backport that pulls in multiple dependent PRs in order to be complete (or alternatively, backport it as-is and then make future backporting annoying). The simpler solution to downgrade the warning was opted for instead. * We need to annotate `CHDPubKey::nVersion` with `[[maybe_unused]]` to avoid upsetting Clang due to `-Wunused-private-field`. GCC 11 doesn't emit a warning for that variable and therefore, finds the annotation unnecessary, triggering an error due to `-Wattributes` ([build](https://gitlab.com/dashpay/dash/-/jobs/9777475240#L1581)). Since both Clang 18 and GCC 14 agree with the `-Wunused-private-field` assessment, we are better off downgrading `-Wattributes` to a warning for GCC 11. ## 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 **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: knst: ACK 40caa7d UdjinM6: utACK 40caa7d Tree-SHA512: 4fb383f4e2142db24ffedffd023cc075804c07e499abfcfc5708047d64fd981599cdb7479d92f501bedede6d94b76cc16f77f1372dd5bce7e1906ddb374679c3
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Motivation
While working on dash#6633, I had built
develop(5e4a892) but the build failed locally due to a-Wthread-safetywarning (see below) that was introduced by bitcoin#25337 (commit). It was caught because I use additionalCXXFLAGSon my local system but it should have been caught by CI, especially since bitcoin#20182 (commit) made--enable-werrorthe default for CI and the sole exception (the Windows build) was remedied with bitcoin#20586 (commit), so every build variant should've run with-Werror.Compile error:
But that didn't happen. Till bitcoin#23149, there were a separate set of warnings (overridable by
CXXFLAGS) and errors (overridable byCXXFLAG_WERROR). Before the backport, coverage was as expected (build, search for-Werror=thread-safety) but after the backport,thread-safety(and other expected warnings) were no longer being evaluated (build, search for-Werrorand-Wthread-safety, only the former is present).Expected
CXXFLAGS:Actual
CXXFLAGS:This happened because
CXXFLAGSare overridden by default which results in none of the warnings making it to the finalCXXFLAGS, which reduced the effectiveness of-Werrorsubstantially (whileCXXFLAG_WERRORwas left undisturbed, which allowed pre-backport builds to include coverage). This is remedied by backporting bitcoin#25972 (done by this PR), which will ensure thatWARN_CXXFLAGSare included even ifCXXFLAGSare overridden and is possible because bitcoin#24391 (commit) is already indevelop.Additional Information
Dependency for fix: resolve or sidestep compiler warnings, re-enable
-Werrorfor Clang, merge bitcoin#13633 #6638Dependency for fix: resolve compiler warnings, suppress errors on
-Warray-boundsand-Wdangling-reference, re-enable-Werrorfor GCC #6639Because the warnings (converted to errors with
-Werror) cast a wider net than the older set of error flags,developin its current state does not compile. To allow CI to bless this PR,-Werrorfor 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-overridewas made unconditional in bitcoin#28348 (commit) but there were two such conditional checks, they were deduplicated in 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.CXXFLAGSset for the fuzz build (commit) that enabled-Werrorare made redundant with bitcoin#20182 and therefore, have been removed.Breaking Changes
None expected.
Checklist