[android] add a module map for Android NDK

[hyp]

This module map covers the Bionic C standard library and other Posix headers used in the Android NDK

[compnerd]

CC: @etcwilde

[hyp]

@swift-ci please test

[etcwilde]

Seems mostly reasonable, what is the plan for selecting a given API level to build against?

[finagolfin]

Oh nice, I had a similar pull I had been trying for both Glibc and Bionic, #66665, but this pull appears more specific to Bionic, which is good. Have you tried this pull with the C++ Interop tests on Android? Because they don't work with the current SwiftGlibc.h approach with NDK 26, hopefully this helps get them working.

[hyp]

but this pull appears more specific to Bionic, which is good. Have you tried this pull with the C++ Interop tests on Android? Because they don't work with the current SwiftGlibc.h approach with NDK 26, hopefully this helps get them working.

Yeah, this allows me to compile Swift code with C++ interop enabled, and use libc++ from the NDK as well in Swift :)

[finagolfin]

Yeah, this allows me to compile Swift code with C++ interop enabled, and use libc++ from the NDK as well in Swift

OK, they were mostly working before the latest NDK 26, but NDK 26 broke many of them. You were using NDK 26? How were you running those tests, in an emulator? Because the community Android CI doesn't run those executable tests, so I run them natively in the Termux app.

[hyp]

OK, they were mostly working before the latest NDK 26, but NDK 26 broke many of them. You were using NDK 26? How were you running those tests, in an emulator?

I didn't run the C++ interop tests on device yet unfortunately. What kind of issues did you see with NDK 26? That's something we will need for sure, but we don't have a good test setup yet.

[hyp]

what is the plan for selecting a given API level to build against?

good question, we didn't discuss that yet. let me figure that out.

[compnerd]

what is the plan for selecting a given API level to build against?

good question, we didn't discuss that yet. let me figure that out.

This seems to be currently behaving as desired - the module triple strips the API level. This allows us to build the SDK against an API level that we want to support (I am currently leaning towards 28). When the client code is built, it can build against a higher API level and still work with the single SDK build. The API level encoded version of the triple would be used by the driver for the linker driver.

[etcwilde]

This seems to be currently behaving as desired - the module triple strips the API level. This allows us to build the SDK against an API level that we want to support (I am currently leaning towards 28). When the client code is built, it can build against a higher API level and still work with the single SDK build. The API level encoded version of the triple would be used by the driver for the linker driver.

API level in the triple seems reasonable. That is in alignment with clang, IIRC.

[finagolfin]

What kind of issues did you see with NDK 26?

The same error I mentioned to you more than a year ago, except dozens of C++ Interop executable tests that passed with NDK 25 now fail to compile with that mbstate_t error with NDK 26. I know the issue is something related to NDK 26 because I natively built a late November trunk source snapshot tag of the Swift toolchain twice in the Termux app: the exact same source built once with NDK 25c then with NDK 26b.

[hyp]

What kind of issues did you see with NDK 26?

The same error I mentioned to you more than a year ago, except dozens of C++ Interop executable tests that passed with NDK 25 now fail to compile with that mbstate_t error with NDK 26. I know the issue is something related to NDK 26 because I natively built a late November trunk source snapshot tag of the Swift toolchain twice in the Termux app: the exact same source built once with NDK 25c then with NDK 26b.

I see. That error should go away with the updated module map, since I am able to successfully build and import the uchar module from the NDK now.

[hyp]

@swift-ci please test

[hyp]

@ian-twilightcoder I updated the module map to prefer split modules, how does it look now.

[hyp]

@swift-ci please test

[hyp]

@swift-ci please test source compatibility

[hyp]

Now mmap and statvfs can be found.

[hyp]

@finagolfin I was able to run more tests now with the regular Linux android setup, without going through termux. I can see that the test you were asking about passes, and I don't see any modularization errors for test/Interop/Cxx/class/constructors-executables.swift (when I build the test case only, without actually executing the test). So the modularization errors you saw seem specific to the termux setup, most likely the android overlay module map is not being applied correctly.

Unfortunately I'm not yet able to run the android linux C++ interop executable tests, but I get an improvement as compared to the baseline with respect to more android C++ interop tests now. I will merge this in, and then will continue on iterating on the remaining C++ interop test failures then so we can get them to be fully passing on a regular linux android setup.

[finagolfin]

I was able to run more tests now with the regular Linux android setup, without going through termux. I can see that the test you were asking about passes, and I don't see any modularization errors for test/Interop/Cxx/class/constructors-executables.swift (when I build the test case only, without actually executing the test).

This was with the version of this pull from last week or the commit you just pushed a couple hours ago?

So the modularization errors you saw seem specific to the termux setup, most likely the android overlay module map is not being applied correctly.

Unlikely, what about the 44 newly failing tests I mentioned, because they're still trying to use Glibc on Android: do you see any of those failures when running the compiler validation suite in host test mode yourself? The one I mentioned before, validation-test/stdlib/Glibc.swift, is an executable test, so you may not have seen that error, but others like this are not executable tests and should be failing for you too when running the compiler validation suite in host test mode.

Unfortunately I'm not yet able to run the android linux C++ interop executable tests, but I get an improvement as compared to the baseline with respect to more android C++ interop tests now.

A majority of the C++ Interop tests are executable, so you will not get a good idea of their status on Android without building them.

I will merge this in, and then will continue on iterating on the remaining C++ interop test failures then so we can get them to be fully passing on a regular linux android setup.

Hold off on merging, as until a couple hours ago, this pull was still failing to compile swift-corelibs-foundation, which just uses regular C interop. I will try building the Swift toolchain natively on Android with these two compiler pulls, that should be a good smoke test of this new Android overlay. Give me till next week to try that.

If you have any more changes to these Android pulls you want to add, now would be the time to add them, else I will have to start over with my testing once you add them.

[hyp]

@finagolfin

This was with the version of this pull from last week or the commit you just pushed a couple hours ago?

this was when building with the latest changes.

Unlikely, what about the 44 newly failing tests I mentioned, because they're still trying to use Glibc on Android: do you see any of those failures when running the compiler validation suite in host test mode yourself? The one I mentioned before, validation-test/stdlib/Glibc.swift, is an executable test, so you may not have seen that error, but others like this are not executable tests and should be failing for you too when running the compiler validation suite in host test mode.

Good call, let me check on them.

[finagolfin]

Before building again, I went ahead and diffed the list of headers included for the Glibc overlay, that was previously used on Android, against your new Android overlay. Here are the differences:

• Several headers from the clang resource directory, ie usr/lib/swift/clang/include/, are in Glibc but not your new Android overlay: float.h, iso646.h, stdarg.h, stdbool.h, stdddef.h, and tgmath.h.
• Two headers that are there in Bionic were seemingly replaced by your new Android overlay: net/if.h by the linux kernel header linux/if.h and netinet/tcp.h by netinet/in6.h.
• Several more sys/ headers from the Glibc overlay are not included in your Android overlay, despite existing: sys/select.h, sys/socket.h, sys/time.h, sys/times.h, sys/types.h, sys/user.h, and sys/wait.h.

Were these choices all intentionally made and why?

[finagolfin]

To check if this pull is working when cross-compiling for Android, I downloaded the latest linux toolchain build you kicked off, checked out the compiler source to the same commits, then ran this command to check the Swift Interop tests that don't require special tools like swift-ide-test:

./swift/utils/build-script -RA --skip-build-cmark --build-llvm=0 --native-clang-tools-path=/home/fina/usr/bin --native-swift-tools-path=/home/fina/usr/bin --build-swift-tools=0 --skip-early-swift-driver --android --android-ndk ~/android-ndk-r26d --android-arch aarch64 --android-api-level 24 --stdlib-deployment-targets=android-aarch64 -T --host-test --skip-test-linux --lit-args=--filter='Interop'

I did find that most of the non-executable Interop tests worked, including constructors-executable.swift cross-compiling successfully after removing the executable_test label. This implies the cyclic dependency errors I've been seeing when building natively are Termux-specific, as you suggested.

I then tried that clang_builtins.swift non-executable test that fails for me natively, and it fails in the same way when cross-compiling. If you would compile a full list of newly failing tests with your pulls when cross-compiling, we can compare with what I'm seeing natively.

I finally tried building swift-corelibs-foundation natively on Android with the latest version of this pull and it now builds successfully. However, I'm now seeing a couple dozen 47 more of those foundation tests failing natively on Android, which weren't before with the old SwiftGlibc module used on Android. I will look into those and let you know what's going on.

[hyp]

@finagolfin re: tests. Thanks for highlighting the Glibc dependency. I fixed up all the different test cases here in the second PR here - 4786393 . Now I do not see any new test failures caused by the new overlay when testing Swift itself as compared to the baseline. Can you check these test changes, to see if the unit test changes look good to you?

I finally tried building swift-corelibs-foundation natively on Android with the latest version of this pull and it now builds successfully. However, I'm now seeing a couple dozen 47 more of those foundation tests failing natively on Android, which weren't before with the old SwiftGlibc module used on Android. I will look into those and let you know what's going on.

Thanks for checking! I'll check these tests as well.

Were these choices all intentionally made and why?

I'll get back to you to about the headers later today.

[hyp]

Re: swift-corelibs-foundation tests. I have pushed some test fixes to the corresponding PR here:

apple/swift-corelibs-foundation@69cfa27

I can now build the swift-corelibs-foundation tests for Android.

[hyp]

@finagolfin

Re: swift-core-libs foundation. I updated the tests like I mentioned above. Could you please take another look.

Re: module map question:

Several headers from the clang resource directory, ie usr/lib/swift/clang/include/, are in Glibc but not your new Android overlay: float.h, iso646.h, stdarg.h, stdbool.h, stdddef.h, and tgmath.h.

They're intentionally not in the module map, but it's a good idea to add them back to the header files themselves. I added them back to 'SwiftAndroidNDK.h' and 'SwiftBionic.h'

Two headers that are there in Bionic were seemingly replaced by your new Android overlay: net/if.h by the linux kernel header linux/if.h and netinet/tcp.h by netinet/in6.h.

That's not an intentional replacement, net/if.h was missing by mistake, thanks for pointing it out! I added it to the module map and the header.

Several more sys/ headers from the Glibc overlay are not included in your Android overlay, despite existing: sys/select.h, sys/socket.h, sys/time.h, sys/times.h, sys/types.h, sys/user.h, and sys/wait.h.

Good call, they should be there. I added them to the 'SwiftAndroidNDK.h'.

All these changes are in the latest commit.

Is there anything else that you think is missing before this change can be merged?

[hyp]

@swift-ci please test

[hyp]

@swift-ci please test source compatibility

[finagolfin]

These are not part of Bionic though, are they usually required by the Bionic headers?

[hyp]

No, I dropped them!

[hyp]

@swift-ci please test

[hyp]

@swift-ci please test source compatibility

[finagolfin]

Can you check these test changes, to see if the unit test changes look good to you?

I'm kicking off a compiler validation suite run natively on Android now, will let you know by tonight.

Re: swift-core-libs foundation. I updated the tests like I mentioned above. Could you please take another look.

Huh, those tests built fine for me without those imports, I will apply your new commit and re-test.

Is there anything else that you think is missing before this change can be merged?

What about netinet/tcp.h, which I mentioned earlier?

Give me a couple days to build and test the Swift toolchain natively on Android with your latest commits and I will let you know what I find.

[finagolfin]

Alright, ran the compiler validation suite with the latest version of these two pulls of yours: looks like your import additions to the tests fixed 20 of the 44 failing tests I mentioned earlier. Another dozen are C++ interop tests that pass with the SwiftGlibc module, but fail with that cyclic dependency build error in Termux with your new SwiftAndroid module, like test/Interop/Cxx/class/method/unsafe-projections.swift.

The rest are probably like the test/Concurrency/Runtime/data_race_detection.swift executable test, which I just patched like this to get passing with your pulls:

diff --git a/test/Concurrency/Runtime/data_race_detection.swift b/test/Concurrency/Runtime/data_race_detection.swift
index cd6a98172d1..18b63fe6b37 100644
--- a/test/Concurrency/Runtime/data_race_detection.swift
+++ b/test/Concurrency/Runtime/data_race_detection.swift
@@ -20,6 +20,8 @@ import Dispatch
 import Darwin
 #elseif canImport(Glibc)
 import Glibc
+#elseif canImport(Android)
+import Android
 #endif
 
 @MainActor func onMainActor() {
@@ -61,14 +63,14 @@ actor MyActor {
 struct Runner {
   static func main() async {
     print("Launching a main-actor task")
-    // CHECK: warning: data race detected: @MainActor function at main/data_race_detection.swift:25 was not called on the main thread
+    // CHECK: warning: data race detected: @MainActor function at main/data_race_detection.swift:27 was not called on the main thread
     launchFromMainThread()
     sleep(1)
 
     let actor = MyActor()
     let actorFn = await actor.getTaskOnMyActor()
     print("Launching an actor-instance task")
-    // CHECK: warning: data race detected: actor-isolated function at main/data_race_detection.swift:54 was not called on the same actor
+    // CHECK: warning: data race detected: actor-isolated function at main/data_race_detection.swift:56 was not called on the same actor
     launchTask(actorFn)
 
     sleep(1)

I will look into those other validation suite regressions and get back to you tomorrow. In the meantime, let me know about that last tcp.h header.

[hyp]

@finagolfin thanks!

What about netinet/tcp.h, which I mentioned earlier?

That was indeed one I missed, thanks! Added it to the module map and the header.

The rest are probably like the test/Concurrency/Runtime/data_race_detection.swift executable test, which I just patched like this to get passing with your pulls:

Thanks, I updated this test case too and included it in the sibling PR: #72634 . Also addressed your comments there related to other changed tests.

[hyp]

@swift-ci please test

[finagolfin]

I updated to the latest version of these two pulls and ran the compiler validation suite again, got the same results. I didn't get to much else though, like going through all the failing tests, will do that this weekend and let you know by Monday.

[hyp]

Thanks, sounds good!

[hyp]

Could you also list the failing tests you saw?

[finagolfin]

Sure, two tests fail because of the issue I raised in #73445, as a result of the frontend patch I showed there: test/Frontend/android-sdk.swift and test/ModuleInterface/BadStdlib.swiftinterface. The first can be properly fixed by changing my patch according to my first comment on that issue, no idea what's going on with that BadStdlib test.

As mentioned, 11 C++ Interop tests that passed natively on Android with the SwiftGlibc module now fail with your proposed module, all with that cyclic dependency error:

test/Interop/Cxx/class/method/unsafe-projections.swift
test/Interop/Cxx/class/move-only/inherited-field-access-irgen.swift
test/Interop/Cxx/class/move-only/inherited-field-access-silgen.swift
test/Interop/Cxx/operators/move-only/move-only-synthesized-property-typecheck.swift
test/Interop/Cxx/stdlib/overlay/custom-collection-module-interface.swift
test/Interop/Cxx/stdlib/overlay/custom-iterator-module-interface.swift
test/Interop/Cxx/stdlib/overlay/custom-sequence-module-interface.swift
test/Interop/Cxx/stdlib/use-cxxstdlib-types-in-module-interface.swift
test/Interop/Cxx/stdlib/use-std-pair-typechecker.swift
test/Interop/CxxToSwiftToCxx/bridge-cxx-struct-back-to-cxx-execution.cpp
test/Interop/CxxToSwiftToCxx/consuming-cxx-struct-parameter-back-to-cxx-execution.cpp

Since only the last two are executable tests, do the first nine all pass for you when cross-compiling from linux in host-test mode with these pulls? I will look into whether Termux is modifying those Bionic headers in some way that gets them to fail with your new Android module.

The remaining eight executable tests I just fixed and submitted as a pull against #72634, hyp#1.

[hyp]

Sure, two tests fail because of the issue I raised in #73445, as a result of the frontend patch I showed there: test/Frontend/android-sdk.swift and test/ModuleInterface/BadStdlib.swiftinterface. The first can be properly fixed by changing my patch according to my first comment on that issue, no idea what's going on with that BadStdlib test.

I'm unable to reproduce these two failures, they pass when testing for android on a regular linux setup setup (cross-compiled stdlib being tested with toolchain built with all changes + extra tools built locally). I'm not sure I fully understand that issue since it appears to be also Termux-specific but why is there still the Glibc dependency in that case?

As mentioned, 11 C++ Interop tests that passed natively on Android with the SwiftGlibc module now fail with your proposed module, all with that cyclic dependency error:

test/Interop/Cxx/class/method/unsafe-projections.swift
test/Interop/Cxx/class/move-only/inherited-field-access-irgen.swift
test/Interop/Cxx/class/move-only/inherited-field-access-silgen.swift
test/Interop/Cxx/operators/move-only/move-only-synthesized-property-typecheck.swift
test/Interop/Cxx/stdlib/overlay/custom-collection-module-interface.swift
test/Interop/Cxx/stdlib/overlay/custom-iterator-module-interface.swift
test/Interop/Cxx/stdlib/overlay/custom-sequence-module-interface.swift
test/Interop/Cxx/stdlib/use-cxxstdlib-types-in-module-interface.swift
test/Interop/Cxx/stdlib/use-std-pair-typechecker.swift
test/Interop/CxxToSwiftToCxx/bridge-cxx-struct-back-to-cxx-execution.cpp
test/Interop/CxxToSwiftToCxx/consuming-cxx-struct-parameter-back-to-cxx-execution.cpp

Since only the last two are executable tests, do the first nine all pass for you when cross-compiling from linux in host-test mode with these pulls? I will look into whether Termux is modifying those Bionic headers in some way that gets them to fail with your new Android module.

Thanks! I tested each individually and they're passing on a regular linux setup (cross-compiled stdlib being tested with toolchain built with all changes + extra tools built locally). Using ndk-r26d. The cyclic dependency error is #72161 (comment) right?

The remaining eight executable tests I just fixed and submitted as a pull against #72634, hyp#1.

Thank you, I will add them to the second PR.

@finagolfin Do you think I can merge these changes now? The remaining failures appear to be Termux-specific if I understand correctly. I just ordered a device and it will arrive in two days, so I will setup execution tests and will be able to fix the Termux interop failures as well. Fwiw I think that the interop cyclic dependency error on Termux is fixable for sure, so I will be able to fix it once I have the device setup.

[finagolfin]

I'm not sure I fully understand that issue since it appears to be also Termux-specific but why is there still the Glibc dependency in that case?

Yes, it only happens because I'm modifying the trunk frontend as shown there, won't happen otherwise. The issue is the stdlib has canImport(Glibc) before canImport(Android), so the trunk frontend looks for Glibc in the new trunk stdlib I'm building with your patches, but doesn't find it, then it looks in the -sdk and finds the old Glibc I used to build the trunk frontend in the first place, ie the host prebuilt toolchain before your patches.

As stated in that issue, that is an unrelated edge case on all platforms that this Android pull simply uncovered when building natively, so don't worry about it. I will see about fixing that separately.

I tested each individually and they're passing on a regular linux setup (cross-compiled stdlib being tested with toolchain built with all changes + extra tools built locally). Using ndk-r26d. The cyclic dependency error is #72161 (comment) right?

Yep, I'll look into those C++ interop issues in Termux later, since you're not seeing them with the NDK.

I just ordered a device and it will arrive in two days, so I will setup execution tests and will be able to fix the Termux interop failures as well. Fwiw I think that the interop cyclic dependency error on Termux is fixable for sure, so I will be able to fix it once I have the device setup.

Great, you will need extra Termux-specific patches to build the Swift toolchain. I've been meaning to document that on my Android SDK github repo, will do so now that you want to try it.

Do you think I can merge these changes now? The remaining failures appear to be Termux-specific if I understand correctly.

The Swift compiler validation suite appears to be in good shape with your pulls, but I'm still seeing those 47 foundation tests regressing with your latest patches. Give me a day or two to look into those, probably not hard to fix.

The good news is that I've been running the tests for other parts of the toolchain with your pulls, and no regressions in the tests for libdispatch, XCTest, and llbuild so far, after modifying them to import Android instead.