Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

import Atomics won't compile in Playgrounds #55

Closed
2 tasks done
rvsrvs opened this issue Aug 7, 2022 · 8 comments · Fixed by #97
Closed
2 tasks done

import Atomics won't compile in Playgrounds #55

rvsrvs opened this issue Aug 7, 2022 · 8 comments · Fixed by #97
Labels
bug Something isn't working

Comments

@rvsrvs
Copy link

rvsrvs commented Aug 7, 2022

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

  • If possible, I've reproduced the issue using the main branch of this package.
  • 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 rvsrvs added the bug Something isn't working label Aug 7, 2022
@rvsrvs
Copy link
Author

rvsrvs commented Aug 9, 2022

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.

@lorentey
Copy link
Member

lorentey commented Dec 9, 2022

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.

@rvsrvs
Copy link
Author

rvsrvs commented Dec 11, 2022

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.

@lorentey
Copy link
Member

Hm. This package needs these two functions only to work around swiftlang/swift#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
Copy link
Member

Planning for getting rid of the C module has started in #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 lorentey linked a pull request Aug 10, 2023 that will close this issue
6 tasks
@lorentey
Copy link
Member

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

@lorentey
Copy link
Member

#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
Copy link
Member

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.

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