-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[Android] Add Android as availability platform #84574
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also add a test for the macros that are android specific in the clang importer invocation.
|
||
if (triple.isAndroid()) { | ||
invocationArgStrs.insert(invocationArgStrs.end(), { | ||
"-D__ANDROID_UNAVAILABLE_SYMBOLS_ARE_WEAK__", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears that the way this works is to turn off all compile-time preprocessor checking of Android API versions, turning them into link-time failures instead, if the API is more recent than the target API and the appropriate shared library link-time checking is enabled for strong references.
I am leery of making this change to all Swift code compiled for Android: is there some way we can only limit this to files/modules that need it because they contain #available
checks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean that this turns off processor checking of Android API versions? This only changes the behaviour of the availability attribute attached to specific declarations.
#if defined(__ANDROID_UNAVAILABLE_SYMBOLS_ARE_WEAK__)
#define __BIONIC_AVAILABILITY(__what, ...) __attribute__((__availability__(android,__what __VA_OPT__(,) __VA_ARGS__)))
#define __INTRODUCED_IN_NO_GUARD_FOR_NDK(api_level) __INTRODUCED_IN(api_level)
#else
#define __BIONIC_AVAILABILITY(__what, ...) __attribute__((__availability__(android,strict,__what __VA_OPT__(,) __VA_ARGS__)))
#define __INTRODUCED_IN_NO_GUARD_FOR_NDK(api_level)
#endif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe my perspective is skewed from only being familiar with Apple's SDKs, but I don't understand why we would not want this to be the default. From my quick reading it looks like it works exactly the same as on Apple platforms. The compiler will enforce that you must always check availability before calling the APIs which may not be available, so unless an API is mis-annotated there shouldn't be any harm and it seems like a strictly better developer experience to have access to newer APIs as long as you verify that they're present before you use them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean that this turns off processor checking of Android API versions?
In order to now be able to use older APIs than the target API, you have to disable compile-time API version checking to some extent. How extensive that would be is what worried me. Mads and I have been discussing this in Slack, which you saw now, and it looks like most Bionic APIs since NDK 28 can continue to be version checked at build time to some extent, by using the version annotations.
If that's the case, we should explore it and see what it can check.
The compiler will enforce that you must always check availability before calling the APIs which may not be available, so unless an API is mis-annotated there shouldn't be any harm and it seems like a strictly better developer experience to have access to newer APIs as long as you verify that they're present before you use them.
I have never used #available
, because I have never built Swift for Darwin platforms, nor have I ever used these new runtime availability options on Android, which were just added a couple years ago with NDK 26. I've been asking Mads privately for more info on how #available
works with C APIs, and he says he can maintain some build-time API version checking.
Let us see what he comes up with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After giving it some more though, I agree that enabling weak symbols is the behaviour we want. The thing that they changed in r28, was actually removing __ANDROID__API
checks from libc. This means that if you have enabled weak symbols, you can actually use the availability checking to call methods introduced in APIs greater than your minimum SDK version. Whereas before, it was just not possible, as the symbols were removed.
It seems like the coverage of the Bionic in terms of INTRODUCED_IN
is quite extensive, and has been since r18.
I also added support to ClangImporter, such that it will actually import the NDK headers with correct @availability
attributes.
This mode should not break anything for existing users that are compiling for Android. If their code was compiling, then all the NDK methods they were using must be supported on the version that they are compiling for, because of the __ANDROID__API
check in r27. And if they now try to use later APIs, then will be statically told to add availability checking.
Since this PR just adds support to #available/@available
, it gives us the static availability checking that I think @finagolfin is referring to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing that they changed in r28, was actually removing __ANDROID__API checks from libc.
About 80% were replaced when going from NDK 27 to 28, not all.
I also added support to ClangImporter, such that it will actually import the NDK headers with correct @Availability attributes.
Did you try this yourself? If so, add some tests for it.
This mode should not break anything for existing users that are compiling for Android.
Nobody has suggested it would. Rather the question was whether introducing new Swift code would then keep the same level of compile-time version checking as before, after you set this macro, which is why this is not the default in the NDK.
And if they now try to use later APIs, then will be statically told to add availability checking.
Great, please add some tests for that and you will have not only kept this checking the same, but added an important new static version checking feature of multiple API levels. 😺
Thanks for working on and looking into all this, @madsodgaard, I think all Android devs would love to have this. I will try this out locally in the coming days and review it.
@swift-ci please smoke test |
@swift-ci please build toolchain |
@swift-ci please build toolchain Windows platform |
1 similar comment
@swift-ci please build toolchain Windows platform |
_ patch: Builtin.Word | ||
) -> Builtin.Int1 { | ||
#if (os(macOS) || os(iOS) || os(tvOS) || os(watchOS) || os(visionOS)) && SWIFT_RUNTIME_OS_VERSIONING | ||
#if (os(macOS) || os(iOS) || os(tvOS) || os(watchOS) || os(visionOS) || os(Android)) && SWIFT_RUNTIME_OS_VERSIONING |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed the pull and tried it locally, unfortunately, the stdlib has a missing symbol after this change and test executables don't link. Informed Mads, he's looking into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a failure to find __isPlatformVersionAvailable
at link time? The LLVM library compiler-rt
provides that function. There may be some build system work necessary to ensure that it's available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, exactly, the Android NDK compiler-rt only provides __isOSVersionAtLeast
instead. Mads and I diagnosed that symbol difference as the problem and he said he will look into it more.
@swift-ci build toolchain windows |
7f1b959
to
5e87885
Compare
// RUN: %target-swift-frontend -target aarch64-unknown-linux-android24 -primary-file %s -emit-ir | %FileCheck %s | ||
// RUN: %target-swift-frontend -target aarch64-unknown-linux-android24 -primary-file %s -O -emit-ir | %FileCheck %s --check-prefix=OPT | ||
|
||
// REQUIRES: OS=linux-android || OS=linux-androideabi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems wrong. We don't care about running on Android, only that we have the android standard library. If you are feeling particularly clever, using -parse-as-stdlib
should allow this test to run always.
Adds support for using
Android
in@available
and#available
, for example:Uses the built-in support in compiler-rt to resolve the OS version at runtime.
Resolves #76671