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

Workaround dyld warning about SwiftSyntax classes #4901

Merged
merged 1 commit into from
Apr 17, 2023

Conversation

keith
Copy link
Collaborator

@keith keith commented Apr 17, 2023

This uses a recent but unannounced (also read as private) feature of
dyld where it ignores duplicate Objective-C classes when they're in a
special format in the binary. https://github.com/apple-oss-distributions/dyld/blob/c8a445f88f9fc1713db34674e79b00e30723e79d/dyld/PrebuiltLoader.cpp#L1660-L1662

I think this is generally safe because hopefully people aren't actually
using the SwiftSyntax classes through the Objective-C runtime, but if
they are we'd still probably prefer to silence the noise and accept the
UB.

Fixes #4782

@SwiftLintBot
Copy link

SwiftLintBot commented Apr 17, 2023

1 Warning
⚠️ This PR may need tests.
18 Messages
📖 Linting Aerial with this PR took 1.06s vs 1.07s on main (0% faster)
📖 Linting Alamofire with this PR took 1.39s vs 1.39s on main (0% slower)
📖 Linting Brave with this PR took 7.39s vs 7.37s on main (0% slower)
📖 Linting DuckDuckGo with this PR took 3.54s vs 3.54s on main (0% slower)
📖 Linting Firefox with this PR took 9.62s vs 9.64s on main (0% faster)
📖 Linting Kickstarter with this PR took 10.32s vs 10.32s on main (0% slower)
📖 Linting Moya with this PR took 0.54s vs 0.55s on main (1% faster)
📖 Linting NetNewsWire with this PR took 3.09s vs 3.1s on main (0% faster)
📖 Linting Nimble with this PR took 0.64s vs 0.62s on main (3% slower)
📖 Linting PocketCasts with this PR took 7.34s vs 7.36s on main (0% faster)
📖 Linting Quick with this PR took 0.23s vs 0.24s on main (4% faster)
📖 Linting Realm with this PR took 11.52s vs 11.54s on main (0% faster)
📖 Linting SourceKitten with this PR took 0.44s vs 0.43s on main (2% slower)
📖 Linting Sourcery with this PR took 2.23s vs 2.22s on main (0% slower)
📖 Linting Swift with this PR took 4.71s vs 4.69s on main (0% slower)
📖 Linting VLC with this PR took 1.38s vs 1.39s on main (0% faster)
📖 Linting Wire with this PR took 8.39s vs 8.37s on main (0% slower)
📖 Linting WordPress with this PR took 11.56s vs 11.49s on main (0% slower)

Generated by 🚫 Danger

@keith keith force-pushed the ks/workaround-dyld-warning-about-swiftsyntax-classes branch from ab32ccd to 5de0031 Compare April 17, 2023 23:04
Package.swift Outdated Show resolved Hide resolved
This uses a recent but unannounced (also read as private) feature of
dyld where it ignores duplicate Objective-C classes when they're in a
special format in the binary. https://github.com/apple-oss-distributions/dyld/blob/c8a445f88f9fc1713db34674e79b00e30723e79d/dyld/PrebuiltLoader.cpp#L1660-L1662

I think this is generally safe because hopefully people aren't actually
using the SwiftSyntax classes through the Objective-C runtime, but if
they are we'd still probably prefer to silence the noise and accept the
UB.

Fixes #4782
@keith keith force-pushed the ks/workaround-dyld-warning-about-swiftsyntax-classes branch from 5de0031 to fe4e25e Compare April 17, 2023 23:31
Copy link
Collaborator

@jpsim jpsim left a comment

Choose a reason for hiding this comment

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

I dont love this solution but I don’t have any better ideas. Thanks!

@jpsim jpsim merged commit 1892c84 into main Apr 17, 2023
@jpsim jpsim deleted the ks/workaround-dyld-warning-about-swiftsyntax-classes branch April 17, 2023 23:55
@keith
Copy link
Collaborator Author

keith commented Apr 17, 2023

I think the other option is to link against Xcode's version, but that obviously has the same downside as the previous internal dylib

@jpsim
Copy link
Collaborator

jpsim commented Apr 18, 2023

Yeah and I’d rather have this workaround if it means we control the version of SwiftSyntax we ship with.

@jpsim
Copy link
Collaborator

jpsim commented Apr 18, 2023

Doesn't seem to apply to tests:

$ bazel test --test_output=streamed //Tests:GeneratedTests
WARNING: Streamed test output requested. All tests will be run locally, without sharding, one at a time
INFO: Analyzed target //Tests:GeneratedTests (0 packages loaded, 0 targets configured).
INFO: Found 1 test target...
Test Suite 'All tests' started at 2023-04-18 13:21:47.833
Test Suite 'GeneratedTests.xctest' started at 2023-04-18 13:21:47.834
Test Suite 'AccessibilityLabelForImageRuleGeneratedTests' started at 2023-04-18 13:21:47.834
Test Case '-[GeneratedTests.AccessibilityLabelForImageRuleGeneratedTests testWithDefaultConfiguration]' started.
objc[25929]: Class _TtC11SwiftSyntax16BumpPtrAllocator is implemented in both /private/var/tmp/_bazel_jsimard/7034bb326caf3934edced22f6f4dd368/execroot/_main/bazel-out/darwin_arm64-fastbuild/bin/Tests/GeneratedTests.xctest/Contents/MacOS/GeneratedTests (0x1079759c0) and /Applications/Xcode-14.3.0.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/swift/host/libSwiftSyntax.dylib (0x10a100a10). One of the two will be used. Which one is undefined.
objc[25929]: Class _TtC11SwiftSyntax35IncrementalParseReusedNodeCollector is implemented in both /private/var/tmp/_bazel_jsimard/7034bb326caf3934edced22f6f4dd368/execroot/_main/bazel-out/darwin_arm64-fastbuild/bin/Tests/GeneratedTests.xctest/Contents/MacOS/GeneratedTests (0x107975b78) and /Applications/Xcode-14.3.0.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/swift/host/libSwiftSyntax.dylib (0x10a100bc0). One of the two will be used. Which one is undefined.
objc[25929]: Class _TtC11SwiftSyntax26IncrementalParseTransition is implemented in both /private/var/tmp/_bazel_jsimard/7034bb326caf3934edced22f6f4dd368/execroot/_main/bazel-out/darwin_arm64-fastbuild/bin/Tests/GeneratedTests.xctest/Contents/MacOS/GeneratedTests (0x107975c10) and /Applications/Xcode-14.3.0.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/swift/host/libSwiftSyntax.dylib (0x10a100c58). One of the two will be used. Which one is undefined.
...

@keith
Copy link
Collaborator Author

keith commented Apr 18, 2023

Unfortunately it looks like the handling is such that only the main binary is consulted for this:

https://github.com/apple-oss-distributions/dyld/blob/c8a445f88f9fc1713db34674e79b00e30723e79d/dyld/PrebuiltLoader.cpp#L1658-L1660

My assumption is that in this case the main binary is xctest instead of our test bundle. Maybe adding a macOS test host would solve that, but that probably wouldn't be worth it :/

jpsim added a commit that referenced this pull request Jun 19, 2023
Similar to #4901 but updated with a new warning from Xcode 15 beta 1.

Unfortunately this doesn't appear to work, even with this change the
warning is still logged.

It might have something to do with how long the symbol name is.
jpsim added a commit that referenced this pull request Jun 22, 2023
Similar to #4901 but updated with a new warning from Xcode 15 beta 1.
Increase dupclass max name length from 64 to 128
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Class X is implemented in both A and B. One of the two will be used. Which one is undefined.
3 participants