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

SIL: Verify invariants of async_continuation instructions. #34210

Merged
merged 1 commit into from
Oct 19, 2020

Conversation

jckarter
Copy link
Contributor

@jckarter jckarter commented Oct 6, 2020

  • Enforce types of continuations and resume/error BBargs for await
  • Can't access the continuation again or exit the function mid-suspend

@jckarter jckarter requested a review from rjmccall October 6, 2020 22:59
@jckarter
Copy link
Contributor Author

jckarter commented Oct 6, 2020

@swift-ci Please test

@swift-ci
Copy link
Contributor

swift-ci commented Oct 7, 2020

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

@swift-ci
Copy link
Contributor

swift-ci commented Oct 7, 2020

Build failed
Swift Test Linux Platform
Git Sha - 4f872bfe8134cdadd62254591322c68abf396186

isAddressForm = false;
throws = gaci->throws();
loweredResumeTy = gaci->getLoweredResumeType();
} else if (auto gacia = dyn_cast<GetAsyncContinuationAddrInst>(cont)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make a base class between these?

// Map of all the get_async_continuation[_addr] instructions in async
// functions, for checking suspendable contexts.
llvm::DenseMap<SILFunction *, SmallVector<SingleValueInstruction *, 2>>
AsyncContinuationSources;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can just do this analysis in checkFlowSensitiveRules, and then you won't need to kindof reinvent it.

@jckarter jckarter force-pushed the async-await-sil-verifier branch from 4f872bf to 9d44102 Compare October 8, 2020 00:41
@jckarter
Copy link
Contributor Author

jckarter commented Oct 8, 2020

@swift-ci Please test

@jckarter
Copy link
Contributor Author

jckarter commented Oct 8, 2020

@rjmccall @theblixguy Thanks for the feedback! How's this look now?

@theblixguy
Copy link
Collaborator

Thanks for applying the FullApplySite suggestion - the change looks good to me!

@swift-ci
Copy link
Contributor

swift-ci commented Oct 8, 2020

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

@jckarter
Copy link
Contributor Author

jckarter commented Oct 8, 2020

@swift-ci Please test OS X

@@ -1519,6 +1519,21 @@ MultipleValueInstruction *MultipleValueInstructionResult::getParent() {
return reinterpret_cast<MultipleValueInstruction *>(value);
}

/// Returns true if evaluation of this node may cause suspension of an
/// async task.
bool SILNode::maySuspend() const {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this just be on SILInstruction? Not sure there's any situation where it makes sense to ask this of a SILValue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it doesn't seem harmful to put it on SILNode either, to save a cast if all you do have is a SILValue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess my question is whether it ever makes sense to just ask this of a SILValue. It's never a property of a value, just of an instruction that can produce a value. If you're asking a SILValue you're almost certainly doing it wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I'll move it to SILInstruction.

lib/SIL/Verifier/SILVerifier.cpp Outdated Show resolved Hide resolved
lib/SIL/Verifier/SILVerifier.cpp Show resolved Hide resolved
lib/SIL/Verifier/SILVerifier.cpp Outdated Show resolved Hide resolved
@jckarter jckarter force-pushed the async-await-sil-verifier branch from 9d44102 to 5b1ddf6 Compare October 9, 2020 18:00
@jckarter
Copy link
Contributor Author

jckarter commented Oct 9, 2020

Thanks for checking my work. I fixed the logic issues; how's this look now?

@jckarter
Copy link
Contributor Author

jckarter commented Oct 9, 2020

@swift-ci Please test

@swift-ci
Copy link
Contributor

swift-ci commented Oct 9, 2020

Build failed
Swift Test Linux Platform
Git Sha - 5b1ddf661615ac2b29c9656158ff25042a642fdb

@beccadax
Copy link
Contributor

beccadax commented Oct 9, 2020

Restarting doomed Linux tests.

@swift-ci please test Linux platform

@rjmccall
Copy link
Contributor

rjmccall commented Oct 9, 2020

Thanks for checking my work. I fixed the logic issues; how's this look now?

Logic fixes look good, thanks.

- Enforce types of continuations and resume/error BBargs for await
- Can't access the continuation again or exit the function mid-suspend
@jckarter
Copy link
Contributor Author

jckarter commented Oct 9, 2020

@swift-ci Please test

@swift-ci
Copy link
Contributor

swift-ci commented Oct 9, 2020

Build failed
Swift Test OS X Platform
Git Sha - 5b1ddf661615ac2b29c9656158ff25042a642fdb

@jckarter jckarter force-pushed the async-await-sil-verifier branch from 5b1ddf6 to 3364c51 Compare October 9, 2020 22:21
@jckarter
Copy link
Contributor Author

@swift-ci Please test OS X

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 3364c51

@jckarter
Copy link
Contributor Author

@swift-ci Please test OS X

@jckarter jckarter merged commit aef8f50 into swiftlang:main Oct 19, 2020
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.

5 participants