2 fixes for rt.backtrace.dwarf #2151

kinke · 2018-03-25T15:44:46Z

No description provided.

kinke · 2018-03-25T15:47:11Z

src/rt/backtrace/dwarf.d

+        {
+            buffer[0 .. 5] = "??:? ";
+            bufferLength = 5;
+        }


Fix 1:

addressBuffer[] = "??:? \0";

=> object.Error@(0): Array lengths don't match for copy https://run.dlang.io/is/4RQZWh

Any chance we can add a test for this to prevent future regressions?

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

@wilzbach: There's no debug druntime (i.e., compiled with enabled bounds checks) available for CI, right? Then it's untestable.

kinke · 2018-03-25T15:48:22Z

src/rt/backtrace/dwarf.d

+        static if (size_t.sizeof == 8)
+            appendToBuffer("[0x%llx]", callstack[i]);
+        else
+            appendToBuffer("[0x%x]", callstack[i]);


Fix 2: don't restrict to lower 32 bits of address on 64-bit platforms, e.g., now ??:? __libc_start_main [0x7f7bb141182f].

JinShil · 2018-03-25T15:53:30Z

Does this fix https://issues.dlang.org/show_bug.cgi?id=18068?

kinke · 2018-03-25T15:59:04Z

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.

dlang-bot · 2018-03-25T17:49:13Z

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 verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

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 references

Your 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 locally

If 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"

kinke · 2018-05-01T15:13:30Z

I extended the previous trivial fix by a revised version of rt.backtrace.dwarf, tackling:

Support for DWARF v4.
Support for 64-bit DWARF.
Alignment issues on non-x86 architectures (DWARF data is packed).
Issue #18068; DSO-relative DWARF addresses vs. absolute addresses returned by backtrace().

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.

wilzbach · 2018-06-28T08:24:30Z

With #2230 merged at least the getMemoryRegion part should become obsolete.

kinke · 2018-06-28T21:48:51Z

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.

kinke · 2019-04-14T23:30:43Z

I restricted this to the first trivial fixes again and moved the 2nd commit to #2559. @thewilsonator: Please push.

kinke requested review from andralex and wilzbach as code owners March 25, 2018 15:44

kinke commented Mar 25, 2018

View reviewed changes

kinke mentioned this pull request Mar 25, 2018

Add support for LLD 6.0.0 & switch CI to LDC-LLVM 6.0.0 ldc-developers/ldc#2608

Merged

kinke force-pushed the bt_dwarf branch from e84da34 to d6a3418 Compare March 25, 2018 17:49

kinke mentioned this pull request Mar 25, 2018

Fix rt.backtrace.dwarf ldc-developers/druntime#124

Merged

kinke closed this in ldc-developers/druntime#124 Apr 29, 2018

kinke reopened this May 1, 2018

dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label May 1, 2018

kinke changed the title ~~Fix rt.backtrace.dwarf~~ Revise & fix rt.backtrace.dwarf (DWARF v4, 64-bit DWARF, DSO-relative addresses) May 1, 2018

kinke mentioned this pull request May 1, 2018

druntime: Fix rt.backtrace.dwarf ldc-developers/ldc#2677

Merged

kinke force-pushed the bt_dwarf branch from 14ba7e8 to d109d53 Compare May 3, 2018 18:32

kinke mentioned this pull request May 3, 2018

Fix Issue 18068 - No file names and line numbers in stack trace #2172

Closed

kinke force-pushed the bt_dwarf branch from d109d53 to 0a7955b Compare May 14, 2018 22:22

ibuclaw mentioned this pull request May 19, 2018

change size_t to uint for OSX 64 bit dlang/dmd#8249

Merged

kinke added a commit to ldc-developers/druntime that referenced this pull request Jun 18, 2018

Fix rt.backtrace merge regressions

c74faa4

Basically applying dlang#2151.

kinke added a commit to ldc-developers/druntime that referenced this pull request Jun 18, 2018

Fix rt.backtrace merge regressions

a382ade

Basically applying dlang#2151.

kinke force-pushed the bt_dwarf branch from 0a7955b to 002c3c0 Compare June 28, 2018 21:46

kinke changed the title ~~Revise & fix rt.backtrace.dwarf (DWARF v4, 64-bit DWARF, DSO-relative addresses)~~ Revise & fix rt.backtrace.dwarf (DWARF v4, 64-bit DWARF) Jul 11, 2018

kinke force-pushed the bt_dwarf branch from 002c3c0 to 7565530 Compare April 14, 2019 23:27

kinke changed the title ~~Revise & fix rt.backtrace.dwarf (DWARF v4, 64-bit DWARF)~~ 2 fixes for rt.backtrace.dwarf Apr 14, 2019

kinke mentioned this pull request Apr 14, 2019

rt.backtrace.dwarf: Add support for DWARF v4 & 64-bit DWARF and fix non-x86 alignment issues #2559

Merged

thewilsonator approved these changes Apr 15, 2019

View reviewed changes

thewilsonator merged commit 50d8d09 into dlang:master Apr 15, 2019

kinke deleted the bt_dwarf branch April 15, 2019 21:23

Uh oh!

Uh oh!

2 fixes for rt.backtrace.dwarf #2151

2 fixes for rt.backtrace.dwarf #2151

Uh oh!

Conversation

kinke commented Mar 25, 2018

Uh oh!

kinke Mar 25, 2018

Choose a reason for hiding this comment

Uh oh!

wilzbach Mar 25, 2018

Choose a reason for hiding this comment

Uh oh!

kinke Mar 25, 2018

Choose a reason for hiding this comment

Uh oh!

kinke Apr 3, 2018

Choose a reason for hiding this comment

Uh oh!

kinke Mar 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JinShil commented Mar 25, 2018

Uh oh!

kinke commented Mar 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Mar 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Testing this PR locally

Uh oh!

kinke commented May 1, 2018

Uh oh!

wilzbach commented Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kinke commented Jun 28, 2018

Uh oh!

kinke commented Apr 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kinke Mar 25, 2018 •

edited

Loading

kinke commented Mar 25, 2018 •

edited

Loading

dlang-bot commented Mar 25, 2018 •

edited

Loading

wilzbach commented Jun 28, 2018 •

edited

Loading