Skip to content

Conversation

@UdjinM6
Copy link

@UdjinM6 UdjinM6 commented Nov 19, 2025

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:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • 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)

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
@UdjinM6 UdjinM6 added this to the 23.1 milestone Nov 19, 2025
@github-actions
Copy link

github-actions bot commented Nov 19, 2025

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

@coderabbitai
Copy link

coderabbitai bot commented Nov 19, 2025

Walkthrough

The pull request adds a targeted per-object CXXFLAGS override in the build configuration. When compiling with Clang, it sets -g0 for the rpc/evo.cpp object (rpc/libbitcoin_node_a-evo.$(OBJEXT)) to avoid an LLVM 18.1.8 debug-info generation crash. The change applies only to that single object and does not modify other build targets or exported/public declarations.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3–5 minutes

  • Inspect src/Makefile.am to confirm the object target name rpc/libbitcoin_node_a-evo.$(OBJEXT) matches the actual generated object for rpc/evo.cpp.
  • Verify the conditional for Clang is correct and that -g0 is the intended flag.
  • Check for Makefile syntax correctness and ensure no unintended side effects on other targets.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: reducing debug level for rpc/evo.cpp to avoid a clang crash, which matches the core objective of the PR.
Description check ✅ Passed The description is directly related to the changeset, explaining the compiler crash issue, the specific problem triggered by a previous commit, the workaround implemented, and testing evidence.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a 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-object CXXFLAGS override is correct; consider clarifying scope of the workaround

The object name rpc/libbitcoin_node_a-evo.$(OBJEXT) and CXXFLAGS += -g0 usage look correct for targeting just rpc/evo.cpp, and appending -g0 should 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.cpp without 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.cpp actually matches rpc/libbitcoin_node_a-evo.$(OBJEXT) in your build (e.g., by running make V=1 and inspecting the compile line).
  • Applying -g0 unconditionally 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

📥 Commits

Reviewing files that changed from the base of the PR and between 97bd9fa and 2227859.

📒 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

@UdjinM6 UdjinM6 changed the title fix: reduce debug level for rpc/evo.cpp to avoid clang crash in guix fix: reduce debug level for rpc/evo.cpp to avoid clang crash Nov 19, 2025
@UdjinM6
Copy link
Author

UdjinM6 commented Nov 19, 2025

@coderabbitai how do I only apply this fix when using Clang?

@coderabbitai
Copy link

coderabbitai bot commented Nov 19, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check the current change in src/Makefile.am
cat src/Makefile.am | grep -A5 -B5 "rpc/libbitcoin_node_a-evo"

Length of output: 445


🏁 Script executed:

#!/bin/bash
# Look for existing compiler detection patterns in configure.ac or Makefile files
rg -n "CLANG|clang\+\+|__clang__|CXX.*clang" configure.ac src/Makefile.am build-aux/ || echo "No existing Clang detection found"

Length of output: 1644


🏁 Script executed:

#!/bin/bash
# Check if there are any existing conditional CXXFLAGS based on compiler
rg -n "if.*CXX|ifeq.*CXX|ifneq.*compiler" src/Makefile.am Makefile.am configure.ac | head -20

Length of output: 784


🏁 Script executed:

#!/bin/bash
# Look for how the build system detects or handles different compilers
cat configure.ac | grep -i "compiler\|clang\|gcc" | head -30

Length of output: 2250


@UdjinM6, there are two main approaches to conditionally apply the -g0 flag only when using Clang:

Option 1: Detect in configure.ac (Recommended)

Add compiler detection in configure.ac:

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 src/Makefile.am, wrap the workaround:

# 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
endif

After modifying configure.ac, you'll need to regenerate the build system with ./autogen.sh.

Option 2: Detect in Makefile.am

Add a check directly in src/Makefile.am before the per-object rule:

# 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
endif

Option 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
Learnt from: kwvg
Repo: dashpay/dash PR: 6529
File: src/rpc/governance.cpp:1074-1089
Timestamp: 2025-02-14T15:15:58.165Z
Learning: Code blocks marked with `// clang-format off` and `// clang-format on` directives should be excluded from clang-format suggestions as they are intentionally formatted manually for better readability.

Learnt from: kwvg
Repo: dashpay/dash PR: 6729
File: src/rpc/evo.cpp:1273-1273
Timestamp: 2025-07-09T15:05:36.250Z
Learning: When clang-format suggestions significantly harm readability (like splitting logical parameter groups across multiple lines), it's acceptable to use `// clang-format off` and `// clang-format on` directives to exclude the problematic section from automatic formatting, prioritizing code readability over strict line length compliance.

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.

Copy link

@coderabbitai coderabbitai bot left a 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 -g0 only 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.cpp across 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

📥 Commits

Reviewing files that changed from the base of the PR and between 2227859 and 387d5bf.

📒 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

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a 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

@kwvg
Copy link
Collaborator

kwvg commented Nov 19, 2025

@UdjinM6 can you try #6988? It bumps to Clang/LLVM 19, if it resolves the crash, we should be able to avoid the workaround.

@UdjinM6
Copy link
Author

UdjinM6 commented Nov 19, 2025

#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.

@kwvg
Copy link
Collaborator

kwvg commented Nov 20, 2025

@UdjinM6 the Clang 19 bump has been extracted to #6995, will post Guix build hashes soon

@kwvg kwvg mentioned this pull request Nov 20, 2025
5 tasks
@UdjinM6 UdjinM6 removed this from the 23.1 milestone Nov 21, 2025
@UdjinM6
Copy link
Author

UdjinM6 commented Nov 21, 2025

Closing in fav of #6995

@UdjinM6 UdjinM6 closed this Nov 21, 2025
PastaPastaPasta added a commit that referenced this pull request Nov 21, 2025
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants