swift-atomics icon indicating copy to clipboard operation
swift-atomics copied to clipboard

import Atomics won't compile in Playgrounds

Open rvsrvs opened this issue 2 years ago • 1 comments

I am trying to import swift-atomics into a Playground. This fails to compile the shims because Playgrounds don't (and as far as I can tell, won't) link libswiftCore.dylib. This is similar to Issue #8 in that its dying in the same place, unfortunately the work arounds don't work here. There is mention of using Unmanaged to do this in: SR-13708.

Given that the only use of these two functions is in:

extension Unmanaged {
  internal func retain(by delta: Int) {
    _sa_retain_n(toOpaque(), UInt32(delta))
  }

  internal func release(by delta: Int) {
    _sa_release_n(toOpaque(), UInt32(delta))
  }
}

I've been giving thought to trying to replace _sa_retain_n(void *object, uint32_t n) and void _sa_release_n(void *object, uint32_t n) with Swift versions that do what the comment in SR13708 suggests, but wanted to check and 1) see if that is actually advisable and 2) ask why the comment:

"Looping over Unmanaged.retain/release doesn't seem like a viable option, although I'm sure that would work too."

indicates both that it "doesn't seem viable" and "would work too" .

Information

  • Package version: 1.0.2
  • Platform version: MacOS 12
  • Swift version: swift-driver version: 1.62.1 Apple Swift version 5.7 (swiftlang-5.7.0.123.7 clang-1400.0.29.50) Target: arm64-apple-macosx12.0

Checklist

  • [X] If possible, I've reproduced the issue using the main branch of this package.
  • [X] I've searched for existing reports of the same issue.

Steps to Reproduce

git clone https://github.com/CSCIX65G/AtomicsExample/tree/main/Sources/AtomicsExample, try to run the Playground

Expected behavior

The playground should compile and run.

Actual behavior

The playground fails to compile

rvsrvs avatar Aug 07 '22 22:08 rvsrvs

Ok I went ahead and implemented the looping construct and now my fork passes all the tests and can run in playgrounds.

I need to run the tests on both the modified and unmodified code to try to quantify how much of a performance hit this is. It seemed noticeably slower, but that could be just that I was watching it.

As a side note, it seems like a pretty big oversight that the std lib doesn't implement these two methods in Unmanaged so that none of this is necessary in a library like swift-atomics. It also seems bad that I have to use a fork to demonstrate the use of swift-atomics in a playground.

Any thoughts or advice for other solutions are welcome.

rvsrvs avatar Aug 09 '22 21:08 rvsrvs

Looping over swift_retain/release technically doesn't break correctness, but if the loop survives into the generated binary, then the resulting code will have unacceptably bad performance. So this isn't a viable workaround, unless the relevant LLVM optimization pass replaces the loop with a direct call to swift_retain_n/swift_release_n.

lorentey avatar Dec 09 '22 02:12 lorentey

I had sort of figured that out. So I have a branch that implements it this way which I use for teaching about atomics using playgrounds and use the main branch version for actual compiled code.

rvsrvs avatar Dec 11 '22 03:12 rvsrvs

Hm. This package needs these two functions only to work around https://github.com/apple/swift/issues/56105.

Fixing that bug would allow the package to remove these, although only if it is compiled using a toolchain that includes the fix.

(The package would still need the headers, though, so this probably wouldn't help with #62. To fix that, we likely need to wait until we have native atomics in the Swift toolchain.)

lorentey avatar Mar 18 '23 04:03 lorentey

Planning for getting rid of the C module has started in https://github.com/apple/swift-atomics/pull/74. However, this won't land until some Swift compiler & stdlib work is done (and is shipping), so the current workaround will need to remain in place for a while.

lorentey avatar Mar 20 '23 19:03 lorentey

As a stopgap workaround, #95 attempts to switch to using dlsym to access these on Darwin platforms, hopefully bypassing this issue.

lorentey avatar Aug 10 '23 22:08 lorentey

#96 and #97 explore two more ways to resolve the problem: hiding the calls inside inline assembly, or using inline assembly directives to smuggle in an extra linkage.

Of the three attempts, #97 seems the least invasive, so that looks like the most promising option.

lorentey avatar Aug 11 '23 22:08 lorentey

I verified that #97 will resolve the "symbol not found" issues for _swift_retain_n and _swift_release_n.

However, I'm still seeing subsequent missing symbol issues about _sa_retain_n and _sa_release_n. These are evidently due to the build system deciding to build _AtomicsShims as a standalone framework, but then failing to link it with the client binary. It seems this is a common failure for packages with C language targets; for example, I managed to reproduce it with swift-nio's NIOConcurrencyHelpers product (which imports a C module called "CNIOAtomics", similar to "Atomics" importing "_AtomicsShims"). Unfortunately I don't know a good workaround.

lorentey avatar Aug 14 '23 23:08 lorentey