Introduce a backward-deployment library for SR-10600. #25030

jckarter · 2019-05-23T22:08:46Z

Build a static archive that can be linked into executables and take advantage of the Swift runtime's
hooking mechanism to work around the issue Doug fixed in #24759.
The Swift 5.0 version of swift_conformsToProtocol would return a false negative in some cases where
a subclass conforms using an inherited conformance, so work around this by successively retrying
the original implementation up the superclass chain to try to find a match.

rdar://problem/50057445

jckarter · 2019-05-23T22:09:28Z

stdlib/public/Compatibility50/CMakeLists.txt

+set(swift_runtime_compile_flags ${SWIFT_RUNTIME_CORE_CXX_FLAGS})
+set(swift_runtime_linker_flags ${SWIFT_RUNTIME_CORE_LINK_FLAGS})
+
+if(SWIFT_BUILD_SDK_OVERLAY)


@gottesmm What's the best way to set up the CMake file so this only builds for Darwin targets?

Provide an explicit list of target SDKs, like the Platform overlay does.

I would just do:

add_swift_target_library(swiftCompatibility50 TARGET_LIBRARY STATIC ProtocolConformance.cpp Overrides.cpp TARGET_SDKS ${SWIFT_APPLE_PLATFORMS} C_COMPILE_FLAGS ${swift_runtime_library_compile_flags} LINK_FLAGS ${swift_runtime_linker_flags} INSTALL_IN_COMPONENT stdlib)

DougGregor

The change looks great, but can we make sure to test backward deployment?

test/Interpreter/SR-10600.swift

lib/Driver/DarwinToolChains.cpp

jrose-apple · 2019-05-24T00:17:14Z

lib/Driver/DarwinToolChains.cpp

+
+    if (llvm::sys::fs::exists(BackDeployLib)) {
+      Arguments.push_back("-force_load");
+      Arguments.push_back(context.Args.MakeArgString(BackDeployLib));


Should this only happen in executable targets, or everywhere? What happens when there's a zillion copies of this section in the final app?

It only takes effect in the executable. I should change it so we only link it (by default) into executables.

jrose-apple · 2019-05-24T00:18:04Z

stdlib/public/Compatibility50/Overrides.h

+  const WitnessTable *(const Metadata *, const ProtocolDescriptor *);
+
+const WitnessTable *
+swiftoverride_conformsToProtocol(const Metadata * const type,


You probably shouldn't give this an exported symbol at all.

It should be hidden visibility by default. We build with -fvisibility=hidden.

Sure, but since it's a static library…it can be even better. Even if you're supposed to only get one copy of it.

I suppose I could lump the whole thing into one .o file, but that wouldn’t scale very well if we add more back deployment stuff. What did you have in mind?

Hm. I had been thinking that you'd have a different override section for each entry point, and then none of them need to be exposed, but maybe that wouldn't work, or maybe it'd be a clunky solution anyway. I guess it's fine this way.

It would be a good idea to more strongly namespace these as swift50override_, in case a future 5.1 compatibility library wants to provide a different hook for the same entry point.

jrose-apple · 2019-05-24T00:18:58Z

stdlib/public/Compatibility50/CMakeLists.txt

+set(swift_runtime_compile_flags ${SWIFT_RUNTIME_CORE_CXX_FLAGS})
+set(swift_runtime_linker_flags ${SWIFT_RUNTIME_CORE_LINK_FLAGS})
+
+if(SWIFT_BUILD_SDK_OVERLAY)


Provide an explicit list of target SDKs, like the Platform overlay does.

jrose-apple · 2019-05-24T00:19:51Z

lib/Driver/DarwinToolChains.cpp

+  } else {
+    backDeployToSwift50 = false;
+  }
+  if (backDeployToSwift50) {


Nitpick: can you provide a flag to explicitly turn this on or off, with the default of the deployment target check? We've found that useful for other support libraries.

gottesmm · 2019-05-24T00:23:27Z

lib/Driver/DarwinToolChains.cpp

+    Triple.getMacOSXVersion(Major, Minor, Micro);
+    backDeployToSwift50 = Minor <= 14;
+  } else if (Triple.isiOS()) {
+    Triple.getiOSVersion(Major, Minor, Micro);


tvOS is a kind of iOS according to llvm::Triple. :-(

That should be a comment.

gottesmm · 2019-05-24T00:23:53Z

stdlib/public/Compatibility50/CMakeLists.txt

+set(swift_runtime_compile_flags ${SWIFT_RUNTIME_CORE_CXX_FLAGS})
+set(swift_runtime_linker_flags ${SWIFT_RUNTIME_CORE_LINK_FLAGS})
+
+if(SWIFT_BUILD_SDK_OVERLAY)


omochi · 2019-05-24T03:26:42Z

Is this first opportunity to use __swift_hook, __swift51_hook which prepared before?

jckarter · 2019-05-24T15:09:29Z

Yeah, this is the first time we're exercising the hooks.

jrose-apple · 2019-05-24T15:33:55Z

Thought: the library should be called Compatibility51 (or actually "5.1"), not 50, because it brings older runtimes up to 5.1. Unless you're planning to make slices based on deployment target?

Is there a test that the hooks are not used on a newer runtime?

jckarter · 2019-05-24T15:53:30Z

I think it's better to name these libraries based on the runtime version they target. The hooks vector is versioned in the section name—Swift 5.1 looks for "__swift51_hooks" instead of "__swift_hooks". For Swift 5.2 or 6.0 or future versions, we might want to link in a 5.0 and 5.1 compatibility library if we're deploying back to Swift 5.0 runtimes.

jrose-apple · 2019-05-24T16:10:57Z

Got it. I'm a little concerned about the number of these hook sections increasing without bound for someone with a broad backwards-deployment story, but compared to the overall size of an app it's almost certainly not a big deal.

Build a static archive that can be linked into executables and take advantage of the Swift runtime's hooking mechanism to work around the issue Doug fixed in swiftlang#24759. The Swift 5.0 version of swift_conformsToProtocol would return a false negative in some cases where a subclass conforms using an inherited conformance, so work around this by successively retrying the original implementation up the superclass chain to try to find a match.

jckarter · 2019-05-24T16:30:06Z

@swift-ci Please test

jckarter · 2019-05-24T16:30:23Z

@jrose-apple @gottesmm @compnerd How's this look now?

swift-ci · 2019-05-24T16:32:18Z

Build failed
Swift Test Linux Platform
Git Sha - 9dd1881696865b8d0b835f654dad8200eaeb82da

swift-ci · 2019-05-24T16:33:57Z

Build failed
Swift Test OS X Platform
Git Sha - 9dd1881696865b8d0b835f654dad8200eaeb82da

jckarter · 2019-05-24T17:25:48Z

@swift-ci Please test

swift-ci · 2019-05-24T17:28:06Z

Build failed
Swift Test Linux Platform
Git Sha - 66ca52c73fdeaf1b6fcb96f7e4e6fced69c90710

swift-ci · 2019-05-24T17:30:11Z

Build failed
Swift Test OS X Platform
Git Sha - 66ca52c73fdeaf1b6fcb96f7e4e6fced69c90710

…t 5.0 runtimes

jckarter · 2019-05-24T19:44:44Z

@swift-ci Please test

swift-ci · 2019-05-24T19:46:56Z

Build failed
Swift Test Linux Platform
Git Sha - 4cd4fe1002c92f784253059a9cd8c8fee93625bf

swift-ci · 2019-05-24T19:48:24Z

Build failed
Swift Test OS X Platform
Git Sha - 4cd4fe1002c92f784253059a9cd8c8fee93625bf

jrose-apple

Driver stuff looks okay. I have a few minor comments but they don't need to block the change.

jrose-apple · 2019-05-24T22:21:54Z

lib/Driver/DarwinToolChains.cpp

@@ -221,9 +221,46 @@ static bool wantsObjCRuntime(const llvm::Triple &triple) {
  llvm_unreachable("unknown Darwin OS");
 }

+/// Return the earliest backward deployment compatibility version we need to
+/// link in for the given target triple, if any.
+static Optional<std::pair<unsigned, unsigned>>


Nitpick: can you use llvm::VersionTuple for this? Bigger, sure, but also obviously a version. (And has an "empty" representation, though the Optional is fine too.)

jrose-apple · 2019-05-24T22:24:41Z

lib/Driver/DarwinToolChains.cpp

+    } else if (value.equals("none")) {
+      runtimeCompatibilityVersion = None;
+    } else {
+      // TODO: diagnose unknown runtime compatibility version?


:-( Yeah, this would have to be somewhere earlier; we currently don't diagnose from command generation.

jckarter · 2019-05-24T22:29:31Z

@jrose-apple @aciidb0mb3r Hm, it looks like a couple of swiftpm tests seem to link pure C/ObjC targets using swiftc in a way that doesn't link against libswiftCore.dylib, e.g.:

14:52:21 Test Case '-[FunctionalTests.CFamilyTargetTestCase testObjectiveCPackageWithTestTarget]' started.
14:52:21 
/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/swiftpm/Tests/FunctionalTests/CFamilyTargetTests.swift:84: error: -[FunctionalTests.CFamilyTargetTestCase testObjectiveCPackageWithTestTarget] : failed - `swift test' failed:
14:52:21 
14:52:21 executionFailure(error: terminated(1): /Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/buildbot_incremental/swiftpm-macosx-x86_64/x86_64-apple-macosx/release/swift-test --package-path /var/folders/_8/79jmzf2142z2xydc_01btlx00000gn/T/CFamilyTargets_ObjCmacOSPackage.WymNVg/CFamilyTargets_ObjCmacOSPackage output:
14:52:21     [1/2] Compiling ObjCmacOSPackageTests HelloWorldTest.m
14:52:21     Undefined symbols for architecture x86_64:
14:52:21       "_swift_getObjCClassMetadata", referenced from:
14:52:21           __ZN5swift34swift50override_conformsToProtocolEPKNS_14TargetMetadataINS_9InProcessEEEPKNS_24TargetProtocolDescriptorIS1_EEPFPKNS_18TargetWitnessTableIS1_EES4_S8_E in libswiftCompatibility50.a(ProtocolConformance.cpp.o)
14:52:21     ld: symbol(s) not found for architecture x86_64
14:52:21     <unknown>:0: error: link command failed with exit code 1 (use -v to see invocation)
14:52:21     [1/2] Linking ObjCmacOSPackagePackageTests
14:52:21     , output: "[1/2] Compiling ObjCmacOSPackageTests HelloWorldTest.m\nUndefined symbols for architecture x86_64:\n  \"_swift_getObjCClassMetadata\", referenced from:\n      __ZN5swift34swift50override_conformsToProtocolEPKNS_14TargetMetadataINS_9InProcessEEEPKNS_24TargetProtocolDescriptorIS1_EEPFPKNS_18TargetWitnessTableIS1_EES4_S8_E in libswiftCompatibility50.a(ProtocolConformance.cpp.o)\nld: symbol(s) not found for architecture x86_64\n<unknown>:0: error: link command failed with exit code 1 (use -v to see invocation)\n[1/2] Linking ObjCmacOSPackagePackageTests\n", stderr: "")
14:52:21 
14:52:21 Test Case '-[FunctionalTests.CFamilyTargetTestCase testObjectiveCPackageWithTestTarget]' failed (2.562 seconds).

Does the swift driver detect that there are no Swift source inputs and leave swiftCore out of the link, or is swiftpm doing that? I could make it so that we don't link the compatibility hooks if we don't link the runtime.

aciidgh · 2019-05-24T23:30:44Z

It must be the driver since we don’t pass anything to indicate the language when linking. /cc @jrose

jckarter · 2019-05-25T01:23:47Z

Thanks @aciidb0mb3r. Jordan noted that we just rely on autolinking to get the runtime link flag. Does swiftpm know when a target is pure C? Would it be able to pass a flag to disable linking against the runtime compatibility library when building a pure C target?

aciidgh · 2019-05-25T02:13:01Z

Yep, that should be easily doable.

jckarter · 2019-05-25T16:59:02Z

Great, I'll look into that.

gottesmm · 2019-05-26T16:54:18Z

lib/Driver/DarwinToolChains.cpp

+    if (Major == 10) {
+      if (Minor <= 14) {
+        return std::make_pair(5u, 0u);
+      } else {


Can you eliminate these else returns? They don't add anything. You can remove the else.

For bonus points: invert if statements to reduce indentation.

That would become unmanageable if we add branches for more OS versions in the future.

jckarter · 2019-05-28T21:36:31Z

Please test with following pull request:
swiftlang/swift-package-manager#2134

@swift-ci Please test

swiftlang/swift#25030 introduces a compatibility library that can be statically linked into binaries in order to back-deploy runtime fixes and new features to OSes that shipped with older Swift runtimes. This library depends on the Swift runtime being linked into the executable, so it will cause link errors for pure Clang products (and even if it didn't, it would be a waste of code size). When building a product without any Swift in it, ask Swift to drive the linker without introducing any runtime compatibility libraries (and shake out a few other unnecessary linker flags while we're here). rdar://problem/50057445

Stage in part of swiftlang#25030 so that build systems can start using it.

swiftlang/swift#25030 introduces a compatibility library that can be statically linked into binaries in order to back-deploy runtime fixes and new features to OSes that shipped with older Swift runtimes. This library depends on the Swift runtime being linked into the executable, so it will cause link errors for pure Clang products (and even if it didn't, it would be a waste of code size). When building a product without any Swift in it, ask Swift to drive the linker without introducing any runtime compatibility libraries (and shake out a few other unnecessary linker flags while we're here). rdar://problem/50057445

jckarter requested review from DougGregor, jrose-apple and gottesmm May 23, 2019 22:09

jckarter commented May 23, 2019

View reviewed changes

BasThomas mentioned this pull request May 23, 2019

[135] Issue #135 - May 30, 2019 SwiftWeekly/swiftweekly.github.io#458

Closed

DougGregor approved these changes May 23, 2019

View reviewed changes

test/Interpreter/SR-10600.swift Show resolved Hide resolved

jrose-apple reviewed May 24, 2019

View reviewed changes

gottesmm reviewed May 24, 2019

View reviewed changes

jckarter force-pushed the SR-10600-back-deploy branch from 9dd1881 to 66ca52c Compare May 24, 2019 16:29

jckarter force-pushed the SR-10600-back-deploy branch from 66ca52c to 4cd4fe1 Compare May 24, 2019 17:25

Driver: Link against compatibility library for deploying back to Swif…

2a2d40d

…t 5.0 runtimes

jckarter force-pushed the SR-10600-back-deploy branch from 4cd4fe1 to 2a2d40d Compare May 24, 2019 19:44

jrose-apple approved these changes May 24, 2019

View reviewed changes

gottesmm reviewed May 26, 2019

View reviewed changes

jckarter mentioned this pull request May 28, 2019

Don't link runtime compatibility library in pure Clang targets. swiftlang/swift-package-manager#2134

Merged

jckarter merged commit 2a28948 into swiftlang:master May 29, 2019

jckarter added a commit to jckarter/swift that referenced this pull request May 29, 2019

Introduce -runtime-compatibility-version flag as a no-op.

a8e0b3c

Stage in part of swiftlang#25030 so that build systems can start using it.

jckarter mentioned this pull request May 29, 2019

[5.1] Introduce -runtime-compatibility-version flag as a no-op. #25126

Merged

drodriguez mentioned this pull request May 29, 2019

[build-script] Move HostSpecificConfiguration out of main script. #23988

Merged

omochi mentioned this pull request Dec 14, 2019

ランタイム関数の差し替え機能 waiwai-swiftc/waiwai-swiftc-ideas#8

Open

Azoy mentioned this pull request Jan 18, 2020

[SE-0283] Implement Equatable, Comparable, and Hashable conformance for Tuples #28833

Merged

jckarter mentioned this pull request May 23, 2019

[SR-10600] Runtime fails to demangle superclass when superclass's generic constraints are satisified in a conditional conformance #53000

Open

Introduce a backward-deployment library for SR-10600. #25030

Introduce a backward-deployment library for SR-10600. #25030

Conversation

jckarter commented May 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DougGregor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omochi commented May 24, 2019

jckarter commented May 24, 2019

jrose-apple commented May 24, 2019

jckarter commented May 24, 2019

jrose-apple commented May 24, 2019

jckarter commented May 24, 2019

jckarter commented May 24, 2019

swift-ci commented May 24, 2019

swift-ci commented May 24, 2019

jckarter commented May 24, 2019

swift-ci commented May 24, 2019

swift-ci commented May 24, 2019

jckarter commented May 24, 2019

swift-ci commented May 24, 2019

swift-ci commented May 24, 2019

jrose-apple left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jckarter commented May 24, 2019 • edited Loading

aciidgh commented May 24, 2019

jckarter commented May 25, 2019

aciidgh commented May 25, 2019

jckarter commented May 25, 2019

Choose a reason for hiding this comment

gottesmm May 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jckarter commented May 28, 2019

jckarter commented May 23, 2019 •

edited

Loading

jckarter commented May 24, 2019 •

edited

Loading

gottesmm May 26, 2019 •

edited

Loading