Refine 16k page alignment support #9075
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
grendello
force-pushed
the
dev/grendel/refine-16k-support
branch
2 times, most recently
from
fba6db9
to
a79f4e1
Compare
grendello
force-pushed
the
dev/grendel/refine-16k-support
branch
3 times, most recently
from
733c7ef
to
063973d
Compare
grendello
force-pushed
the
dev/grendel/refine-16k-support
branch
from
063973d
to
2111ae6
Compare
Merged
grendello
force-pushed
the
dev/grendel/refine-16k-support
branch
from
2111ae6
to
bde0496
Compare
grendello
added a commit
that referenced
this pull request
Jul 19, 2024
Changes: https://github.com/android/ndk/wiki/Changelog-r27 NDK r27 has been released. The most important change affecting us is that we should start building all our native libraries with [16k page alignment](https://github.com/android/ndk/wiki/Changelog-r27#announcements), support for which is already in `main` and will be refined once #9075 is merged. Notable changes: * NDK switched to LLVM 18.0.1 as its toolchain * A RISC-V sysroot (AKA riscv64, or rv64) has been added. It is not supported. It is present to aid bringup for OS vendors, but it's not yet a supported Android ABI. It will not be built by default. * Added APP_SUPPORT_FLEXIBLE_PAGE_SIZES for ndk-build and ANDROID_SUPPORT_FLEXIBLE_PAGE_SIZES for CMake. * The unsupported libclang, libclang-cpp, libLLVM, and libLTO libraries were removed to save space.
Context: ddb215b Context: https://github.com/google/bundletool/releases/tag/1.17.0 * ddb215b added an field to the `application_config` structure which specified the mask required to check whether entries in the APK that we read at run time are properly alignment at the 4k or 16k page boundary. This PR removes that code because it's possible that `bundletool` will change the package alignment **after** our code is already built. This would cause a false negative and an abort during application execution. Instead, we simply check whether the entry is 4k **or** 16k aligned. * Bump `bundletool` to 1.17.0, which now defaults to producing 16k-aligned archives. * Pass required alignment flags to Mono AOT compiler to produce properly aligned shared libraries. * Don't align 32-bit shared libraries to 16k, always use 4k alignment. This is in line with what NDK r27 does.
grendello
force-pushed
the
dev/grendel/refine-16k-support
branch
from
bde0496
to
fc13b7a
Compare
grendello
requested review from
dellis1972,
jonathanpeppers and
jonpryor
as code owners
Comment on lines
+63
to
+65
| if (segment64.Alignment == pageSize) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Does this need to continue; reading all segments? Or can it return early?
There was a problem hiding this comment.
We need to make sure all the relevant segments are aligned properly, so we can't return early. I also wanted to log all the misaligned segments in the debug message, so that the developer gets the full information (that's also why the warning is issued for every bad segment - to make it more visible and annoying, so that the issue is fixed instead of ignored)
Comment on lines
-62
to
66
| // assemblies must be 4-byte aligned, or Bad Things happen | ||
| if ((state.data_offset & application_config.zip_alignment_mask) != 0) { | ||
| // assemblies must be 16-byte or 4-byte aligned, or Bad Things happen | ||
| if (((state.data_offset & 0xf) != 0) || ((state.data_offset & 0x3) != 0)) { | ||
| log_fatal (LOG_ASSEMBLY, "Assembly '%s' is located at bad offset %lu within the .apk", entry_name.get (), state.data_offset); | ||
| log_fatal (LOG_ASSEMBLY, "You MUST run `zipalign` on %s to align it on %u bytes ", strrchr (state.file_name, '/') + 1, application_config.zip_alignment_mask + 1); | ||
| log_fatal (LOG_ASSEMBLY, "You MUST run `zipalign` on %s to align it on 4 or 16 bytes ", strrchr (state.file_name, '/') + 1); | ||
| Helpers::abort_application (); |
There was a problem hiding this comment.
What would happen if we just let Android try to load a non-aligned .so? Do we have to abort? Or would Android give its own error message that might be more appropriate, as it would show up in web searches.
There was a problem hiding this comment.
It would crash the application in some way and at some stage, I prefer to control the narrative, so to speak. This check is probably superfluous anyway, as I suppose bundletool will fail to create the aab/apk files in the future if any .so libraries are misaligned.
grendello
added a commit
that referenced
this pull request
Jul 23, 2024
* main: (23 commits) Localized file check-in by OneLocBuild Task (#9129) [ci] Disable CodeQL on CI/PR pipelines (#9128) Refine 16k page alignment support (#9075) [build] fix `ConfigureLocalWorkload` target (#9124) Bump to NDK r27 (#9020) [ci] Use drop service for SDK insertion artifacts (#9116) Fix up all mapping paths (#9121) [ci] Fix maestro publishing for stable packages (#9118) Bump to dotnet/sdk@2f14fea98b 9.0.100-preview.7.24367.21 (#9108) Missing androidx.window.[extensions|sidecar] warnings (#9085) [ci] Use sign-artifacts template for macOS signing (#9091) [ci] Use DotNetCoreCLI to sign macOS files (#9102) [ci] Disable CodeQL on macOS, Linux, non-main jobs (#9111) [tests] re-enable `JavaAbstractMethodTest` (#9097) [Microsoft.Android.Sdk.ILLink] preserve types with `IJniNameProviderAttribute` (#9099) [Mono.Android] Data sharing and Close() overrides (#9103) [AndroidManifest] Add `Android.App.PropertyAttribute` (#9016) [Mono.Android] Add support for AndroidMessageHandler ClientCertificates (#8961) [Mono.Android] Bind and enumify API-35 (#9043) Bump to dotnet/java-interop@7a058c0e (#9066) ...
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context: ddb215b
Context: https://github.com/google/bundletool/releases/tag/1.17.0
ddb215b added an field to the
application_configstructure whichspecified the mask required to check whether entries in the APK that
we read at run time are properly alignment at the 4k or 16k page
boundary. This PR removes that code because it's possible that
bundletoolwill change the package alignment after our code isalready built. This would cause a false negative and an abort during
application execution.
Instead, we simply check whether the entry is 4k or 16k aligned.
Bump
bundletoolto 1.17.0, which now defaults to producing16k-aligned archives.
Pass required alignment flags to Mono AOT compiler to produce
properly aligned shared libraries.
Don't align 32-bit shared libraries to 16k, always use 4k alignment.
This is in line with what NDK r27 does.