Skip to content

[ownership] Add verification that parent borrow scopes completely enclose implicit child borrow scopes introduced via BorrowScopeOperand #27376

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

Conversation

gottesmm
Copy link
Contributor

@gottesmm gottesmm commented Sep 26, 2019

As an example consider a begin_borrow/end_borrow scope and a coroutine:

  %0 = begin_borrow %...
  ...
  %token = apply %coroutine(%0)
  ...
  end_apply %token
  ...
  end_borrow %0

With this change, we will validate that the end_borrow never enters the region
of code until the coroutine is actually finished executing (as shown by the
end_apply).

…lose implicit child borrow scopes introduced via BorrowScopeOperand.

As an example consider a begin_borrow/end_borrow scope and a coroutine:

```
  %0 = begin_borrow %...
  ...
  %token = apply %coroutine(%0)
  ...
  end_apply %token
  ...
  end_borrow %0
```

With this change, we will validate that the end_borrow never enters the region
of code until the coroutine is actually finished executing (as shown by the
end_apply).
@gottesmm gottesmm requested a review from atrick September 26, 2019 03:51
@gottesmm
Copy link
Contributor Author

@swift-ci test

@gottesmm
Copy link
Contributor Author

@swift-ci test source compatibility

@gottesmm
Copy link
Contributor Author

Once this is in, I am going to introduce mark_dependence as an instruction like this.

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - c195f73

@gottesmm
Copy link
Contributor Author

Nice! The OS X check caught something

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - c195f73

Copy link
Contributor

@atrick atrick left a comment

Choose a reason for hiding this comment

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

lgtm

@gottesmm
Copy link
Contributor Author

@atrick Found the problem. Mandatory inlining was not handling coroutines correctly. It was inserting the end_borrows/destroys for certain values after the apply site of the coroutine, not after the end_apply, abort_apply.

Interestingly, I was just improving this code here in #27405 where I refactored out this apply site functionality (since I needed it somewhere else where begin_apply was actually not supported). I am going to add in a commit based upon that one for FullApplySites only that acts like insertAfter [see #27405] for apply and try_apply and that returns the token users of the begin_apply as insertion points for begin_apply instead.

The nice thing about this is when we add a coroutine apply function that is a terminator (which I imagine we will), all this code will be correct automagically. The beauty of having abstractions! = ).

@gottesmm
Copy link
Contributor Author

I landed the coroutine fix. I am curious if this passes now.

@gottesmm
Copy link
Contributor Author

@swift-ci test

@gottesmm
Copy link
Contributor Author

@swift-ci test source compatibility

@gottesmm
Copy link
Contributor Author

Debug source compat suite failure was a jenkins error.

@gottesmm
Copy link
Contributor Author

@swift-ci Please Test Source Compatibility Debug

@gottesmm
Copy link
Contributor Author

gottesmm commented Nov 1, 2019

Release Source Compat only failure is a known failure:

17:35:51 FAIL: Sourcery, 5.1, 20bdf6, Swift Package

I expected Debug to fail in a similar way.

@gottesmm
Copy link
Contributor Author

gottesmm commented Nov 1, 2019

Debug did fail the same way. Merging!

@gottesmm gottesmm merged commit c326016 into swiftlang:master Nov 1, 2019
@gottesmm gottesmm deleted the pr-04c94db25674845b859877b727fa7ff6504e4ec5 branch November 1, 2019 01:39
@gottesmm gottesmm changed the title [ownership] Add verification that parent borrow scopes completely enc… [ownership] Add verification that parent borrow scopes completely enclose implicit child borrow scopes introduced via BorrowScopeOperand Nov 1, 2019
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.

3 participants