Skip to content

Conversation

@PastaPastaPasta
Copy link
Member

Issue being fixed or feature implemented

Sometimes we see "stack traces" like;

2025-11-14T14:49:05Z Posix Signal: Segmentation fault
   0#: (0xAF72A4F30718) <unknown-file> - ???
   1#: (0xF19AEAC3F8F8) <unknown-file> - ???
   2#: (0xF19AEAA4BA94) <unknown-file> - ???
   3#: (0xAF72A4F8CCE8) <unknown-file> - ???
   4#: (0xAF72A49C8FA4) <unknown-file> - ???
   5#: (0xAF72A460F7E0) <unknown-file> - ???
   6#: (0xF19AEA997400) <unknown-file> - ???
   7#: (0xF19AEA9974D8) <unknown-file> - ???
   8#: (0xAF72A462A410) <unknown-file> - ???
2025-11-14T14:49:05Z Posix Signal: Trace/breakpoint trap
   0#: (0xAF72A4F30718) <unknown-file> - ???
   1#: (0xF19AEAC3F8F8) <unknown-file> - ???
   2#: (0xF19AEA9971F0) <unknown-file> - ???
   3#: (0xAF72A4F30880) <unknown-file> - ???
   4#: (0xF19AEAC3F8F8) <unknown-file> - ???
   5#: (0xF19AEAA4BA94) <unknown-file> - ???
   6#: (0xAF72A4F8CCE8) <unknown-file> - ???
   7#: (0xAF72A49C8FA4) <unknown-file> - ???
   8#: (0xAF72A460F7E0) <unknown-file> - ???
   9#: (0xF19AEA997400) <unknown-file> - ???
  10#: (0xF19AEA9974D8) <unknown-file> - ???
  11#: (0xAF72A462A410) <unknown-file> - ???
Posix Signal: Segmentation fault
   0#: (0xAF72A4F30718) <unknown-file> - ???
   1#: (0xF19AEAC3F8F8) <unknown-file> - ???
   2#: (0xF19AEAA4BA94) <unknown-file> - ???
   3#: (0xAF72A4F8CCE8) <unknown-file> - ???
   4#: (0xAF72A49C8FA4) <unknown-file> - ???
   5#: (0xAF72A460F7E0) <unknown-file> - ???
   6#: (0xF19AEA997400) <unknown-file> - ???
   7#: (0xF19AEA9974D8) <unknown-file> - ???
   8#: (0xAF72A462A410) <unknown-file> - ???
Posix Signal: Trace/breakpoint trap
   0#: (0xAF72A4F30718) <unknown-file> - ???
   1#: (0xF19AEAC3F8F8) <unknown-file> - ???
   2#: (0xF19AEA9971F0) <unknown-file> - ???
   3#: (0xAF72A4F30880) <unknown-file> - ???
   4#: (0xF19AEAC3F8F8) <unknown-file> - ???
   5#: (0xF19AEAA4BA94) <unknown-file> - ???
   6#: (0xAF72A4F8CCE8) <unknown-file> - ???
   7#: (0xAF72A49C8FA4) <unknown-file> - ???
   8#: (0xAF72A460F7E0) <unknown-file> - ???
   9#: (0xF19AEA997400) <unknown-file> - ???
  10#: (0xF19AEA9974D8) <unknown-file> - ???
  11#: (0xAF72A462A410) <unknown-file> - ???

This isn't helpful; if we have no useful info; let's print the compact format

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

  • 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)

@PastaPastaPasta PastaPastaPasta added this to the 23.1 milestone Nov 14, 2025
@coderabbitai
Copy link

coderabbitai bot commented Nov 14, 2025

Walkthrough

The change updates GetCrashInfoStr in src/stacktraces.cpp to use a stricter check for useful debug data. It computes hasUsefulInfo by scanning stackframeInfos for any frame with a non-empty filename or function. If no useful info is found, GetCrashInfoStrNoDebugInfo(ci) is used; if at least one frame has data, the existing debug-output path is retained. No public signatures were changed.

Sequence Diagram(s)

sequenceDiagram
    participant Caller
    participant GetCrashInfoStr
    participant hasUsefulInfo
    participant GCS_Debug as GetCrashInfoStr (debug)
    participant GCS_NoDebug as GetCrashInfoStrNoDebugInfo

    Caller->>GetCrashInfoStr: call with CrashInfo
    GetCrashInfoStr->>hasUsefulInfo: scan stackframeInfos (filename|function non-empty)
    alt hasUsefulInfo == true
        hasUsefulInfo-->>GetCrashInfoStr: true
        GetCrashInfoStr->>GCS_Debug: generate debug info string
        GCS_Debug-->>Caller: return debug string
    else hasUsefulInfo == false
        hasUsefulInfo-->>GetCrashInfoStr: false
        GetCrashInfoStr->>GCS_NoDebug: generate no-debug string
        GCS_NoDebug-->>Caller: return non-debug string
    end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Single-file change concentrated in one function
  • Verify the hasUsefulInfo predicate correctly detects non-empty filename/function fields
  • Confirm behavior unchanged when at least one frame contains useful info and that fallback triggers when all frames are empty

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The PR title clearly and accurately summarizes the main change: replacing verbose, unhelpful stack traces with compact ones when debug info is empty or not useful.
Description check ✅ Passed The PR description is related to the changeset, providing context about the problem (unhelpful verbose stack traces with only addresses and unknown files) and the intent to use compact format when debug info is not useful.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c760e44 and c4971dc.

📒 Files selected for processing (1)
  • src/stacktraces.cpp (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
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.
📚 Learning: 2025-11-14T23:17:08.477Z
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6976
File: src/stacktraces.cpp:475-482
Timestamp: 2025-11-14T23:17:08.477Z
Learning: In C++ code reviews, always check for opportunities to use standard algorithms (std::any_of, std::all_of, std::none_of, std::find_if, std::transform, std::for_each, etc.) instead of manual for-loops, especially when the loop body performs simple checks, transformations, or accumulations.

Applied to files:

  • src/stacktraces.cpp
⏰ 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_nowallet-build / Build source
  • GitHub Check: arm-linux-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: linux64_fuzz-build / Build source
  • GitHub Check: mac-build / Build source
  • GitHub Check: linux64_tsan-build / Build source
  • GitHub Check: win64-build / Build source
  • GitHub Check: Lint / Run linters
🔇 Additional comments (1)
src/stacktraces.cpp (1)

472-480: LGTM! Previous feedback correctly implemented.

The use of std::any_of properly addresses the concern from the previous review and aligns with the PR objective to detect when debug information is not useful. The logic correctly handles all edge cases:

  • Empty stackframeInfos → compact format
  • All frames with empty filename and function → compact format
  • At least one frame with data → detailed format

The explanatory comments clearly describe the rationale for this stricter check.

Based on learnings


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/stacktraces.cpp (1)

472-486: LGTM! Consider using std::any_of for more idiomatic C++.

The logic correctly addresses the PR objective by detecting when debug information is present but not useful (all frames have empty filename and function). The IIFE lambda pattern works, but could be replaced with a more conventional approach.

Apply this diff for a more idiomatic implementation:

-    // Check if we have any useful debug information at all
-    // libbacktrace may return stackframe_info entries but with empty filenames and functions
-    // when it can find the binary but can't resolve symbols
-    bool hasUsefulInfo = [&ci]() {
-        for (const auto& si : ci.stackframeInfos) {
-            if (!si.filename.empty() || !si.function.empty()) {
-                return true;
-            }
-        }
-        return false;
-    }();
+    // Check if we have any useful debug information at all
+    // libbacktrace may return stackframe_info entries but with empty filenames and functions
+    // when it can find the binary but can't resolve symbols
+    bool hasUsefulInfo = std::any_of(ci.stackframeInfos.begin(), ci.stackframeInfos.end(),
+        [](const auto& si) { return !si.filename.empty() || !si.function.empty(); });
 
     if (!hasUsefulInfo) {
         return GetCrashInfoStrNoDebugInfo(ci);
     }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d3fa315 and c760e44.

📒 Files selected for processing (1)
  • src/stacktraces.cpp (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
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.

@github-actions
Copy link

github-actions bot commented Nov 14, 2025

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

Co-authored-by: UdjinM6 <[email protected]>
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK c4971dc

@PastaPastaPasta PastaPastaPasta requested review from knst and kwvg November 14, 2025 23:23
@knst
Copy link
Collaborator

knst commented Nov 15, 2025

This isn't helpful; if we have no useful info; let's print the compact format

How compact format looks? can you post an example?

Also; addresses are quite useful, aren't they?

Copy link
Collaborator

@kwvg kwvg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK c4971dc

@PastaPastaPasta
Copy link
Member Author

@knst it looks like:

> $ ./src/dashd -printcrashinfo=bvcgc43iinzgc43ijfxgm3ybaacwiyltnbsciudponuxqictnftw4ylmhiqfgzlhnvsw45dboruw63ramzqxk3duhiqdcmigobpolex777777zhnkurqaaaaad6h52ms777777zubs2jf7777775ayk5sl777777osbbuiyaaaaaaaa=
Posix Signal: Segmentation fault: 11
No debug information available for stacktrace. You should add debug information and then run:
dashd -printcrashinfo=bvcgc43iinzgc43ijfxgm3ybaacwiyltnbsciudponuxqictnftw4ylmhiqfgzlhnvsw45dboruw63ramzqxk3duhiqdcmigobpgmt7777777zhn23p77777776h42sp777777zubq2u77777775ayo6j3777777osbjxx7777776aa=

Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK c4971dc

@PastaPastaPasta PastaPastaPasta merged commit caf5010 into dashpay:develop Nov 15, 2025
53 of 65 checks passed
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.

4 participants