[android] Use emulated thread-local storage for API 28 and earlier #77883

finagolfin · 2024-11-30T16:11:46Z

Android before API 29 and a few other platforms don't support native TLS, so fall back to LLVM's emulated TLS there, just like clang does. Also, make sure -Xcc -f{no-,}emulated-tls flags passed in are applied to control what the Swift compiler does.

Swift 6 added a thread-local variable to the stdlib for all linux-based platforms for the first time, ie outside linux threading (Android uses pthreads instead), which then generated code in Foundation with native TLS even for Android API 24, breaking my Android CI that runs cross-compiled tests in the Android emulator at API 24.

I assumed the LLVM codegen backend automatically handled this, but it turns out it has to be explicitly enabled by the clang frontend, so I'm doing the same with the Swift frontend now. I just ran the compiler validation suite and toolchain tests on the Nov. 20 trunk snapshot built natively on Android 13 AArch64: this patch fixed running the Foundation tests and looks like it didn't cause any test regressions.

Considering Swift doesn't appear to have a way to choose native TLS for Swift variables, this pull appears to only really be useful for inline functions from C/C++ headers that are inserted into Swift's IR and codegened together.

@Azoy, who added this TLS variable in the C/C++ header earlier this year, and @al45tair, let me know what you think.

@compnerd and @hyp, I don't know if you test your Android SDK on API 28 or earlier, let me know if you've seen this regression.

@3405691582, OpenBSD is listed as one of the platforms using emulated TLS in that second link, let us know if that's correct.

finagolfin · 2024-12-04T11:42:08Z

@swift-ci please test

compnerd · 2024-12-04T23:06:10Z

lib/IRGen/IRGen.cpp

@@ -148,6 +148,9 @@ swift::getIRTargetOptions(const IRGenOptions &Opts, ASTContext &Ctx) {
  if (Clang->getTargetInfo().getTriple().isOSBinFormatWasm())
    TargetOpts.ThreadModel = llvm::ThreadModel::Single;

+  if (Clang->getTargetInfo().getTriple().hasDefaultEmulatedTLS())
+    TargetOpts.EmulatedTLS = true;


We probably should honour -Xcc -femulated-tls as well.

That's an interesting point: I tried it and right now it does nothing when generating Swift code. I'm against adding those sorts of -Xcc flags to affect codegen, particularly important ones, as I stated in the forum earlier this year.

However, I agree with you that it would be good to have a user-configurable flag for this affecting Swift codegen, and it's debatable if this rises to the level of importance of getting its own Swift frontend flag instead.

If you or anyone else has a preference on whether it should be its own Swift flag or an -Xcc flag, let me know and I will implement whatever the consensus is.

I chose the established -Xcc flags to control this, because this only affects inline functions from C/C++ headers that have been spliced into Swift IR anyway, as Swift doesn't seem to have a TLS annotation for its own variables.

finagolfin · 2024-12-29T17:57:12Z

Rebased and updated to add support for command-line flags that manually control this backend codegen flag. I only ran this new test natively on Android, so let's see how well it works on other CI platforms.

@swift-ci please test

finagolfin · 2024-12-29T18:07:47Z

@swift-ci please test

finagolfin · 2024-12-29T21:06:32Z

@swift-ci please test

finagolfin · 2024-12-30T15:46:41Z

@swift-ci please test windows

al45tair · 2024-12-30T16:28:35Z

lib/IRGen/IRGen.cpp

@@ -148,6 +148,8 @@ swift::getIRTargetOptions(const IRGenOptions &Opts, ASTContext &Ctx) {
  if (Clang->getTargetInfo().getTriple().isOSBinFormatWasm())
    TargetOpts.ThreadModel = llvm::ThreadModel::Single;

+  TargetOpts.EmulatedTLS = Opts.UseEmulatedTLS;


As @compnerd suggested, maybe we should copy the setting from Clang's options? A simple way might be do to something like this (untested, written in this comment):

Suggested change

TargetOpts.EmulatedTLS = Opts.UseEmulatedTLS;

TargetOpts.EmulatedTLS = Opts.UseEmulatedTLS || Clang->getCodeGenOpts().EmulatedTLS;

As noted above yesterday, I already implemented support for that flag then. However, I just tried your suggestion and that works too and is simpler, so thanks for the suggestion: I will update this pull to reuse your suggested ClangImporter flag instead.

Android before API 29 and a few other platforms don't support native TLS, so fall back to LLVM's emulated TLS there, just like clang does. Also, make sure `-Xcc -f{no-,}emulated-tls` flags passed in are applied to control what the Swift compiler does.

finagolfin · 2024-12-30T17:40:14Z

@swift-ci please test

finagolfin · 2024-12-31T09:28:26Z

@swift-ci please test

finagolfin · 2025-01-02T14:48:22Z

Ping, passed CI and addressed all reviews, just waiting for final approval.

finagolfin · 2025-01-08T11:11:32Z

Ping @compnerd, please review, would like to get this into 6.1 and 6.0 next.

3405691582 · 2025-01-08T13:13:40Z

OpenBSD is listed as one of the platforms using emulated TLS in that second link, let us know if that's correct.

I think this is still the case, but I haven't had a chance to confirm that.

compnerd

The warning on the ignored emulated TLS would be nice along with an explanation of why its ignored on WASI.

compnerd · 2025-01-08T17:02:25Z

test/IRGen/emulated-tls.swift

+// EMUTLS-macosx: __emutls_get_address
+// EMUTLS-openbsd: __emutls_get_address
+// EMUTLS-windows-msvc: __emutls_get_address
+// EMUTLS-wasi-NOT: __emutls_get_address


Hmm, why is this negated for WASI? Perhaps we should warn about the ignored setting?

I honestly didn't want to deal with WASI at all, but was forced to because the linux CI builds it and runs these tests for it too.

My understanding is that the initial __emutls_v._swift_stdlib_gettid.tid symbol is swapped in because the LLVM codegen pass runs on all platforms, but it doesn't have __emutls_get_address in compiler-rt, like the other platforms do, so it doesn't call that function. I could've simply disabled this test for WASI instead, but I figured it was worth checking the __emutls_v._swift_stdlib_gettid.tid symbol at least on all platforms.

This is just an artifact of the current CI and WASI support, I don't think it's worth addressing in the test.

Ah, okay, its just that the call is something else? That seems reasonable enough then. Perhaps an issue to track restoring this would be good.

Ah, okay, its just that the call is something else?

I don't know: I just did what I had to do to get the test to pass, as the CI was forcing me to address WASI but I'm not that familiar with it, didn't know if it even had threads yet. Looking at the relevant LLVM source now, the LowerToTLSEmulatedModel() method that inserts the __emutls_get_address call is not invoked for the WASM target, as it is for several other architectures. It appears that WASI threads is still in the standardization phase, so the implementation may still be in flux.

@kateinoigakukun, could you let us know if this might be a useful flag for WASM one day and if this test is okay for now?

All WebAssembly targets currently have proper TLS support (assuming dynamic linking is only for Emscripten, not with WASI), so the lowering does not implement emulation at all and silently ignores the option today as you mentioned. The thread representation model in Wasm is still unstable but I don't expect we need the emulation in the future, so it's fine as is now.

finagolfin · 2025-01-09T09:36:29Z

Merging as I want to get this into the release branches next: @compnerd, if you have any further concerns about the test, let me know and we'll get that in.

finagolfin requested a review from rjmccall as a code owner November 30, 2024 16:11

finagolfin mentioned this pull request Dec 1, 2024

Error when running swift-algorithmsPackageTests.xctest: "unknown reloc type 16" finagolfin/swift-android-sdk#175

Closed

compnerd reviewed Dec 4, 2024

View reviewed changes

finagolfin force-pushed the droid branch from b689be8 to fe05335 Compare December 29, 2024 17:52

finagolfin requested review from artemcm, tshortli, hborla, slavapestov and xedin as code owners December 29, 2024 17:52

finagolfin force-pushed the droid branch from fe05335 to ee40970 Compare December 29, 2024 21:05

al45tair reviewed Dec 30, 2024

View reviewed changes

finagolfin force-pushed the droid branch from ee40970 to c69a12c Compare December 30, 2024 17:38

finagolfin mentioned this pull request Jan 6, 2025

Enable PACBTI on OpenBSD/arm64. #78394

Merged

compnerd approved these changes Jan 8, 2025

View reviewed changes

finagolfin merged commit f67b35c into swiftlang:main Jan 9, 2025
5 checks passed

finagolfin deleted the droid branch January 9, 2025 09:38

This was referenced Jan 9, 2025

[android] Use emulated thread-local storage for API 28 and earlier #78513

Merged

[android] Use emulated thread-local storage for API 28 and earlier #78591

Merged

	TargetOpts.EmulatedTLS = Opts.UseEmulatedTLS;
	TargetOpts.EmulatedTLS = Opts.UseEmulatedTLS \|\| Clang->getCodeGenOpts().EmulatedTLS;

[android] Use emulated thread-local storage for API 28 and earlier #77883

[android] Use emulated thread-local storage for API 28 and earlier #77883

Uh oh!

Conversation

finagolfin commented Nov 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

finagolfin commented Dec 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

finagolfin commented Dec 29, 2024

Uh oh!

finagolfin commented Dec 29, 2024

Uh oh!

finagolfin commented Dec 29, 2024

Uh oh!

finagolfin commented Dec 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

finagolfin commented Dec 30, 2024

Uh oh!

finagolfin commented Dec 31, 2024

Uh oh!

finagolfin commented Jan 2, 2025

Uh oh!

finagolfin commented Jan 8, 2025

Uh oh!

3405691582 commented Jan 8, 2025

Uh oh!

compnerd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

finagolfin commented Jan 9, 2025

Uh oh!

Uh oh!

finagolfin commented Nov 30, 2024 •

edited

Loading