fix: avoid maybe-uninitialized warning in ScopedBLSLegacyScheme - #7586
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
✅ Final review complete — no blockers (commit 0d092f5) |
PastaPastaPasta
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1df5f8a9d9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # QDBusConnectionManager lazily spawns a watcher QThread that is never joined | ||
| # before process exit; the manager itself is suppressed above but the thread | ||
| # object's allocation carries only the QThread constructor frame. | ||
| leak:QThread::QThread |
There was a problem hiding this comment.
When the ASan/LSan CI runs Qt tests, this substring suppresses every leaked allocation whose stack contains QThread::QThread, not only the QDBus watcher described above it. Application code constructs its own QThreads in paths such as src/qt/intro.cpp, clientfeeds.cpp, clientmodel.cpp, and walletcontroller.cpp, so leaks in their constructor-owned storage can now pass undetected; fix the watcher lifecycle or limit the suppression to the affected test rather than disabling generic QThread leak coverage globally. This is also unrelated to the BLS warning fix and should not be mixed into that narrowly described change.
AGENTS.md reference: AGENTS.md:L13-L14
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The ScopedBLSLegacyScheme constructor split preserves the behavior of all four call sites while removing the optional dereference implicated in the GCC warning. The LSan addition addresses the reported Qt failure, but its generic QThread match weakens leak detection across sanitizer processes and should be scoped more narrowly.
Source: reviewer backend model: gpt-5.6-sol; final verifier backend model: gpt-5.6-sol; orchestration only (not reviewer evidence): openclaw-agent/cliproxy/gpt-5.6-sol.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `test/sanitizer_suppressions/lsan`:
- [SUGGESTION] test/sanitizer_suppressions/lsan:9: Avoid suppressing leaks from every QThread constructor
LeakSanitizer suppression templates match any allocation stack containing the named symbol. Because both CI configurations load this suppression file through the common `LSAN_OPTIONS`, `leak:QThread::QThread` is not limited to the QDBus watcher: it also suppresses constructor-owned allocations for application QThreads created by `ClientModel`, `ClientFeeds`, `Intro`, `WalletController`, and other Qt paths. This can hide leaks whose only identifying allocation frame is the generic constructor, just as described for the current 120-byte leak. Restrict the rule to the affected test or process, obtain a QDBus-specific frame with better unwinding, or fix the watcher lifecycle instead of applying this generic symbol suppression throughout sanitizer testing.
| # QDBusConnectionManager lazily spawns a watcher QThread that is never joined | ||
| # before process exit; the manager itself is suppressed above but the thread | ||
| # object's allocation carries only the QThread constructor frame. | ||
| leak:QThread::QThread |
There was a problem hiding this comment.
🟡 Suggestion: Avoid suppressing leaks from every QThread constructor
LeakSanitizer suppression templates match any allocation stack containing the named symbol. Because both CI configurations load this suppression file through the common LSAN_OPTIONS, leak:QThread::QThread is not limited to the QDBus watcher: it also suppresses constructor-owned allocations for application QThreads created by ClientModel, ClientFeeds, Intro, WalletController, and other Qt paths. This can hide leaks whose only identifying allocation frame is the generic constructor, just as described for the current 120-byte leak. Restrict the rule to the affected test or process, obtain a QDBus-specific frame with better unwinding, or fix the watcher lifecycle instead of applying this generic symbol suppression throughout sanitizer testing.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Avoid suppressing leaks from every QThread constructor no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
knst
left a comment
knst
left a comment
There was a problem hiding this comment.
Changes in ScopedBLSLEgacyScheme looks legit.
For QThread I want pasta-claw's response first.
| //! pre-V19 blocks under an active post-V19 snapshot must set it on entry | ||
| //! (ProcessSpecialTxsInBlock only ever switches legacy->basic) and must not | ||
| //! commit it back to the active chainstate's consumers. | ||
| //! The `enter` constructor establishes the scheme for the scope. The flag is |
There was a problem hiding this comment.
nit: Comment should not be changed, because behaviour is not changed. Revert it please
There was a problem hiding this comment.
Done in 0d092f5 — comment wording restored verbatim; only the constructor split remains. The LSan/QThread suppression has also been dropped from this PR entirely and moved to a separate PR so this one is just the build fix.
🤖 Posted autonomously by Claude on behalf of pasta.
| bls::bls_legacy_scheme.store(*enter); | ||
| LogPrintf("ScopedBLSLegacyScheme: entered bls_legacy_scheme=%d\n", *enter); | ||
| } | ||
| explicit ScopedBLSLegacyScheme(bool enter) noexcept : |
There was a problem hiding this comment.
NOTE FOR REVIEWERS:
that is silly fix but it is required because false-alarm in gcc
See relevant changes in mainstream Bitcoin Core:
commit 5294f0d5a94cc7beaf692131fba0cad8beec9f13
Author: fanquake <[email protected]>
Date: Mon Mar 22 11:22:06 2021 +0800
refactor: return std::nullopt instead of {}
In #21415 we decided to return `std::optional` rather than `{}` for
uninitialized values. This PR repalces the two remaining usages of `{}`
with `std::nullopt`.
As a side-effect, this also quells the spurious GCC 10.2.x warning that
we've had reported quite a few times. i.e #21318, #21248, #20797.
```bash
txmempool.cpp: In member function 'CTxMemPool::setEntries CTxMemPool::GetIterSet(const std::set<uint256>&) const':
txmempool.cpp:898:13: warning: '<anonymous>' may be used uninitialized in this function [-Wmaybe-uninitialized]
898 | return {};
| ^
It's false alarm in gcc, so, std::optional should not be default initialized to avoid warnings.
| # QDBusConnectionManager lazily spawns a watcher QThread that is never joined | ||
| # before process exit; the manager itself is suppressed above but the thread | ||
| # object's allocation carries only the QThread constructor frame. |
There was a problem hiding this comment.
IMO explanation in PR body is enough.
@thepastaclaw please figure out why exactly Dash Core does need this exception but Bitcoin Core never had it?
There was a problem hiding this comment.
do bitcoin#35937 instead of QThread - seems as leaks are forever-friends for Qt Apps
GCC 14 at -O2 (the nowallet CI job) rejects the std::optional<bool> constructor parameter with -Werror=maybe-uninitialized when the guard is default-constructed: the inliner loses track of the has_value() check guarding the payload read. Replace the optional parameter with a default constructor and an explicit bool constructor; both existing value-passing call sites already pass a plain bool, so no caller changes.
1df5f8a to
0d092f5
Compare
PastaPastaPasta
changed the title
|
most builds look happy here: https://github.com/PastaPastaPasta/dash/actions/runs/31641899026/job/94266577757 |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The constructor split preserves the behavior of all four ScopedBLSLegacyScheme call sites while removing the optional payload access that triggered GCC's maybe-uninitialized warning. The prior generic QThread LeakSanitizer suppression was removed from the current PR, and no in-scope defects remain.
Source: reviewer backend model: gpt-5.6-sol; final verifier backend model: gpt-5.6-sol; orchestration-only (not reviewer evidence): openclaw-agent/cliproxy/gpt-5.6-sol.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
…nder -fsanitize=integer 8b200cd fix: make the wrapped successor explicit in CRangesSet under -fsanitize=integer (pasta) Pull request description: ## Issue being fixed or feature implemented **develop's `linux64_asan` job is red since #7587 merged.** The boundary tests merged there exercise `Add(UINT64_MAX)`, whose half-open end `value + 1` intentionally wraps to `0` — the representation the rest of #7587 teaches the class to understand. The asan job builds with `-fsanitize=integer`, which reports the intentional wrap as `unsigned integer overflow: 18446744073709551615 + 1` at `util/ranges_set.cpp:27` and fails `make check`. My verification of #7587 ran the full unit suite but not under sanitizers, which is exactly the gap this slipped through; apologies for the breakage. ## What was done? Spelled the successor as an explicit branch — a file-local `WrappedSuccessor(value)` (`value == UINT64_MAX ? 0 : value + 1`) — at the three arithmetic sites in `Add()`/`Remove()`. This states the wrap as intent instead of overflow, which is preferable to a sanitizer suppression here: unlike the quorum-snapshot skip-list encoding (suppressed by symbol in eacd9e0 because its wraparound is consensus wire format), this is a private in-memory representation that can simply be written unambiguously. No behavior change: for every `value != UINT64_MAX` the expression is `value + 1` as before, and for `UINT64_MAX` it produces the same `0` the wrap produced. ## How Has This Been Tested? Built with `--with-sanitizers=undefined,integer` (the failing job's relevant checks): `util_tests/test_CRanges` reproduces the exact CI failure without the fix and passes with it, using the repo's ubsan suppressions file. Full unit suite green on a regular `--enable-werror` build, rebased on current develop (the `linux64_nowallet` failure visible on this branch's earlier CI was the pre-existing `ScopedBLSLegacyScheme` gcc-14 warning, fixed independently by #7586). ## Breaking Changes None. ## 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 - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ Top commit has no ACKs. Tree-SHA512: c1810c61e8f7a9dc98023c11c86f88740c4cb8941d8e8f4152e21a8ddf8acf63711004156de04f0f88d2d3ccedd10f2c30caa98e2de5ae4a161a3368569a8e02
3 participants
Issue being fixed or feature implemented
developis currently red: thelinux64_nowalletbuild fails withGCC 14 at
-O2(the nowallet job's compiler) loses track of thehas_value()check guarding thestd::optional<bool>payload read inScopedBLSLegacyScheme's constructor when the guard is default-constructed fromConnectTip. Introduced by 311efbc; every PR based on the current tip inherits the failure.What was done?
Replaced the
std::optional<bool>constructor parameter with a default constructor and an explicitboolconstructor. Both existing value-passing call sites already pass a plainbool(!DeploymentActiveAt(...)), so no caller changes; the optional deref the warning pointed at no longer exists. No behavior change.The asan
test_dash-qtQThread leak that was briefly bundled here has been split into its own PR per review feedback, so this PR is only the build fix.How Has This Been Tested?
test_dash --run_test=evo_dip3_activation_testsandbls_testspass.-Werrorin thelinux64_nowalletCI job — this PR's CI run is the authoritative check.Breaking Changes
None.
Checklist: