-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: reduce debug level for rpc/evo.cpp to avoid clang crash #6991
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Workaround for LLVM 18.1.8 bug: reduce debug level for rpc/evo.cpp to avoid crash in debug info generation for complex std::function template
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThe pull request adds a targeted per-object CXXFLAGS override in the build configuration. When compiling with Clang, it sets Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3–5 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Makefile.am (1)
619-622: Per-objectCXXFLAGSoverride is correct; consider clarifying scope of the workaroundThe object name
rpc/libbitcoin_node_a-evo.$(OBJEXT)andCXXFLAGS += -g0usage look correct for targeting justrpc/evo.cpp, and appending-g0should reliably strip debug info for that one TU while leaving other flags intact. This is a minimal, well-scoped workaround.One thing to be aware of: this rule applies unconditionally, so all builds using this Makefile (not just Guix / Clang 18.1.8) will now compile
rpc/evo.cppwithout debug symbols. If that’s intentional, it might be worth making the comment explicit (e.g., mention that the workaround is applied globally and can be revisited once the upstream LLVM/Clang bug is fixed, maybe with a bug/issue reference).Please double‑check that:
- The generated object file name from
rpc/evo.cppactually matchesrpc/libbitcoin_node_a-evo.$(OBJEXT)in your build (e.g., by runningmake V=1and inspecting the compile line).- Applying
-g0unconditionally is acceptable for non-Guix, non-Clang-18 builds, or whether you’d prefer to gate this via a configure option or environment flag in a follow-up, to avoid scope creep here. Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Makefile.am(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: Lint / Run linters
|
@coderabbitai how do I only apply this fix when using Clang? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 445 🏁 Script executed: Length of output: 1644 🏁 Script executed: Length of output: 784 🏁 Script executed: Length of output: 2250 @UdjinM6, there are two main approaches to conditionally apply the Option 1: Detect in configure.ac (Recommended)Add compiler detection in AC_MSG_CHECKING([if compiler is clang])
AC_COMPILE_IFELSE([AC_LANG_SOURCE([[
#ifndef __clang__
#error "not clang"
#endif
]])], [is_clang=yes], [is_clang=no])
AC_MSG_RESULT([$is_clang])
AM_CONDITIONAL([IS_CLANG], [test x$is_clang = xyes])Then in # Workaround for LLVM 18.1.8 bug: reduce debug level for rpc/evo.cpp to avoid crash
# in debug info generation for complex std::function template
if IS_CLANG
rpc/libbitcoin_node_a-evo.$(OBJEXT): CXXFLAGS += -g0
endifAfter modifying configure.ac, you'll need to regenerate the build system with Option 2: Detect in Makefile.amAdd a check directly in # Workaround for LLVM 18.1.8 bug: reduce debug level for rpc/evo.cpp to avoid crash
# in debug info generation for complex std::function template
ifeq ($(shell $(CXX) -dM -E -x c++ /dev/null 2>/dev/null | grep -q __clang__ && echo yes),yes)
rpc/libbitcoin_node_a-evo.$(OBJEXT): CXXFLAGS += -g0
endifOption 1 is cleaner and more consistent with the existing autotools patterns in your codebase (I can see similar compiler detection in configure.ac). Option 2 is simpler if you want to avoid touching configure.ac, but runs the detection test every time make is invoked. Which approach would you prefer? 🧠 Learnings used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Makefile.am (1)
619-622: Effective workaround for the Clang compiler crash.The implementation correctly applies
-g0only when Clang is detected, using a per-object CXXFLAGS override that follows Make conventions. The shell-based detection is straightforward and addresses the immediate issue.Known tradeoffs:
- The shell command executes on every Make invocation (characteristic of the Makefile-based detection approach).
- Debug symbols are disabled for
rpc/evo.cppacross all Clang versions, not just 18.1.8. This is conservative but ensures the crash is avoided.Optional refinement: If minimizing the impact on other Clang versions is desired, you could add version-specific detection:
rpc/libbitcoin_node_a-evo.$(OBJEXT): CXXFLAGS += $(shell $(CXX) --version | grep -E 'clang version (18\.1\.[0-9]+|18\.1\.8)' -q && echo "-g0")However, the current broad approach is simpler and safer for a workaround.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Makefile.am(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_tsan-build / Build source
PastaPastaPasta
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strange; but utACK 387d5bf
|
#6988 doesn't have this issue it seems https://github.com/UdjinM6/dash/actions/runs/19511128262/job/55850873237 but is it ready? I think we could merge this and revert it in #6988. This way we could make sure we don't miss another clang build issue while waiting for #6988. |
|
Closing in fav of #6995 |
c359fb2 ci: bump to Clang 19 (Kittywhiskers Van Gogh) 9c9e876 partial bitcoin#33185: update time-machine to 5cb84f2013c5b1e48a7d0e617032266f1e6059e2 (Kittywhiskers Van Gogh) 7fef63d merge bitcoin#31529: latest 2.31 glibc (Kittywhiskers Van Gogh) de1b467 merge bitcoin#30730: Bump time machine to 53396a22afc04536ddf75d8f82ad2eafa5082725 (Kittywhiskers Van Gogh) Pull request description: ## Additional Information Spun-off from [dash#6998](#6988) to remedy CI failures. Potential alternative to [dash#6991](#6991) as Clang 19 does not seem to exhibit failures observed with Clang 18 ([comment](#6991 (comment))) caused by change in compile flags in [dash#6966](#6966). **Note:** ~~The base of this pull request does not use latest `develop` (86e84d7) due to build issues potentially related to [dash#6971](#6971) (see below) and uses a slightly out-of-date base to sidestep the issue. The build issue was found to be present independent of this PR using the Guix container.~~ Fixed with [dash#6997](#6997). <details> <summary>Build error:</summary> ``` make[2]: Nothing to be done for 'install-exec-am'. /home/ubuntu/.guix-profile/bin/mkdir -p '/distsrc-base/distsrc-23.0.0-rc.3-453-g86e84d701641-x86_64-linux-gnu/installed/dashcore-23.0.0-rc.3-453-g86e84d701641//lib/pkgconfig' /home/ubuntu/.guix-profile/bin/install -c -m 644 libdashconsensus.pc '/distsrc-base/distsrc-23.0.0-rc.3-453-g86e84d701641-x86_64-linux-gnu/installed/dashcore-23.0.0-rc.3-453-g86e84d701641//lib/pkgconfig' make[2]: Leaving directory '/distsrc-base/distsrc-23.0.0-rc.3-453-g86e84d701641-x86_64-linux-gnu' make[1]: Leaving directory '/distsrc-base/distsrc-23.0.0-rc.3-453-g86e84d701641-x86_64-linux-gnu' objcopy: Unable to recognise the format of the input file `dashcore-23.0.0-rc.3-453-g86e84d701641/bin/dash-tx.dbg' objcopy: Unable to recognise the format of the input file `dashcore-23.0.0-rc.3-453-g86e84d701641/lib/libdashconsensus.so.0.0.0.dbg' objcopy: Unable to recognise the format of the input file `dashcore-23.0.0-rc.3-453-g86e84d701641/bin/dash-cli.dbg' objcopy: Unable to recognise the format of the input file `dashcore-23.0.0-rc.3-453-g86e84d701641/bin/dash-wallet.dbg' objcopy: Unable to recognise the format of the input file `dashcore-23.0.0-rc.3-453-g86e84d701641/bin/dash-qt.dbg' objcopy: Unable to recognise the format of the input file `dashcore-23.0.0-rc.3-453-g86e84d701641/bin/test_dash.dbg' objcopy: Unable to recognise the format of the input file `dashcore-23.0.0-rc.3-453-g86e84d701641/bin/dashd.dbg' ``` </details> ## 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 - [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 c359fb2 Tree-SHA512: 69576f6e6c0c101be434b273e9a84d1ab9f9341734241e794585615f194b453d2983dce077f9a2efa04b93bfba2374274aad0f90a1a07aed4ef87ff3f22ddd59
Issue being fixed or feature implemented
The LLVM 18.1.8 compiler is crashing when generating debug information for rpc/evo.cpp:
https://github.com/dashpay/dash/actions/runs/19477052188/job/55739582899
https://github.com/UdjinM6/dash/actions/runs/19470819712/job/55717447032
2699740 from #6966 triggered a bug in clang - no crash after reverting it:
https://github.com/UdjinM6/dash/actions/runs/19495040944
What was done?
Implement a workaround.
How Has This Been Tested?
https://github.com/UdjinM6/dash/actions/runs/19495218479
Breaking Changes
Not exactly breaking but the tradeoff is that rpc/evo.cpp won't have debug symbols which is acceptable imo.
Checklist: