minimal implementation of sdk-version dependent... #752

supervacuus · 2022-09-22T11:13:31Z

...dynamic loader, but mostly to show that using the generic unwinder is probably a dead end. There was a lot more time invested than is visible, but I wanted to break this down to a minimal implementation to be able to discuss its issues. This contains an approach to tackle #743, but it has many problems:

libunwindstack, in SDK 28+, is still not exposed via any recent NDK and is thus not accessible to App developers or their dependencies but only to platform developers.
it seems clear that the generic unwinder does not wrap AOSP libunwindstack to make it available to app developers indirectly, so while this PR uses it for SDKs >= 28, the unwinder doesn't provide a better stack-trace than our stripped-down version of libunwindstack.
in addition to the above, the generic unwinder does not provide an interface to unwind relatively to the registers in ucontext_t or a passed-in address, which leads to stack traces that start from sentry-native frames.

So, at this point, it is relatively obvious that we cannot release this simply because it provides no visible improvements to the users (or rather regressions if you are deploying on targets with SDK >= 28).

It is not all bad, though:

by packaging our fork of libunwindstack in a shared library, which we dynamically load during sentry_init, I could get down to 1.5MiB (for the .so) from 2MiB (for the .a) on aarch64.
solely depending on the NDK unwinder (not that it would help us in the current implementation), produces an unwinder .soof only 26KiB, which would allow users who target only SDK >= 28 to drop the heavy legacy .so from their deployment packages.

So the question is how we should proceed with this (if at all):

we can stay with what we currently have and only provide the upstream updates.
we can pull in the upstream updates and change the unwinder on android to be dynamically loaded during sentry_init, cash in the package-size reduction, but not provide any improvements for SDK >= 28.
we can further investigate and try to resolve the C++ symbols from the platform libunwindstack dynamically, but it is very unclear how successful this will be. For one, it requires us to provide a minimal header environment to build the unwinder code in sentry-native and it requires the platform's builders to provide symbol-name stability for us to find even the small subset of the interface across deployed androids in the wild.
we can see if the backtrace() implementation in API 33 wraps libunwindstack and then replace the generic unwinder implementation we have now with backtrace(). In the worst case, it would at least provide us with an interface for a relative start into the stack.

Please chime in with further suggestions.

...dynamic loader but mostly to show that using the generic unwinder is probably a dead end.

github-actions · 2022-09-22T11:14:38Z

	Fails
🚫	Please consider adding a changelog entry for the next release.

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- minimal implementation of sdk-version dependent ([#752](https://github.com/getsentry/sentry-native/pull/752))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 6297019

Swatinem

lgtm, although it is a bit complex admittedly.

No matter what we decide specifically, we should update our fork regardless, maybe independently from this PR.

Swatinem · 2022-09-26T07:55:03Z

src/unwinder/sentry_unwinder_libunwindstack.c

+void
+sentry__unload_unwinder()
+{
+    int result = dlclose(handle);


manpage doesn’t really say if you can safely pass NULL here?

We should surely check this either way.

Swatinem · 2022-09-26T07:55:21Z

src/unwinder/sentry_unwinder_libunwindstack.c

+static android_unwind_stack_t android_unwind_stack;
+static void *handle;


we should explicitly initialize these to NULL

Swatinem · 2022-09-26T07:56:07Z

src/unwinder/sentry_unwinder_libunwindstack.cpp

@@ -1,6 +1,5 @@
 extern "C" {
-#include "sentry_boot.h"
-#include "sentry_core.h"
+#include <sentry.h>


this whole file isn’t used anymore, is it?

Swatinem · 2022-09-26T08:06:07Z

src/unwinder/libunwindstack_os.cpp

+    (void)addr;
+    (void)uctx;
+    trace_state state = { ptrs, ptrs + max_frames };
+    _Unwind_Backtrace(unwind_callback, &state);


I always confuse the _Unwind_* and the unw_* APIs. So unfortunate that the first one does not allow unwinding from uctx.

The unw_ prefix is the "namespace" of libunwind which would be very convenient library and I would love to use, but AOSP dropped it and upstream doesn't really support Android.

But even if it would provide a uctx interface, I am not sure if the added complexity justifies the changes, as long as it doesn't offer any expected/significant improvements in the stack trace (for instance by mapping to libunwindstack).

This is especially true since most users use a pre-built package, which doesn't gain anything from the size improvements when using only a slimmapper.so, without ("manually") removing the legacy.so.

libunwind is still deployed with NDK as the unwinder for C++ exception-handling and C scope-cleanups but it is not exposed as an app-developer library.

The hint that NDK versions below r23 re-export the symbols of the previous libgcc unwinder, potentially screwing with client code that uses exception-handling, shows that stability of that "interface" is not a driver for decisions). Nonetheless, i can also try to access this implementation.

supervacuus · 2022-09-26T09:57:05Z

No matter what we decide specifically, we should update our fork regardless, maybe independently from this PR.

This is probably the most valuable outcome of this investigation.

supervacuus · 2022-10-06T14:54:20Z

Closing since there is no conclusive net improvement visible for the changes required:

libunwindstack is still a platform library and not exposed to NDK users.
we cannot load (via dlopen/dlsym) the deployed libunwindstack.so system-library, even if we were able to provide a header-less mapping to its symbols, because android doesn't allow loading system libraries. I think this is for good reason, because an application cannot provide a stable mapping cross-version or cross-OEM. It is hidden for a reason.
using the generic unwinder does improve the stack trace slightly (it eliminates some frames, which we cannot for which we haven't even a mapping entry), but has no interface that allows us to provide a ucontext to start from (making the stack trace less helpful for users).
backtrace() in SDK_VER 33 is also implemented in terms of the generic unwinder: https://android-review.googlesource.com/c/platform/bionic/+/1863333/6/libc/bionic/execinfo.cpp (which we can
libunwinder is not supported as an NDK library and is currently only packaged in NDK as an unwinder for C++ exception handling.

what we can extract from this:

we can use an upstream synced version of our stripped down libunwindstack (which does not provide any considerable improvements visible in our limited testing). This will happen in a separate PR after merging Chore: update from upstream libunwindstack-ndk#6
we found out - as a side investigation - that for native crashdumps on android in release R and above we probably should use the tombstones, which provide a much richer crash context than we could ever extract ourselves (without access to system libraries). This should be discussed with in a bigger round, outside the confines of a PR.
we can try to further reduce the size a bit using our knowledge from this PR after updating our libunwindstack

minimal implementation of sdk-version dependent...

6297019

...dynamic loader but mostly to show that using the generic unwinder is probably a dead end.

Swatinem reviewed Sep 26, 2022

View reviewed changes

supervacuus closed this Oct 6, 2022

marandaneto mentioned this pull request Oct 14, 2022

Investigate the RN new architecture getsentry/sentry-react-native#2226

Closed

supervacuus mentioned this pull request Oct 14, 2022

Use Android system unwinder if API >= 28 #743

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

minimal implementation of sdk-version dependent... #752

minimal implementation of sdk-version dependent... #752

supervacuus commented Sep 22, 2022

github-actions bot commented Sep 22, 2022

Swatinem left a comment

Swatinem Sep 26, 2022

supervacuus Sep 26, 2022

Swatinem Sep 26, 2022

Swatinem Sep 26, 2022

Swatinem Sep 26, 2022

supervacuus Sep 26, 2022

supervacuus Sep 26, 2022

supervacuus commented Sep 26, 2022

supervacuus commented Oct 6, 2022

		static android_unwind_stack_t android_unwind_stack;
		static void *handle;

minimal implementation of sdk-version dependent... #752

minimal implementation of sdk-version dependent... #752

Conversation

supervacuus commented Sep 22, 2022

github-actions bot commented Sep 22, 2022

Instructions and example for changelog

Swatinem left a comment

Choose a reason for hiding this comment

Swatinem Sep 26, 2022

Choose a reason for hiding this comment

supervacuus Sep 26, 2022

Choose a reason for hiding this comment

Swatinem Sep 26, 2022

Choose a reason for hiding this comment

Swatinem Sep 26, 2022

Choose a reason for hiding this comment

Swatinem Sep 26, 2022

Choose a reason for hiding this comment

supervacuus Sep 26, 2022

Choose a reason for hiding this comment

supervacuus Sep 26, 2022

Choose a reason for hiding this comment

supervacuus commented Sep 26, 2022

supervacuus commented Oct 6, 2022