Skip to content
This repository was archived by the owner on Feb 8, 2024. It is now read-only.

Integrate backtrace refactoring and fixes for libunwind#192

Merged
kinke merged 11 commits intoldc-developers:ldcfrom
Geod24:integrate-backtrace-fixes
Jan 16, 2021
Merged

Integrate backtrace refactoring and fixes for libunwind#192
kinke merged 11 commits intoldc-developers:ldcfrom
Geod24:integrate-backtrace-fixes

Conversation

This makes it reusable by other backtrace providers,
and just generally makes the code easier to follow.
So that it can be called from another overload of `traceHandlerOpApplyImpl`.
The execinfo functionality really only concerns itself with backtrace,
which provide addresses and function names.
However, the general ability to iterate over locations and fill them
with file/line informations, as well as the utilities tied to it
(e.g. TraceInfoBuffer) can be used by other implementations.
@kinke
Copy link
Copy Markdown
Member

kinke commented Jan 10, 2021

Thanks! Had to follow-up with a little fixup:

diff --git a/src/core/internal/backtrace/dwarf.d b/src/core/internal/backtrace/dwarf.d
index 76d5b4d3..4775c61b 100644
--- a/src/core/internal/backtrace/dwarf.d
+++ b/src/core/internal/backtrace/dwarf.d
@@ -52,6 +52,13 @@ module core.internal.backtrace.dwarf;
 import core.internal.execinfo;
 import core.internal.string;

+version (DRuntime_Use_Libunwind)
+    private enum hasLibunwind = true;
+else
+    private enum hasLibunwind = false;
+
+static if (hasExecinfo || hasLibunwind):
+
 version (OSX)
     version = Darwin;
 else version (iOS)
@@ -165,6 +172,9 @@ static if (hasExecinfo)
         import core.stdc.stdio : snprintf;
         import core.sys.posix.stdlib : free;

+        const char** frameList = backtrace_symbols(callstack.ptr, cast(int) callstack.length);
+        scope(exit) free(cast(void*) frameList);
+
         auto image = Image.openSelf();

         // find address -> file, line mapping using dwarf debug_line
diff --git a/src/core/internal/backtrace/libunwind.d b/src/core/internal/backtrace/libunwind.d
index fff25bb1..b0fd6ec2 100644
--- a/src/core/internal/backtrace/libunwind.d
+++ b/src/core/internal/backtrace/libunwind.d
@@ -181,6 +181,7 @@ else version (AArch64)

         version (linux)
         {
+            import core.stdc.config : c_ulong;
             import core.sys.posix.signal : sigset_t, stack_t;

             // libunwind has some special tweaking to reduce
@@ -226,6 +227,8 @@ else version (ARM)
     }
     else
     {
+        import core.stdc.config : c_ulong;
+
         struct unw_tdep_context_t
         {
             c_ulong[16] regs;

There are still compile errors for Android wrt. missing ucontext_t which could presumably be worked around with a version = DRuntime_Use_LLVM_Libunwind for Android (the NDK comes with a libunwind.a); no idea whether that's a good idea.

Should the compiler predefine DRuntime_Use_Libunwind for musl targets etc.? Otherwise the druntime user might not get what he asks for with a prebuilt druntime compiled with a manual version...

Edit: Oh and does the C compiler need an explicit -lunwind or so with DRuntime_Use_Libunwind, or is that one of the automagic libs?

@Geod24
Copy link
Copy Markdown
Author

Geod24 commented Jan 11, 2021

Should the compiler predefine DRuntime_Use_Libunwind for musl targets etc.? Otherwise the druntime user might not get what he asks for with a prebuilt druntime compiled with a manual version...

I was planning on patching it in the package until it's fully tested and upstreamed, but that works too. For reference, this is what I have for DMD (it's using the old implementation, before I dropped non-LLVM libunwind). But since Alpine is the main (only?) target for Musl at the moment, might as well do it in LDC.

which could presumably be worked around with a version = DRuntime_Use_LLVM_Libunwind

version = DRuntime_Use_Libunwind. I dropped support for the actual libunwind.

Edit: Oh and does the C compiler need an explicit -lunwind or so with DRuntime_Use_Libunwind, or is that one of the automagic libs?

I don't know. But since I'm patching the config file anyway, I can take care of that when I update the packages.

@kinke
Copy link
Copy Markdown
Member

kinke commented Jan 11, 2021

I dropped support for the actual libunwind.

So what's the DRuntime_Use_LLVM_Libunwind in libunwind.d?

As mentioned in the module DDOC, those bindings are only intended
to be used internally for the backtracing code,
hence only a couple functions and the related types were added.
libunwind is a portable, lightweight, widely adopted library to unwind the stack.
It is part of GCC via libiberty, and LLVM as well.

It is an alternative to the `backtrace` approach, which is not always available,
e.g. Musl libc does not provide a `backtrace` implementation, and the alternative,
libexecinfo, is known to have some bugs.

This is currently hidden behind a version, meaning only packagers or power users
will get access to it, so it can be properly tested.
@Geod24 Geod24 force-pushed the integrate-backtrace-fixes branch 2 times, most recently from c3c0b92 to 302f9a0 Compare January 12, 2021 00:55
@Geod24
Copy link
Copy Markdown
Author

Geod24 commented Jan 12, 2021

So what's the DRuntime_Use_LLVM_Libunwind in libunwind.d?

Cherry picked from the wrong branch 🤦
Fixed now, and integrated your diff.

EDIT: Actually just found a small bug, hold on.

Fixed: I needed to add the skip code in traceHandlerOpApply2 too.

This idea was suggested during the review of the new libunwind feature,
and is already used in LDC. Thanks to the recent refactoring,
we can improve on LDC's version by skipping the double demangling/iteration.
@Geod24 Geod24 force-pushed the integrate-backtrace-fixes branch from 302f9a0 to b1e8e33 Compare January 12, 2021 01:01
@kinke
Copy link
Copy Markdown
Member

kinke commented Jan 12, 2021

Okay, I've pushed 2 new commits.

@Geod24
Copy link
Copy Markdown
Author

Geod24 commented Jan 13, 2021

@kinke : Thanks! Anything else you need from my side ? Are you interested in upstreaming the additional commits ?

@kinke
Copy link
Copy Markdown
Member

kinke commented Jan 13, 2021

Are you interested in upstreaming the additional commits ?

I'll do it at one point unless you're keen on cherry-picking it yourself. ;)

Anything else you need from my side ?

I'm now predefining the version and linking-lunwind for musl targets in ldc-developers/ldc#3641 - could you please test & verify it for a smooth v1.25.0 on Alpine?

@kinke kinke force-pushed the integrate-backtrace-fixes branch from 822b767 to 5a41c0b Compare January 16, 2021 11:14
@kinke kinke merged commit 5a41c0b into ldc-developers:ldc Jan 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants