backport: Merge bitcoin/bitcoin#23149: build: make --enable-werror just -Werror #6596
Conversation
38fd709 build: make --enable-werror just -Werror (fanquake) Pull request description: No longer special case a set of warnings, to make up our own -Werror, just use -Werror outright. This shouldn't really have any effect on existing builders, who were already using `--enable-werror`, and is more inline with what they would expect `--enable-werror` to be, which is erroring on any/all warnings. We keep `-Wno-error=return-type` because we know that is broken when using mingw-w64. It should only be applied when cross-compiling for Windows. Similar to the change in bitcoin#20544, but with (hopefully) less work-arounds, and other bundled changes. A step towards some configure "cleanups". ACKs for top commit: hebasto: ACK 38fd709 (also see bitcoin#23149 (comment)), tested: MarcoFalke: Concept ACK 38fd709 Tree-SHA512: 37f1857d9408442cab63e40f9280427b73e09cdf03146b19c1339d7e44abd78e93df7f270ca1da0e83b79343cd3ea915f7b9e4e347488b5bc5ceaaa7540e5926
WalkthroughThe changes consist of two main modifications. First, adjustments in the build configuration simplify the handling of compiler warning flags. The logic previously used for multiple specific Second, several Qt source files have been updated to modify the lambda capture semantics in signal-slot connections. The lambda expressions, which previously captured all local variables by value using ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
|
Looks like gcc11 can't handle this. Bumping to gcc12 c0719a8 fixes the issue https://github.com/UdjinM6/dash/actions/runs/13493387032. Not sure if we need to do some other backports first though. @kwvg pls chime in. |
I'd prefer to keep gcc 11 in CI so we know that our code is buildable with that "minimum" supported version and still officially support gcc 11 awhile. I'd prepare other fix soon |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/uint256.h (1)
55-57: Enhance the TODO comment to better document the rationale.The TODO comment could be improved to:
- Reference the specific GCC-11 bug/issue if available
- Document when this workaround can be removed (e.g., "Can be removed once minimum GCC version is >= 12")
- Explain why the cast is safe in this context
- // TODO: remove this gcc-11 workaround for memcmp - // gcc-11 can not recognise size of internal data in std::array even if width is not zero and show this warning: - // error: 'int __builtin_memcmp_eq(const void*, const void*, long unsigned int)' specified bound 32 exceeds source size 0 [-Werror=stringop-overread] + // TODO: Remove this GCC-11 workaround once minimum GCC version is >= 12 + // GCC-11 cannot recognize the size of internal data in std::array, resulting in: + // error: 'int __builtin_memcmp_eq(const void*, const void*, long unsigned int)' + // specified bound 32 exceeds source size 0 [-Werror=stringop-overread] + // The cast to const void* is safe here as WIDTH is statically verified to match m_data's size
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/uint256.h(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
🔇 Additional comments (1)
src/uint256.h (1)
55-58: LGTM! The workaround effectively addresses the GCC-11 warning.The explicit casting of data pointers to
const void*is a safe solution that maintains the original functionality while avoiding the compiler warning.
My 22.04 machine (gcc11) built 8b8e73c 👍 |
|
@UdjinM6, diffing upstream and our Weird that this workaround is needed since Bitcoin maintains that Edit: Bitcoin Core v28.1 successfully builds with GCC 11.4, seems like we're missing something in terms of build configuration. Build parameters:
|
|
CI still fails even with the workaround applied btw... |
This workaround let enable -Werror -Wall on our CI
In member function ‘constexpr int base_blob<BITS>::Compare(const base_blob<BITS>&) const [with unsigned int BITS = 256]’,
inlined from ‘constexpr bool operator!=(const base_blob<256>&, const base_blob<256>&)’ at ./uint256.h:58:96,
inlined from ‘bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock&, gsl::not_null<const CBlockIndex*>, BlockValidationState&, const CCoinsViewCache&, CDeterministicMNList&, llmq::CQuorumSnapshotManager&, bool)’ at evo/deterministicmns.cpp:818:119:
./uint256.h:55:77: error: ‘int __builtin_memcmp_eq(const void*, const void*, long unsigned int)’ specified bound 32 exceeds source size 0 [-Werror=stringop-overread]
55 | constexpr int Compare(const base_blob& other) const { return std::memcmp(m_data.data(), other.m_data.data(), WIDTH); }
| ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
What was done?
Backport bitcoin#23149 introduced more strict list of warnings that are considered as compilation error.
It caused several compilation errors, that has been addressed by this PR.
It has been split to own PR for easier testing / debugging from #6577
How Has This Been Tested?
It has been tested with clang 16.0.6 and gcc 13.2.0
No compilation errors.
Breaking Changes
N/A
Checklist: