Conversation
{ | ||
buffer[0 .. 5] = "??:? "; | ||
bufferLength = 5; | ||
} |
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.
Fix 1:
addressBuffer[] = "??:? \0";
=> object.Error@(0): Array lengths don't match for copy
https://run.dlang.io/is/4RQZWh
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.
Any chance we can add a test for this to prevent future regressions?
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.
It's pretty bad with -release
, causing a segfault as long as the destination buffer is sufficiently large: https://run.dlang.io/is/NEqZOB. Without -release
an exception is thrown during stacktrace generation, which is also pretty bad.
The latter is how I stumbled on this. LLVM 6.0 led to DWARF v4 debuginfos, rt.backtrace.dwarf doesn't support that version, so no file/line infos => "??:? \0" + exception (debug druntime only; release druntime compiled with -release
happily reads beyond the string constant, causing no segfault on x86 apparently). The exception with debug-druntime is swallowed in my test case, it only manifests itself in an incomplete stack trace (single entry).
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.
@wilzbach: There's no debug druntime (i.e., compiled with enabled bounds checks) available for CI, right? Then it's untestable.
static if (size_t.sizeof == 8) | ||
appendToBuffer("[0x%llx]", callstack[i]); | ||
else | ||
appendToBuffer("[0x%x]", callstack[i]); |
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.
Fix 2: don't restrict to lower 32 bits of address on 64-bit platforms, e.g., now ??:? __libc_start_main [0x7f7bb141182f]
.
Does this fix https://issues.dlang.org/show_bug.cgi?id=18068? |
Nope, but I discovered bug no. 1 because of missing file/line infos too, or rather because the code here doesn't handle Dwarf v4+ debuginfos. |
Thanks for your pull request and interest in making D better, @kinke! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "master + druntime#2151" |
I extended the previous trivial fix by a revised version of
Tests are missing, I'm unsure how to tackle that in a robust fashion (for DMD, I'd know how for LDC ;)). |
Basically applying dlang#2151.
Basically applying dlang#2151.
With #2230 merged at least the |
Rebased and obsolete 18068 fix removed. FWIW, the first commit is part of LDC 1.9, the second in 1.10. |
Fix 1: mismatching array lengths for a copy (the `??:? \0` piece). Luckily, these bounds checks are disabled in non-debug druntime, so that people haven't seen this bug (in backtrace-printing code!) that often. Fix 2: print the full 64-bit address on 64-bit systems, not just the 32 lower bits. That's one part of Issue 19653.
I restricted this to the first trivial fixes again and moved the 2nd commit to #2559. @thewilsonator: Please push. |
No description provided.