[Exclusivity] Enforce exclusive access for class offsets in KeyPaths #15502

devincoughlin · 2018-03-25T21:36:41Z

Add dynamic enforcement of exclusive access when a KeyPath directly accesses a final
stored property on an instance of a class. For read-only projections, this begins and ends
the access immediately. For mutable projections, this uses the ClassHolder to perform
a long-term access that lasts as long as the lifetime of the ClassHolder.

rdar://problem/31972680

atrick

Awesome!

atrick · 2018-03-25T22:21:56Z

stdlib/public/core/KeyPath.swift

+
+    // Tail allocate the UnsafeValueBuffer used as the AccessRecord.
+    // This avoids a second heap allocation since we cannot have a
+    // stored property of Builtin.UnsafeValueBuffer type.


There is no Swift-level way to initialize a value of UnsafeValueBuffer type, so the initializer doesn't pass DI. I'll update the comment with more information.

atrick · 2018-03-25T22:27:24Z

stdlib/public/core/KeyPath.swift

+                                                          1._builtinWordValue,
+                                                          AccessRecord.self)
+
+    withUnsafeMutablePointer(to: &holder.previous) {


I guess this is more a question for @jckarter. Why did we drop down to withUnsafeMutablePointer, instead of just holding on to an Unmanaged reference?

I don't see any reason we couldn't use Unmanaged and manual reference counting here. Are there undesired interactions between the exclusivity calls and the implicit refcounting at the unmanaged-managed boundaries?

Nevermind, it's obvious now that I look at this again, the ClassHolder is now allocated with a builtin. There's just no way to get the memory in an initialized state without using an unsafe pointer.

atrick · 2018-03-25T22:43:44Z

test/Interpreter/enforce_exclusive_access_backtrace.swift

+// Key path accesses are performed in the Standard Library. The Standard Library
+// is currently compiled in Swift 3 mode and the compiler currently logs conflicts
+// (rather than trapping on them) code compiled in Swift 3 mode. For this reason
+// conflicts where the second conflicting access is via a kKeyey path will log rather than


typo: kKeyey

devincoughlin · 2018-03-31T17:19:49Z

Updated to add additional comments about allocation and initialization of ClassHolder instances. Also rebased on top of master.

devincoughlin · 2018-03-31T17:20:08Z

@swift-ci Please smoke test and merge

Add dynamic enforcement of exclusive access when a KeyPath directly accesses a final stored property on an instance of a class. For read-only projections, this begins and ends the access immediately. For mutable projections, this uses the ClassHolder to perform a long-term access that lasts as long as the lifetime of the ClassHolder. rdar://problem/31972680

devincoughlin · 2018-03-31T19:00:56Z

Rebase on top of @atrick 's recent 'noNestedConflict' changes. In SILGenBuiltin.cpp I've made emitBuiltinBeginUnpairedModifyAccess() have 'noNestedConflict' set to false and emitBuiltinPerformInstantaneousReadAccess() have it set to true. @atrick does this seem right to you?

devincoughlin · 2018-03-31T19:01:14Z

@swift-ci please smoke test

atrick · 2018-03-31T23:05:17Z

@devincoughlin that looks good to me.

atrick · 2018-03-31T23:29:27Z

@swift-ci smoke test OS X Platform.

atrick · 2018-03-31T23:46:31Z

@devincoughlin you can either set noNestedAccess to false and let the future optimization handle it, or wait until this PR is merged and retest: #15659

atrick · 2018-04-01T00:45:41Z

@swift-ci smoke test.

devincoughlin · 2018-04-01T03:01:52Z

@swift-ci Please smoke test

atrick · 2018-04-01T03:50:32Z

Interpreter/enforce_exclusive_access.swift

19:58:09 [ RUN      ] ExclusiveAccess.KeyPathInoutDirectWriteClassStoredProp
19:58:09 expecting a crash, but the test did not crash
19:58:09 [     FAIL ] ExclusiveAccess.KeyPathInoutDirectWriteClassStoredProp
19:58:09 [ RUN      ] ExclusiveAccess.KeyPathInoutDirectReadClassStoredProp
19:58:09 expecting a crash, but the test did not crash
19:58:09 [     FAIL ] ExclusiveAccess.KeyPathInoutDirectReadClassStoredProp

So, the IRGen "fix" I mentioned above,
#15659, should not have changed any functionality. Skipping the endAccess runtime call is purely an optimization (which is why I originally lumped it in with my
optimization pass). If the beginAccess isn't tracked, then the scratch buffer will be invalid and endAccess will do nothing.

What's more, I don't think the no_nested_conflict changes should have any effect on the KeyPathInoutDirectWriteClassStoredProp test that is failing (there's no instantaneous access in that test).

So, was the test ever passing? I looked into it and noticed that it requires the standard library to emit the unpaired access markers. We had never intended to enable access markers in the standard library because, once it's been tested, there's no point shipping it with extra checks. This is generally the right strategy, but doesn't work for the Builtin checks, which actually form an outer access scope surrounding user code.

I'm tempted to suggest handling the Builtins specially, and unconditionally emitting markers. But first we should consider whether there are other instances of this issue. Maybe the solution needs to
be generalized. As long as the stdlib doesn't instantiate internal classes with materializeForSet properties, I don't think this problem will show up elsewhere. @rjmccall can you think of any other places that we need to emit stdlib checks to pick up on user violations?

devincoughlin · 2018-04-01T04:09:47Z

Oof. That explains why the test passes locally. I'm using a debug version of the standard library.

rjmccall · 2018-04-02T05:59:01Z

I would hope that the standard library generally doesn't need dynamic checks because it can just use static enforcement. So the approach I would suggest would be to look at all the dynamic checks that we do emit and either (1) decide they're statically okay (and disable them with an attribute?) or (2) leave them in as dynamic checks. A base policy of disabling all the checks feels like going the wrong way about it.

atrick · 2018-04-12T01:03:24Z

@gottesmm plans to add a function-level @semantics attribute to disable access marker elimination (i.e. force them to be emitted). That should unblock this fix.
rdar://problem/39335800 Add a function attribute to KeyPath to force exclusivity enforcement.

…ename it to IRGenPrepare. I am going to be adding logic here to enable swiftlang#1550 to be completed. The rename makes sense due to precedent from LLVM's codegen prepare and also since I am going to be expanding what the pass is doing beyond just "cleaning up". It is really a grab bag pass for performing simple transformations that we do not want to pollute IRGen's logic with. swiftlang#15502 rdar://39335800

gottesmm · 2018-04-17T08:32:50Z

@devincoughlin I am going to chop off a part of this commit since I need it to test my semantics thing. Specifically I am just going to cut off the builtin emission part.

atrick · 2018-04-17T20:13:40Z

@devincoughlin, Since @gottesmm is testing this in conjunction with his changes, he'll probably end of staging it back in as two parts it in new PRs.

…ename it to IRGenPrepare. I am going to be adding logic here to enable swiftlang#1550 to be completed. The rename makes sense due to precedent from LLVM's codegen prepare and also since I am going to be expanding what the pass is doing beyond just "cleaning up". It is really a grab bag pass for performing simple transformations that we do not want to pollute IRGen's logic with. swiftlang#15502 rdar://39335800

atrick · 2018-04-23T16:51:13Z

See #16051..

devincoughlin requested review from jckarter, rjmccall and atrick March 25, 2018 21:37

atrick approved these changes Mar 25, 2018

View reviewed changes

devincoughlin force-pushed the exclusivity-keypaths-class-offset branch 2 times, most recently from 6305429 to f062d6a Compare March 31, 2018 17:18

devincoughlin force-pushed the exclusivity-keypaths-class-offset branch from f062d6a to d1757fc Compare March 31, 2018 18:57

gottesmm mentioned this pull request Apr 17, 2018

SILCleanup => IRGenPrepare. #15973

Merged

atrick mentioned this pull request Apr 20, 2018

[Exclusivity] Enforce exclusive access for class offsets in KeyPaths #16051

Merged

atrick closed this Apr 23, 2018

[Exclusivity] Enforce exclusive access for class offsets in KeyPaths #15502

[Exclusivity] Enforce exclusive access for class offsets in KeyPaths #15502

Uh oh!

Conversation

devincoughlin commented Mar 25, 2018

Uh oh!

atrick left a comment

Choose a reason for hiding this comment

Uh oh!

atrick Mar 25, 2018

Choose a reason for hiding this comment

Uh oh!

devincoughlin Mar 26, 2018

Choose a reason for hiding this comment

Uh oh!

atrick Mar 25, 2018

Choose a reason for hiding this comment

Uh oh!

jckarter Mar 26, 2018

Choose a reason for hiding this comment

Uh oh!

atrick Mar 26, 2018

Choose a reason for hiding this comment

Uh oh!

atrick Mar 25, 2018

Choose a reason for hiding this comment

Uh oh!

devincoughlin commented Mar 31, 2018

Uh oh!

devincoughlin commented Mar 31, 2018

Uh oh!

devincoughlin commented Mar 31, 2018

Uh oh!

devincoughlin commented Mar 31, 2018

Uh oh!

atrick commented Mar 31, 2018

Uh oh!

atrick commented Mar 31, 2018

Uh oh!

atrick commented Mar 31, 2018

Uh oh!

atrick commented Apr 1, 2018

Uh oh!

devincoughlin commented Apr 1, 2018

Uh oh!

atrick commented Apr 1, 2018

Uh oh!

devincoughlin commented Apr 1, 2018

Uh oh!

rjmccall commented Apr 2, 2018

Uh oh!

atrick commented Apr 12, 2018

Uh oh!

gottesmm commented Apr 17, 2018

Uh oh!

atrick commented Apr 17, 2018

Uh oh!

atrick commented Apr 23, 2018

Uh oh!

Uh oh!