search

apple / swift-atomics

Low-level atomic operations for Swift
Apache License 2.0
1.06k stars 50 forks source link

Ship Swift Atomics 1.2 #104

Closed lorentey closed 11 months ago

lorentey commented 11 months ago

This release will technically not contain any API-level changes, but the fix for #88 is essentially a complete rewrite of the package, so it's risky enough to deserve a minor version bump.

I will not be going through the full test matrix for this release.

Candidate commit:

Diff: https://github.com/apple/swift-atomics/compare/1.1.0...main

Testing progress: (On Mac and Linux, this is done by running Utilities/run-full-tests.sh. This script builds and tests this package using various build systems and build configurations.)

macOS/AS (Including simulator tests for iOS/watchOS/tvOS)

macOS/x86_64

Linux/Aarch64

Linux/x86_64

Windows 10/x86_64

Known failures:

lorentey commented 11 months ago

Gah, CMakeLists.txt references the .gyb files as sources. This doesn't seem to cause problems, but we should not list them.

lorentey commented 11 months ago

Xcode 15 seems to have trouble linking test targets building for the generic watchOS simulator, which seems to include i386. Ignoring.

Ld /tmp/run-full-tests.sh.A57jS/xcodebuild/Build/Intermediates.noindex/swift-atomics.build/Release-watchsimulator/AtomicsTests.build/Objects-normal/i386/Binary/AtomicsTests normal i386 (in target 'AtomicsTests'
 from project 'swift-atomics')
...
Undefined symbols for architecture i386:
  "_$s6XCTest12XCTAssertNil__4file4lineyypSgyKXK_SSyXKs12StaticStringVSutF", referenced from:
      _$s12AtomicsTests019AtomicLazyReferenceB0C26test_unsafe_create_destroyyyFTo in AtomicLazyReference.o
      _$s12AtomicsTests019AtomicLazyReferenceB0C30test_unsafe_storeIfNilThenLoadyyFTf4d_n in AtomicLazyReference.o
      _$s12AtomicsTests019AtomicLazyReferenceB0C31test_managed_storeIfNilThenLoadyyFTf4d_n in AtomicLazyReference.o
      _$s12AtomicsTests027LockFreeSingleConsumerStackB0C11test_BasicsyyFTf4d_n in LockFreeSingleConsumerStack.o
     (maybe you meant: _$s6XCTest12XCTAssertNil__4file4lineyypSgyKXK_SSyXKs12StaticStringVSutFfA0_SSycfu_)
  "_$s6XCTest13XCTAssertTrue__4file4lineySbyKXK_SSyXKs12StaticStringVSutF", referenced from:
      _$s12AtomicsTests019AtomicLazyReferenceB0C30test_unsafe_storeIfNilThenLoadyyFTf4d_n in AtomicLazyReference.o
      _$s12AtomicsTests019AtomicLazyReferenceB0C31test_managed_storeIfNilThenLoadyyFTf4d_n in AtomicLazyReference.o
      _$s12AtomicsTests015BasicAtomicBoolB0C28test_compareExchange_relaxedyyFTf4d_n in BasicAtomicBoolTests.o
      _$s12AtomicsTests015BasicAtomicBoolB0C30test_compareExchange_acquiringyyFTf4d_n in BasicAtomicBoolTests.o
      _$s12AtomicsTests015BasicAtomicBoolB0C30test_compareExchange_releasingyyFTf4d_n in BasicAtomicBoolTests.o
      _$s12AtomicsTests015BasicAtomicBoolB0C42test_compareExchange_acquiringAndReleasingyyFTf4d_n in BasicAtomicBoolTests.o
      _$s12AtomicsTests015BasicAtomicBoolB0C43test_compareExchange_sequentiallyConsistentyyFTf4d_n in BasicAtomicBoolTests.o
      ...
     (maybe you meant: _$s6XCTest13XCTAssertTrue__4file4lineySbyKXK_SSyXKs12StaticStringVSutFfA0_SSycfu_)
... etc ...
lorentey commented 11 months ago

The xcodebuild.library-evolution build is failing to import the _AtomicShims module on Xcode 15 (iOS). This smells like the known issue with XCBuild's dependency management for C modules.

This package isn't designed to be built with library evolution enabled, so I'm not treating this as a blocker.

[22 xcodebuild.library-evolution] xcodebuild -scheme swift-atomics -destination generic/platform=macOS -destination generic/platform=iOS BUILD_LIBRARY_FOR_DISTRIBUTION=YES
SwiftVerifyEmittedModuleInterface normal arm64 Verifying\ emitted\ module\ interface\ Atomics.private.swiftinterface /Users/klorentey/Library/Developer/Xcode/DerivedData/swift-atomics-fflkxpmrsggtlnbcxorrecorsseo/Build/Intermediates.noindex/swift-atomics.build/Debug/Atomics.build/Objects-normal/arm64/Atomics.private.swiftinterface (in target 'Atomics' from project 'swift-atomics')
...
/Users/klorentey/Library/Developer/Xcode/DerivedData/swift-atomics-fflkxpmrsggtlnbcxorrecorsseo/Build/Intermediates.noindex/swift-atomics.build/Debug/Atomics.build/Objects-normal/arm64/Atomics.private.swiftinterface:8:8: error: no such module '_AtomicsShims'
import _AtomicsShims
       ^
lorentey commented 11 months ago

A wild failure appeared!

[26 xcodebuild.test.macCatalyst] on macOS 14, AS:

Command line invocation:
    /Applications/Xcode-MAS.app/Contents/Developer/usr/bin/xcodebuild -scheme swift-atomics -configuration Release -destination "platform=macOS,variant=Mac Catalyst" -derivedDataPath /tmp/run-full-tests.sh.faoJS/xcodebuild test
[...]
Test Case '-[AtomicsTests.BasicAtomicPointerTests test_weakCompareExchange_acquiringAndReleasing_acquiring]' started.
/Users/klorentey/Docker/swift-atomics/Tests/AtomicsTests/Basics/autogenerated/BasicAtomicPointerTests.swift:1603: error: -[AtomicsTests.BasicAtomicPointerTests test_weakCompareExchange_acquiringAndReleasing_acquiring] : XCTAss
ertTrue failed
/Users/klorentey/Docker/swift-atomics/Tests/AtomicsTests/Basics/autogenerated/BasicAtomicPointerTests.swift:1605: error: -[AtomicsTests.BasicAtomicPointerTests test_weakCompareExchange_acquiringAndReleasing_acquiring] : XCTAss
ertEqual failed: ("0x0000600002183860") is not equal to ("0x0000600002183870")
/Users/klorentey/Docker/swift-atomics/Tests/AtomicsTests/Basics/autogenerated/BasicAtomicPointerTests.swift:1612: error: -[AtomicsTests.BasicAtomicPointerTests test_weakCompareExchange_acquiringAndReleasing_acquiring] : XCTAss
ertFalse failed
/Users/klorentey/Docker/swift-atomics/Tests/AtomicsTests/Basics/autogenerated/BasicAtomicPointerTests.swift:1613: error: -[AtomicsTests.BasicAtomicPointerTests test_weakCompareExchange_acquiringAndReleasing_acquiring] : XCTAss
ertEqual failed: ("0x0000600002183860") is not equal to ("0x0000600002183870")
Test Case '-[AtomicsTests.BasicAtomicPointerTests test_weakCompareExchange_acquiringAndReleasing_acquiring]' failed (0.999 seconds).

The same test is passing on every other configuration I've tried so far. The failure is specific to this one ordering. Huh?

lorentey commented 11 months ago

Oh, d'oh, the weak compare-exchange is allowed to spuriously fail, but the test expects it to unconditionally succeed. The scheduler must have decided to randomly suspend execution of the process in the middle of the LL/SC transaction.

All weakCompareExchange tests need to be adjusted to allow spurious failures. 🙈

lorentey commented 11 months ago

The Swift 5.11 snapshot as of 2023-09-24 doesn't interoperate well with Xcode 15 GM's package integration, so xcodebuild fails to build with "Invalid manifest" errors. This is due to the version mismatch between xcbuild and swiftc/swiftpm; it isn't specific to this codebase.

watchOS builds of the xcodeproj under ./Xcode fail to link due to what looks like a missing arch issue in the toolchain's libraries. Also filing this as due to a version mismatch unrelated to this project.

Otherwise 5.11 seems to work okay on the candidate commit.

lorentey commented 11 months ago

Oops, platform conditionals are broken on 5.8:

[...]/swift-atomics/Sources/Atomics/Primitives/autogenerated/Primitives.native.swift:2113:5: error: unexpected platform condition (expected 'os', 'arch', or 'swift')
#if _pointerBitWidth(_64)
    ^
[...]/swift-atomics/Sources/Atomics/Primitives/autogenerated/Primitives.native.swift:2441:5: error: unexpected platform condition (expected 'os', 'arch', or 'swift')
#if _pointerBitWidth(_64)
    ^
[...]/swift-atomics/Sources/Atomics/Primitives/autogenerated/Primitives.native.swift:2444:9: error: unexpected platform condition (expected 'os', 'arch', or 'swift')
#elseif _pointerBitWidth(_32)
        ^
[...]/swift-atomics/Sources/Atomics/Types/DoubleWord.swift:16:5: error: unexpected platform condition (expected 'os', 'arch', or 'swift')
#if _pointerBitWidth(_32)
    ^
[...]/swift-atomics/Sources/Atomics/Types/DoubleWord.swift:32:9: error: unexpected platform condition (expected 'os', 'arch', or 'swift')
#elseif _pointerBitWidth(_64)
        ^

We do no enable ATOMICS_NATIVE_BUILTINS on 5.8, so evidently this is because of Swift's uniquely frustrating parsing rules. :-P

lorentey commented 11 months ago

d976dd29 fixes the parsing issue on 5.8. A quick retest confirms that this does not appear to have adverse effects on 5.9 & 5.11, so not restarting the process.

lorentey commented 11 months ago

Swift 5.8 fails the -Werror test with valid complaints:

[2/21] Compiling Atomics IntegerOperations.swift
<module-includes>:1:9: note: in file included from <module-includes>:1:
#import "_AtomicsShims.h"
        ^
[...]/swift-atomics/Sources/_AtomicsShims/include/_AtomicsShims.h:215:1: error: incompatible pointer types passing '_Atomic(intptr_t) *' to parameter of type 'intptr_t *' (aka 'long *')
SWIFTATOMIC_DEFINE_INTEGER_TYPE(Int, intptr_t)
^
[...]/swift-atomics/Sources/_AtomicsShims/include/_AtomicsShims.h:207:3: note: expanded from macro 'SWIFTATOMIC_DEFINE_INTEGER_TYPE'
  SWIFTATOMIC_DEFINE_TYPE(swiftType, cType)                            \
  ^
[...]/swift-atomics/Sources/_AtomicsShims/include/_AtomicsShims.h:203:3: note: expanded from macro 'SWIFTATOMIC_DEFINE_TYPE'
  SWIFTATOMIC_CMPXCHG_FNS(strong, swiftType, cType)                    \
  ^
[...]/swift-atomics/Sources/_AtomicsShims/include/_AtomicsShims.h:179:3: note: expanded from macro 'SWIFTATOMIC_CMPXCHG_FNS'
  SWIFTATOMIC_CMPXCHG_FN(kind, swiftType, cType, relaxed, relaxed)      \
  ^
[...]/swift-atomics/Sources/_AtomicsShims/include/_AtomicsShims.h:142:7: note: expanded from macro 'SWIFTATOMIC_CMPXCHG_FN'
      &expected->value,                                                 \
      ^
lorentey commented 11 months ago

Oh hm, it looks like we won't be able to properly unify the two low-level primitive implementations -- they will need to produce different operations. The fix is in commit 48fad5a2.

lorentey commented 11 months ago

Swift 5.8 on Intel macOS also has trouble with library evolution enabled:

/Users/klorentey/Library/Developer/Xcode/DerivedData/swift-atomics-dlyapsytwcmzwnderwymccydfvuw/Build/Intermediates.noindex/swift-atomics.build/Debug/Atomics.build/Objects-normal/arm64/Atomics.private.swiftinterface:5:8: error: no such module '_AtomicsShims'
import _AtomicsShims
       ^
/Users/klorentey/Library/Developer/Xcode/DerivedData/swift-atomics-dlyapsytwcmzwnderwymccydfvuw/Build/Intermediates.noindex/swift-atomics.build/Debug/Atomics.build/Objects-normal/arm64/Atomics.private.swiftinterface:1:1: error: failed to verify module interface of 'Atomics' due to the errors above; the textual interface may be broken by project issues or a compiler bug
// swift-interface-format-version: 1.0
^
lorentey commented 11 months ago

The .xcodeproj only supports building Swift 5.9, as it unconditionally configures the project to use native builtins. (This is by design.)

RobFrancisAu commented 11 months ago

The xcodebuild.library-evolution build is failing to import the _AtomicShims module on Xcode 15 (iOS). This smells like the known issue with XCBuild's dependency management for C modules.

This package isn't designed to be built with library evolution enabled, so I'm not treating this as a blocker.

[22 xcodebuild.library-evolution] xcodebuild -scheme swift-atomics -destination generic/platform=macOS -destination generic/platform=iOS BUILD_LIBRARY_FOR_DISTRIBUTION=YES
SwiftVerifyEmittedModuleInterface normal arm64 Verifying\ emitted\ module\ interface\ Atomics.private.swiftinterface /Users/klorentey/Library/Developer/Xcode/DerivedData/swift-atomics-fflkxpmrsggtlnbcxorrecorsseo/Build/Intermediates.noindex/swift-atomics.build/Debug/Atomics.build/Objects-normal/arm64/Atomics.private.swiftinterface (in target 'Atomics' from project 'swift-atomics')
...
/Users/klorentey/Library/Developer/Xcode/DerivedData/swift-atomics-fflkxpmrsggtlnbcxorrecorsseo/Build/Intermediates.noindex/swift-atomics.build/Debug/Atomics.build/Objects-normal/arm64/Atomics.private.swiftinterface:8:8: error: no such module '_AtomicsShims'
import _AtomicsShims
       ^

I am using Atomics in a framework that is being build as an XCFramework which is used by others.

Is there any way around this if you need BUILD_LIBRARY_FOR_DISTRIBUTION = Yes ?

lorentey commented 11 months ago

The most satisfying solution would be to remove the _AtomicShims module altogether -- unfortunately that'll have to wait for a while, as we still need the swift_retain_n/swift_release_n wrappers.

Until then, we're sort of wedged -- IIRC the problem is that the C headers don't get properly embedded in the xcframework, but the swift module needs to publicly import them.

Wait... in 5.9 we could actually hide the import. 🤔 There may be a solution!

lorentey commented 11 months ago

Ah, just to clarify -- @RobFrancisAu, are you currently doing this with swift-atomic 1.1 and Xcode 14.3?

(I'm asking because I saw the same failures before the 1.1 release earlier this year -- so it is quite possible that the test script's failure will not interfere with your actual use case! (E.g. you're probably building atomics as a dependency, not directly as a top-level thing.)🤞)

lorentey commented 11 months ago

Using @_implementationOnly import _AtomicsShims in the Swift 5.9 configuration resolves the test failure, but it introduces a potential new problem, as @_implementationOnly isn't designed to be used in projects that aren't ABI stable. (The upcoming internal import feature would work, but it's not shipping yet.)

Given that the failure isn't new (and assuming that it doesn't interfere with using swift-atomics as an internal dependency of a .xcframework), I think I'll continue ignoring it for now. I expect we'll be able to eliminate the entire _AtomicsShims module for Swift 5.10 (or 5.11), ridding us of all these issues.

RobFrancisAu commented 11 months ago

Ah, just to clarify -- @RobFrancisAu, are you currently doing this swift-atomic 1.1 and Xcode 14.3?

(I'm asking because I saw the same failures before the 1.1 release earlier this year -- so it is quite possible that the test script's failure will not interfere with your actual use case! (E.g. you're probably building atomics as a dependency, not directly as a top-level thing.)🤞)

Firstly, thanks for your help @lorentey.

Yes using 1.1.0 Which seems to be working fine. My project builds without an issue on my machine.

The failure is occurring in the my CICD pipeline when trying to archive whilst including BUILD_LIBRARY_FOR_DISTRIBUTION=YES

.... IntermediateBuildFilesPath/swift-atomics.build/Release-iphoneos/Atomics.build/Objects-normal/arm64/Atomics.swiftinterface:5:8: no such module '_AtomicsShims'

The following build commands failed:

  SwiftVerifyEmittedModuleInterface normal arm64 Verifying\ emitted\ module\ interface\ Atomics.private.swiftinterface ..... /IntermediateBuildFilesPath/swift-atomics.build/Release-iphoneos/Atomics.build/Objects-normal/arm64/Atomics.private.swiftinterface (in target 'Atomics' from project 'swift-atomics')
lorentey commented 11 months ago

Terrific, Swift 5.7 doesn't like seeing _pointerBitWidth conditionals, even after a failing conjunction:

[2/20] Compiling Atomics UnsafeAtomic.swift
[...]/swift-atomics/Sources/Atomics/Primitives/autogenerated/Primitives.native.swift:2113:24: error: unexpected platform condition (expected 'os', 'arch', or 'swift')
#if compiler(>=5.9) && _pointerBitWidth(_64)
                       ^
[...]/swift-atomics/Sources/Atomics/Primitives/autogenerated/Primitives.native.swift:2441:24: error: unexpected platform condition (expected 'os', 'arch', or 'swift')
#if compiler(>=5.9) && _pointerBitWidth(_64)
                       ^
[...]/swift-atomics/Sources/Atomics/Primitives/autogenerated/Primitives.native.swift:2444:28: error: unexpected platform condition (expected 'os', 'arch', or 'swift')
#elseif compiler(>=5.9) && _pointerBitWidth(_32)
                           ^
[...]/swift-atomics/Sources/Atomics/Types/DoubleWord.swift:16:24: error: unexpected platform condition (expected 'os', 'arch', or 'swift')
#if compiler(>=5.9) && _pointerBitWidth(_32)
                       ^
[...]/swift-atomics/Sources/Atomics/Types/DoubleWord.swift:32:28: error: unexpected platform condition (expected 'os', 'arch', or 'swift')
#elseif compiler(>=5.9) && _pointerBitWidth(_64)
                           ^
lorentey commented 11 months ago

Moving the compiler(>=5.9) conditional to a dedicated #if construct on the topmost level seems to let 5.7 parse things correctly. Fixed in fe0fc8b76c18522bddee1ef125a557db7d1d92d3.

lorentey commented 11 months ago

The failure is occurring in the my CICD pipeline when trying to archive whilst including BUILD_LIBRARY_FOR_DISTRIBUTION=YES

Oh, so it's been broken in 1.1, too. That's unfortunate!

Using @_implementationOnly import _AtomicsShims in Unmanaged extension.swift (appears to) resolve the issue, but @_implementationOnly does not work correctly unless library evolution is enabled, so doing that is somewhat risky. (I aren't aware of a way to conditionally enable/disable code based on whether we have a stable ABI. 😞)

I'll try to land that change anyway and partially rerun the tests to see if anything looks amiss. The risk of regressions is pretty high here, and the "full" tests aren't covering everything, but it's worth a try.

lorentey commented 11 months ago

@RobFrancisAu f26da16e6e67fc85c15276074b65f3f6f1333114 has the potential fix. It will only work on Swift 5.9 or better -- unfortunately we the shims module needs to be a public import on earlier toolchains.

(Sadly this may not fix all issues, but hopefully it will resolve the specific build error. Building packages with library evolution enabled can still hit other issues -- xcodebuild's package integration in particular seems to have unique issues with C modules in packages.)

lorentey commented 11 months ago

This never ends!

fe0fc8b76c18522bddee1ef125a557db7d1d92d3 surfaced a CMake build issue on 5.9. The build config failed to enable native builtins there, so CMake is trying to use C shims even on new compilers:

/Users/klorentey/Docker/swift-atomics/Sources/Atomics/Types/DoubleWord.swift:21:8: error: no such module 'Builtin'
import Builtin
       ^
/Users/klorentey/Docker/swift-atomics/Sources/Atomics/Types/DoubleWord.swift:21:8: error: no such module 'Builtin'
import Builtin
       ^
/Users/klorentey/Docker/swift-atomics/Sources/Atomics/Types/DoubleWord.swift:21:8: error: no such module 'Builtin'
import Builtin
       ^
lorentey commented 11 months ago

3431b8543fbd4ab93b02030c9480c1f62c27f5c7 (attempts to) fix the CMake configuration.

Previous commits to that restrict the Xcode project to only work with Xcode 15, and adjust the code signing configuration to allow running tests on devices.

lorentey commented 11 months ago

OK, it looks like 3431b8543fbd4ab93b02030c9480c1f62c27f5c7 may be a workable commit.