Skip to content

Conversation

@finagolfin
Copy link
Member

Mads just added this compiler feature for Android in swiftlang/swift#84574

I tested this locally on an Android API 35 device in the Termux app, no problem, before transferring the Testing test runner to an API 28 device. It would not link there, since it was an executable and libc.so at API 28 didn't have backtrace(), so I couldn't run the tests that way.

I then ran patchelf --add-needed libandroid-execinfo.so swift-testingPackageTests.xctest to have the test runner use the backported libexecinfo from the Termux app, which got all the tests to run. Three backtrace tests correctly failed because the runtime #available checking disabled this call, showing that runtime version checking worked. 😄

@finagolfin finagolfin added android 🤖 Android support cross-compilation Compiling tests for other platforms than the host labels Oct 21, 2025
@grynspan
Copy link
Contributor

We need to continue to build with the Swift 6.2 toolchain on other platforms. Do we need to preserve that functionality for Android given there is no official Android 6.2 toolchain? (Also pinging @compnerd.)

If so, the right way forward is to hold off on merging this PR until Swift 6.3 is released, at which point we'll drop 6.2 support and can proceed.

@grynspan grynspan added the tech-debt 💾 reduces technical debt label Oct 21, 2025
initializedCount = .init(clamping: backtrace(addresses.baseAddress!, .init(clamping: addresses.count)))
}
#elseif os(Android)
#if !SWT_NO_DYNAMIC_LINKING
Copy link
Contributor

Choose a reason for hiding this comment

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

You can get rid of this #if.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought so too initially, but the alternative is Android with static linking, in which case it will hit the final #else clause below and error.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's not what we use SWT_NO_DYNAMIC_LINKING for. We use it to indicate if dlsym() or equivalent is available. (I named it bad.) So you can just remove it. All Android codepaths will go through here. Even statically-linked Android binaries should be able to use weakly-linked NDK symbols.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see, I thought you meant remove just the #if, meaning adding an &&. I kept the check based on your adding it before and misreading this doc, will remove it.

Unsure how this will interact with static linking executables, given the dynamic linking issue I mentioned, but considering most will static link this into shared libraries eventually, may not matter.

Copy link
Member

Choose a reason for hiding this comment

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

@grynspan's suggestion makes sense - testing requires dynamic linking (i.e. you cannot statically link the testing framework). We know that the platform supports dynamic symbol resolution and because this is within the testing framework, you will be dynamically linking. As a result, just the new handling for the lookup is needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can in fact statically link Swift Testing!

Copy link
Member Author

Choose a reason for hiding this comment

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

In any case, we agreed on removing this check, which he had added, not me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yarp.

@grynspan
Copy link
Contributor

It would not link there, since it was an executable and libc.so at API 28 didn't have backtrace(), so I couldn't run the tests that way.

Does this mean that compiling against API 33+ and then running on an older device will fail? If so, that's not what we want: it indicates that weak linking isn't working, which means you can't use if #available(Android) to conditionally enable new NDK API use.

@finagolfin
Copy link
Member Author

finagolfin commented Oct 21, 2025

Do we need to preserve that functionality for Android given there is no official Android 6.2 toolchain?

Not sure what you mean, is that because trunk in this repo is currently auto-merged to release/6.2? If so, we could also check #if compiler(>=6.3) to make sure this is not parsed by the 6.2 toolchain.

Does this mean that compiling against API 33+ and then running on an older device will fail? If so, that's not what we want: it indicates that weak linking isn't working, which means you can't use if #available(Android) to conditionally enable new NDK API use.

Yep, if it's an executable, which seem to be required on Android to resolve all symbols at startup. Since the SwiftPM-produced test runner executable for this repo, that I built against a target, ie minimum, of API 24, doesn't invoke these methods from the toolchain's libTesting.so, but keeps them in the test runner and invokes them from there, I believe that's why it failed.

I don't think it will be an issue in Android apks, which almost always ship with native code in shared libraries instead, but haven't looked into it in detail.

@stmontgomery
Copy link
Contributor

If so, we could also check #if compiler(>=6.3) to make sure this is not parsed by the 6.2 toolchain.

That was going to be my suggestion as well.

@grynspan
Copy link
Contributor

Not sure what you mean, is that because trunk in this repo is currently auto-merged to release/6.2? If so, we could also check #if compiler(>=6.3) to make sure this is not parsed by the 6.2 toolchain.

It is possible and supported to build Swift Testing as a package using the current released toolchain (6.2). So we need to keep that working.

Yep, if it's an executable, which seem to be required on Android to resolve all symbols at startup. Since the SwiftPM-produced test runner executable for this repo, that I built against a target, ie minimum, of API 24, doesn't invoke these methods from the toolchain's libTesting.so, but keeps them in the test runner and invokes them from there, I believe that's why it failed.

I would expect weak linking to NDK symbols to work even for executable targets. If it doesn't, then we can't use #available here and must continue using dynamic lookup indefinitely.

@finagolfin
Copy link
Member Author

It is possible and supported to build Swift Testing as a package using the current released toolchain (6.2). So we need to keep that working.

Ah, I see, changing the dynamic linking check to #if compiler(>=6.3) instead okay then?

I would expect weak linking to NDK symbols to work even for executable targets. If it doesn't, then we can't use #available here and must continue using dynamic lookup indefinitely.

I don't know whether it should work differently for executables or not, was just presenting my hypothesis, as most won't use executables for testing their apps anyway. However, I'll look into it with Mads and let you know.

@grynspan
Copy link
Contributor

I don't know whether it should work differently for executables or not, was just presenting my hypothesis, as most won't use executables for testing their apps anyway. However, I'll look into it with Mads and let you know.

Let's see if we can solve the weak-linking mystery and then revisit. If it turns out it's fine, or it's some narrow edge case we really don't need to care about, great. If there's a bug in how #available functions on Android, I'd prefer we resolve that bug before proceeding. Thanks!

@grynspan
Copy link
Contributor

When you tried to run on API 28, had you set a minimum deployment target at compile time? Do we have a mechanism for doing so when building for Android? If it built against API 35 and you didn't set a minimum deployment target, I'd expect it to not bother weak-linking (same as on Darwin.)

@finagolfin
Copy link
Member Author

Yep, target, ie minimum, was API 24, mentioned it above. Mads and I are looking into it, could be something specific to the toolchain I natively built in the Termux app on Android, will let you know in a day or two.

@grynspan
Copy link
Contributor

D'oh, misread. Okay.

@finagolfin
Copy link
Member Author

It was ninja edited in, so maybe you read the first version. 😉

@finagolfin
Copy link
Member Author

We looked into the weak linking issue: it only shows up in the natively-built toolchain in the Termux app, not when cross-compiling. I'll look into the Termux issue later, but no reason to keep this waiting on that.

Rebased and made the changes discussed, ready to go ahead.

@grynspan
Copy link
Contributor

grynspan commented Nov 5, 2025

Alright. We can merge once CI is passing.

@finagolfin
Copy link
Member Author

Been discussing this pull with @marcprux and @madsodgaard: turns out this pull won't build with the next trunk Android SDK snapshot tag either, because this new #available(Android <API>, *) feature requires Android NDK 28 or later, but we currently build the Swift SDK for Android and use it with LTS NDK 27 only.

I just passed in NDK 28 on github to the official Docker scripts that build the SDK snapshots, and other than four compiler validation suite tests failing, everything worked, including locally cross-compiling this pull.

@compnerd, what do you think about switching the trunk 6.3 SDK snapshots over to NDK 28 now, both in Docker and in the Windows toolchain? We can still keep the current 6.2 release branch and the official 6.2 CI for Android on LTS NDK 27, thus only offering LTS NDK support for actual releases.

This will allow us to start preparing for the final 6.3 release in 4-5 months, so it will then use what will likely be the next LTS NDK 30 around that time.

@compnerd
Copy link
Member

compnerd commented Nov 9, 2025

There's certainly a strong reason to do that - the availability would simplify the Android build maintenance. I'm not totally opposed to it, but the LTS NDK is certainly preferable. We should verify that there's no other fallout from the migration before we commit to it I think.

…look for `backtrace()`

Mads just added this compiler feature for Android in swiftlang/swift#84574, so
update the NDK version too, since that new feature requires NDK 28 or later.
@finagolfin
Copy link
Member Author

I tried to change the NDK version used here, which appears to have disabled running the tests again. It won't work till the next trunk tag of the Swift SDK for Android anyway.

}

#if os(Android) && !SWT_NO_DYNAMIC_LINKING
#if compiler(<6.3) && os(Android)

Choose a reason for hiding this comment

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

why have you changed the order of conditionals here?

Copy link
Member Author

Choose a reason for hiding this comment

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

No particular reason, just the perception that such compiler versioning is more important.

Choose a reason for hiding this comment

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

which conditional has more complexity, the compiler version check or the os check?

Copy link
Member Author

Choose a reason for hiding this comment

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

No idea what you mean by complexity in this context, I just figured that the compiler check is more generally known by Swift devs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, compiler() has special powers in the compiler (heh). If it evaluates to false, subsequent conditions are not even parsed. This means that you can write something like #if compiler(>=6.3) && objectFormat(ELF) and an older compiler won't complain that it doesn't know what objectFormat() is.

It's good to get into the habit of putting compiler() first for this reason.

#if compiler(>=6.3)
if #available(Android 33, *) {
initializedCount = addresses.withMemoryRebound(to: UnsafeMutableRawPointer.self) { addresses in
.init(clamping: backtrace(addresses.baseAddress!, .init(clamping: addresses.count)))

Choose a reason for hiding this comment

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

maybe an if let for baseAddress here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe this almost always succeeds- unless the allocation up top fails?- so the unwrapping is a mere formality, but I'm just bringing back Jonathan's earlier code here: I didn't write this.

Choose a reason for hiding this comment

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

I see, for some reason (correct me I'm wrong) the closure gives you a buggy object with nil property references (such as a de-reference issue) maybe you will lead this into a SIGTRAP I think

Copy link
Member Author

Choose a reason for hiding this comment

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

In my experience, this always works and is a common idiom in the Swift corelibs.

Copy link
Contributor

Choose a reason for hiding this comment

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

This unwrap cannot fail. Hence the forced unwrap. There is no point in an if let because we know the else case will never be executed.

! is considered acceptable style within the Swift toolchain under these circumstances.

Copy link

@aluco100 aluco100 left a comment

Choose a reason for hiding this comment

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

LGTM

@grynspan
Copy link
Contributor

@finagolfin I'm approving now because obviously the code is shaped the way we want. Please don't merge it until we know for certain that the Android build will pass and is correctly emitting the requisite weak symbol reference.

@finagolfin
Copy link
Member Author

Yep, as referenced above, this would break both the Android and Windows CI unless we switched both to use NDK 28 or later, so we are in no hurry. 😉

ios_host_exclude_xcode_versions: '[{"xcode_version": "16.2"}, {"xcode_version": "16.3"}, {"xcode_version": "16.4"}]'
enable_wasm_sdk_build: true
enable_android_sdk_build: true
android_ndk_version: '["r28c"]'

Choose a reason for hiding this comment

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

This needs to be android_ndk_versions (plural) now; I changed it.

Choose a reason for hiding this comment

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

Also, r27d is the LTS NDK release, so maybe we should keep that and include r28?

Copy link
Member Author

Choose a reason for hiding this comment

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

r27d is the LTS NDK release, so maybe we should keep that and include r28?

We can't, as this new #available(Android feature in trunk and 6.3 only compiles with NDK 28 or later- the Bionic libc changed how it versioned its APIs by API level starting with NDK 28 and this feature requires that change- hence the drive to get all trunk/6.3 CI on NDK 28 or 29 now, in preparation for the likely next LTS NDK 30.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

android 🤖 Android support cross-compilation Compiling tests for other platforms than the host tech-debt 💾 reduces technical debt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants