search

getsentry / sentry-dotnet

Sentry SDK for .NET
https://docs.sentry.io/platforms/dotnet
MIT License
599 stars 206 forks source link

Support 16 KB page sizes on Android - libsentrysupplemental #3723

Closed jamescrosswell closed 2 weeks ago

jamescrosswell commented 3 weeks ago

Resolves https://github.com/getsentry/sentry-dotnet/issues/3633

Summary

When targeting net9.0-android we get build warnings from Xamarin.Android.Common.Debugging.targets when it tries to build the apk:

warning XA0141: NuGet package '<unknown>' version '<unknown>' contains a shared library 'libsentry.so' which is not correctly aligned. See https://developer.android.com/guide/practices/page-sizes for more details
warning XA0141: NuGet package '<unknown>' version '<unknown>' contains a shared library 'libsentry-android.so' which is not correctly aligned. See https://developer.android.com/guide/practices/page-sizes for more details
warning XA0141: NuGet package '<unknown>' version '<unknown>' contains a shared library 'libsentrysupplemental.so' which is not correctly aligned. See https://developer.android.com/guide/practices/page-sizes for more details

This link contained in the warning explains.

Solution

sentry-dotnet repo

This PR addresses the warnings for libsentrysupplemental.so, the source for which is in this repository. The most relevant parts of the PR are: https://github.com/getsentry/sentry-dotnet/blob/8669e4c4c840809eb1e98dba4f458df312a30215/lib/sentrysupplemental/CMakeLists.txt#L6-L7

Rebuilding with those changes results in changes to the so files, which we have checked in to this repo (presumably because they change very infrequently).

And to test this (since it only presents for us when targeting net9.0-android) we bumped the target frameworks for samples/Sentry.Samples.Maui/Sentry.Samples.Maui.csproj from net8.0 to net9.0.

sentry-jave repo

The other shared libraries are part of the sentry-native repository that we get transitively through sentry-java when targeting net9.0-android.

Tracking issue:

jamescrosswell commented 3 weeks ago

That fix was included in the 0.7.8 release. As we already have version 0.7.10 of the sentry-native module refenced in setnry-dotnet, it's unclear why this fix isn't flowing through to net9.0-ios builds.... needs some digging.

@supervacuus it looks like when you made the change to support this in sentry-native, it was only enabled for shared libs:

if(SENTRY_BUILD_SHARED_LIBS)
    target_link_libraries(sentry PRIVATE
            "$<$<OR:$<PLATFORM_ID:Linux>,$<PLATFORM_ID:Android>>:-Wl,--build-id=sha1,--version-script=${PROJECT_SOURCE_DIR}/src/exports.map>")

    # Support 16KB page sizes
    target_link_libraries(sentry PRIVATE
        "$<$<PLATFORM_ID:Android>:-Wl,-z,max-page-size=16384>"
    )
endif()

Where the comment for SENTRY_BUILD_SHARED_LIBS is:

option(SENTRY_BUILD_SHARED_LIBS "Build shared libraries (.dll/.so) instead of static ones (.lib/.a)" ${BUILD_SHARED_LIBS})

The sentry-dotnet library appears to link sentry-native statically however: https://github.com/getsentry/sentry-dotnet/blob/830ba36c1f508f7729cdb11439b3fc9f665699e2/src/Sentry/Platforms/Native/buildTransitive/Sentry.Native.targets#L30-L34

Could that explain why we're getting alignment warnings from the sentry-native libraries we're linking in the sentry-dotnet repo?

Full disclosure: I know virtually nothing about native development... I vaguely remember studying C and C++ about 30 years ago 😛

supervacuus commented 3 weeks ago

I don't think what you're seeing is related to the version of sentry-native that the dotnet SDK references directly, but rather the one packaged via sentry-android. The static builds aren't used on Android either, and the build parameter is only enabled for sentry-native Android builds.

The changes here, together with a release of sentry-android that includes the 16kb compatible native SDK binaries, should fix #3633.

tracking issue: https://github.com/getsentry/sentry-java/issues/3657 alpha release for testing: https://github.com/getsentry/sentry-java/releases/tag/7.17.0-alpha.1

bruno-garcia commented 3 weeks ago

The sentry-dotnet library appears to link sentry-native statically however:

We only do this for Native AOT on desktop afaik (do we also do this on mobile @vaind ?)

I don't think what you're seeing is related to the version of sentry-native

One of those .so files are bundled in this repo so makes sense this PR is addressing this one at least. Everything else makes sense and will be fixed by bumping sentry-android as you indicated

supervacuus commented 3 weeks ago

I don't think what you're seeing is related to the version of sentry-native

One of those .so files are bundled in this repo so makes sense this PR is addressing this one at least. Everything else makes sense and will be fixed by bumping sentry-android as you indicated

Absolutely, that is why I wrote:

The changes here, together with a release of sentry-android that includes the 16kb compatible native SDK binaries, should fix https://github.com/getsentry/sentry-dotnet/issues/3633.

jamescrosswell commented 2 weeks ago

Thanks @supervacuus,

I don't think what you're seeing is related to the version of sentry-native that the dotnet SDK references directly, but rather the one packaged via sentry-android.

Ah, OK that makes sense.

The changes here, together with a release of sentry-android that includes the 16kb compatible native SDK binaries, should fix #3633.

tracking issue: getsentry/sentry-java#3657 alpha release for testing: https://github.com/getsentry/sentry-java/releases/tag/7.17.0-alpha.1

Just tested with 7.17.0-alpha.1 and I can confirm that this resolves the alignment warnings 👍🏻

vaind commented 2 weeks ago

The sentry-dotnet library appears to link sentry-native statically however:

https://github.com/getsentry/sentry-dotnet/blob/830ba36c1f508f7729cdb11439b3fc9f665699e2/src/Sentry/Platforms/Native/buildTransitive/Sentry.Native.targets#L30-L34

Could that explain why we're getting alignment warnings from the sentry-native libraries we're linking in the sentry-dotnet repo?

Full disclosure: I know virtually nothing about native development... I vaguely remember studying C and C++ about 30 years ago 😛

We only do this for Native AOT on desktop afaik (do we also do this on mobile @vaind ?)

Sentry.Native subproject here is just desktop. I'm not aware of what the mobile side is doing in sentry-java but as @jamescrosswell have just noticed, there's a PR in sentry-java to fix this issue which would come in due time and I don't think we need to do anything in the dotnet repo.

bruno-garcia commented 2 weeks ago

Sentry.Native subproject here is just desktop. I'm not aware of what the mobile side is doing in sentry-java but as @jamescrosswell have just noticed, there's a https://github.com/getsentry/sentry-java/pull/3620 to fix this issue which would come in due time and I don't think we need to do anything in the dotnet repo.

We need both. This PR fixes the "suplemental" lib which is just an example crash we bundle. And sentry-java monorepo has Android, which has NDK which has sentry-native, so native libs on Android also built there. Which that PR fixes.

So we can merge this, and independently bump Android once that PR is merged, and the all the warnings should be gone