[IRGen] Use Singleton metadata strategy in JIT mode. #25504

jirid · 2019-06-16T03:25:58Z

In Xcode 10.2.1, the Swift compiler crashes when interpreting https://github.com/JiriTrecak/Laurine/blob/master/LaurineGenerator.swift. I have reduced the problem and am adding it as a test case. The swift compiler built from the master branch at the time of writing fails this test. My change makes this test pass and does not brake any other tests (on my machine).

The fix approach is based on work I have done on try! Swift San Jose and would like to thank Slava Pestov for providing explanation and ideas how to fix this.

This commit: jirid@d2f1d36
shows the original fix recommended by Slava, unfortunately that did break the following tests:
IRGen/playground.swift
Interpreter/imported_objc_generics.swift
IRGen/objc_generic_class_metadata.sil

Please review thoroughly as my understanding of the Swift compiler is very limited and this is my first contribution to Swift.

Resolves rdar://49639321

compnerd · 2019-06-16T04:07:29Z

test/IRGen/jit_metadata_strategy.swift

@@ -0,0 +1,39 @@
+// RUN: %target-swift-frontend -interpret %s
+// REQUIRES: executable_test


I think that this is better tested in terms of IRGen, rather than checking that the executable executes.

You could also have both.

Sure, as long as they are in separate files. The reason for preferring checking IRGen is that means that you can verify IRGen irrespective of the target that you are on. This massively helped for example in the Windows bring up where I did the entirety of the work on Linux before I started running the executable tests.

jirid · 2019-06-16T04:51:10Z

The test currently fails on Linux, because it can't import Foundation.

slavapestov · 2019-06-17T02:24:00Z

@jirid You can REQUIRES: objc_interop to ensure the test only runs on Darwin. However, I suggest looking at the way that test/Interpreter/struct_resilience.swift builds and links against test/Inputs/resilient_struct.swift. You can reproduce the original problem with a class containing any resilient struct as a field, not just a resilient struct from Foundation.

compnerd · 2019-06-20T04:31:01Z

@jirid You can REQUIRES: objc_interop to ensure the test only runs on Darwin.

No, that is wrong. I've had to (as has @yln) go through the test suite cleaning up misuse of that require.

If you want to test it on Darwin only, fine, you can do // REQUIRES: OS=macOS. However, if you need Foundation present, please check it using // REQUIRES: foundation. We have proper features to check for Foundation and libdispatch.

jirid · 2019-06-20T04:49:56Z

Thanks for the feedback, I'll look into it and update the PR.

slavapestov · 2019-06-23T01:24:21Z

@jirid Let me know if you need any help writing the IRGen tests. The fix is good to go other than that!

jirid · 2019-06-25T13:43:02Z

@slavapestov I am working on trying to create a runtime test case (crash) without using URL from Foundation and have not succeeded yet. I have successfully integrated the resilient_struct.swift into my test case, but it behaves differently than URL. Regardless of what resilient struct I use selfTI.getClassLayout(..., forBackwardDeployment=false) has the flag ClassMetadataFlags::ClassHasResilientMembers so it takes the desired branch in the compiler. But selfTI.getClassLayout(..., forBackwardDeployment=true) has the same flag for any of the resilient structs but does not have the flag for URL. The resilient structs use the ClassMetadataStrategy::Singleton strategy even in the master branch of the compiler. So I need to design a type that has resiliently sized fields when using the resilient layout but does not have resiliently sized fields when using the fragile layout forBackwardDeployment. I don't seem to be able to figure out what this property means and how to design a type which has it. Could you please provide some hints?

I have not started looking into designing a test case in terms of IRGen.

slavapestov · 2019-07-03T22:23:23Z

@jirid Sorry I forgot about that part. Grep the existing tests for -read-legacy-type-info-path. You need to create a YAML file with the layout of the resilient struct to use on old deployment targets. Pick a resilient type like ResilientDouble in test/Inputs/resilient_struct.swift that has the same size on all targets.

slavapestov · 2019-07-03T22:24:04Z

If the tests pass, we can merge this now since I'd like to put the fix into swift-5.1-branch as well. You can decouple the test cases from Foundation in a follow-up PR if you want.

slavapestov · 2019-07-03T22:25:23Z

@swift-ci Please test

swift-ci · 2019-07-04T00:03:31Z

Build failed
Swift Test OS X Platform
Git Sha - 6a75a6cbe7d8beaa14f400e025d924c804960e32

jirid · 2019-07-04T01:06:36Z

17:01:42  FAIL: Swift(iphonesimulator-i386) :: IRGen/jit_metadata_strategy_runtime.swift (10922 of 12123)
17:01:42  ******************** TEST 'Swift(iphonesimulator-i386) :: IRGen/jit_metadata_strategy_runtime.swift' FAILED ********************
17:01:42  Script:
17:01:42  --
17:01:42  : 'RUN: at line 1';   /Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/buildbot_incremental/swift-macosx-x86_64/bin/swiftc -frontend -target i386-apple-ios7.0  -module-cache-path '/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/buildbot_incremental/swift-macosx-x86_64/swift-test-results/i386-apple-ios7.0/clang-module-cache' -sdk '/Applications/Xcode-beta.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator13.0.sdk' -swift-version 4   -typo-correction-limit 10  -interpret /Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/swift/test/IRGen/jit_metadata_strategy_runtime.swift
17:01:42  --
17:01:42  Exit Code: 132
17:01:42  
17:01:42  Command Output (stderr):
17:01:42  --
17:01:42  /Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/buildbot_incremental/swift-macosx-x86_64/test-iphonesimulator-i386/IRGen/Output/jit_metadata_strategy_runtime.swift.script: line 1: 61029 Illegal instruction: 4  /Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/buildbot_incremental/swift-macosx-x86_64/bin/swiftc -frontend -target i386-apple-ios7.0 -module-cache-path '/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/buildbot_incremental/swift-macosx-x86_64/swift-test-results/i386-apple-ios7.0/clang-module-cache' -sdk '/Applications/Xcode-beta.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator13.0.sdk' -swift-version 4 -typo-correction-limit 10 -interpret /Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/swift/test/IRGen/jit_metadata_strategy_runtime.swift
17:01:42  
17:01:42  --
17:01:42  
17:01:42  ********************

Test passes on macOS but fails on iPhone simulator. I'll investigate.

slavapestov · 2019-07-04T01:39:12Z

test/IRGen/jit_metadata_strategy_runtime.swift

@@ -0,0 +1,40 @@
+// RUN: %target-swift-frontend -interpret %s
+// REQUIRES: executable_test
+// REQUIRES: foundation


You should use REQUIRES: OS=macosx. I don't think simulator or device testing supports the JIT.

This one is REQUIRES: swift_interpreter.

Ah, nice. I'll fix that up and cherry-pick this to the 5.1 branch.

FixedOrUpdate strategy does not work for non-generic classes in JIT mode. Fixes rdar://49639321

slavapestov · 2019-07-08T16:17:00Z

@swift-ci Please test and merge

slavapestov · 2019-07-08T16:38:59Z

@swift-ci Please test

swift-ci · 2019-07-08T16:41:06Z

Build failed
Swift Test Linux Platform
Git Sha - 6a75a6cbe7d8beaa14f400e025d924c804960e32

swift-ci · 2019-07-08T16:42:32Z

Build failed
Swift Test OS X Platform
Git Sha - 6a75a6cbe7d8beaa14f400e025d924c804960e32

compnerd reviewed Jun 16, 2019

View reviewed changes

slavapestov self-assigned this Jun 23, 2019

jirid force-pushed the fix-jit-metadata-strategy branch from b3f9624 to 6a75a6c Compare June 27, 2019 02:44

slavapestov reviewed Jul 4, 2019

View reviewed changes

[IRGen] Use Singleton metadata strategy in JIT mode.

8150a01

FixedOrUpdate strategy does not work for non-generic classes in JIT mode. Fixes rdar://49639321

jirid force-pushed the fix-jit-metadata-strategy branch from 6a75a6c to 8150a01 Compare July 5, 2019 22:46

slavapestov merged commit ef0f7dc into swiftlang:master Jul 8, 2019

slavapestov mentioned this pull request Jul 8, 2019

[IRGen] Use Singleton metadata strategy in JIT mode [5.1] #26002

Merged

		@@ -0,0 +1,39 @@
		// RUN: %target-swift-frontend -interpret %s
		// REQUIRES: executable_test

[IRGen] Use Singleton metadata strategy in JIT mode. #25504

[IRGen] Use Singleton metadata strategy in JIT mode. #25504

Uh oh!

Conversation

jirid commented Jun 16, 2019

Uh oh!

compnerd Jun 16, 2019

Choose a reason for hiding this comment

Uh oh!

slavapestov Jun 17, 2019

Choose a reason for hiding this comment

Uh oh!

compnerd Jun 20, 2019

Choose a reason for hiding this comment

Uh oh!

jirid commented Jun 16, 2019

Uh oh!

slavapestov commented Jun 17, 2019

Uh oh!

compnerd commented Jun 20, 2019

Uh oh!

jirid commented Jun 20, 2019

Uh oh!

slavapestov commented Jun 23, 2019

Uh oh!

jirid commented Jun 25, 2019

Uh oh!

slavapestov commented Jul 3, 2019

Uh oh!

slavapestov commented Jul 3, 2019

Uh oh!

slavapestov commented Jul 3, 2019

Uh oh!

swift-ci commented Jul 4, 2019

Uh oh!

jirid commented Jul 4, 2019

Uh oh!

slavapestov Jul 4, 2019

Choose a reason for hiding this comment

Uh oh!

jrose-apple Jul 8, 2019

Choose a reason for hiding this comment

Uh oh!

slavapestov Jul 8, 2019

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Jul 8, 2019

Uh oh!

slavapestov commented Jul 8, 2019

Uh oh!

swift-ci commented Jul 8, 2019

Uh oh!

swift-ci commented Jul 8, 2019

Uh oh!

Uh oh!