Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support 16 KB page sizes on Android - libsentrysupplemental #3723

Merged
merged 45 commits into from
Nov 5, 2024

Conversation

jamescrosswell
Copy link
Collaborator

@jamescrosswell jamescrosswell commented Oct 31, 2024

Resolves #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:

# Set alignment to 16 KB. See https://developer.android.com/guide/practices/page-sizes#cmake
target_link_options(sentrysupplemental PRIVATE "-Wl,-z,max-page-size=16384")

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 and others added 30 commits October 22, 2024 13:23
@jamescrosswell
Copy link
Collaborator Author

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:

<ItemGroup Condition="'$(FrameworkSupportsNative)' == 'true' and ('$(RuntimeIdentifier)' == 'osx-x64' or '$(RuntimeIdentifier)' == 'osx-arm64')">
<DirectPInvoke Include="sentry-native" />
<NativeLibrary Include="$(MSBuildThisFileDirectory)..\sentry-native\osx\libsentry-native.a" />
<NativeSystemLibrary Include="curl" />
</ItemGroup>

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
Copy link

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: getsentry/sentry-java#3657
alpha release for testing: https://github.com/getsentry/sentry-java/releases/tag/7.17.0-alpha.1

@bruno-garcia
Copy link
Member

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
Copy link

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 #3633.

Base automatically changed from net9 to version-5.0.0 November 3, 2024 10:46
@jamescrosswell
Copy link
Collaborator Author

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 👍🏻

@jamescrosswell jamescrosswell linked an issue Nov 4, 2024 that may be closed by this pull request
@vaind
Copy link
Collaborator

vaind commented Nov 4, 2024

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

<ItemGroup Condition="'$(FrameworkSupportsNative)' == 'true' and ('$(RuntimeIdentifier)' == 'osx-x64' or '$(RuntimeIdentifier)' == 'osx-arm64')">
<DirectPInvoke Include="sentry-native" />
<NativeLibrary Include="$(MSBuildThisFileDirectory)..\sentry-native\osx\libsentry-native.a" />
<NativeSystemLibrary Include="curl" />
</ItemGroup>

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.

This reverts commit 8669e4c.
@jamescrosswell jamescrosswell marked this pull request as ready for review November 4, 2024 10:19
Copy link
Contributor

@bricefriha bricefriha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@bruno-garcia
Copy link
Member

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 getsentry/sentry-java#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

CHANGELOG.md Outdated Show resolved Hide resolved

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@jamescrosswell jamescrosswell merged commit d1e7392 into version-5.0.0 Nov 5, 2024
10 checks passed
@jamescrosswell jamescrosswell deleted the align-libsentry branch November 5, 2024 05:08
@jamescrosswell jamescrosswell linked an issue Nov 17, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support 16kb page size on Android 15 Alignment warnings when targeting net9.0-android - MAUI
6 participants