Rust: Handle async blocks in CFG and SSA #17949
Conversation
| | unreachable.rs:301:13:301:32 | CallExpr | Call should have one enclosing callable but has 0. | | ||
| | unreachable.rs:306:5:306:18 | CallExpr | Call should have one enclosing callable but has 0. | | ||
| | unreachable.rs:308:5:308:18 | CallExpr | Call should have one enclosing callable but has 0. | | ||
| | unreachable.rs:310:5:310:18 | CallExpr | Call should have one enclosing callable but has 0. | |
There was a problem hiding this comment.
These added inconsistencies are only because more nodes are now included in the CFG. Before the CFG would end in a loop in the for_ever async block. Now the block gets its own scope and the remaining code are the block is also in the CFG.
paldepind
force-pushed
the
rust-async-blocks
branch
2 times, most recently
from
7e78d64 to
ef886de
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
paldepind
force-pushed
the
rust-async-blocks
branch
from
ef886de to
9f0fba1
Compare
hvitved
left a comment
hvitved
left a comment
There was a problem hiding this comment.
Great work! Mostly some stylistic comments.
|
|
||
| final class CfgScope = CfgScopeImpl; | ||
|
|
||
| class AsyncBlockScope extends CfgScopeImpl, BlockExpr { |
There was a problem hiding this comment.
If you add instance ExprTrees::AsyncBlockExprTree after the extends clause, then you can remove the charpred, and replace the this.(ExprTrees::AsyncBlockExprTree) terms with super.
| async fn async_block_capture() { | ||
| let mut i: i64 = 0; // i | ||
| let block = async { | ||
| i = 1; // $ read_access=i |
There was a problem hiding this comment.
Why is this not a write_access?
There was a problem hiding this comment.
Oops. There was even testFailures in the .expected file that I missed 🙈
|
|
||
| final class CfgScope = Scope::CfgScope; | ||
|
|
||
| predicate getEnclosingCfgScope = Scope::getEnclosingCfgScope/1; |
There was a problem hiding this comment.
We rarely expose classless predicates like this one; I think it would be better as a member predicate on AstNode, like getEnclosingCallable. We probably also want to cache it like with AsNode.getEnclosingCallable.
| } | ||
|
|
||
| /** Holds if evaluating `e` jumps to the evaluation of a different CFG scope. */ | ||
| predicate isControlFlowJump(Expr e) { e instanceof CallExprBase or e instanceof AwaitExpr } |
There was a problem hiding this comment.
I think we should just make this a (private) predicate inside SsaImpl.qll.
|
Thanks for the feedback 😄. The comments should be addressed now. |
| } | ||
|
|
||
| /** Gets the CFG scope that encloses this node, if any. */ | ||
| CfgScope getEnclosingCfgScope() { |
There was a problem hiding this comment.
I still think we'd want to cache this.
There was a problem hiding this comment.
Great. Let's finally do a DCA run before merging.
|
DCA looks good, so let's merge. |
2 participants
This PR changes how
asyncblocks are modeled in the CFG. Consider this example:Currently
asyncblocks are treated as regular blocks in the CFG. The control enters the block at when the block is encountered. But, in Rust futures are based on polling and they do nothing unless they are polled throughawait. This implies thatasyncblocks aren't executed before it is awaited. Hence, in the above example "hello" is always printed after "godbye" since the order of the.awaitsdetermine when the content of theasyncblocks are polled.To model this, this PR changes the CFG s.t.
asyncblock.asyncblocks are stepped over when they are encountered.In the SSA construction:
asyncblocks are handled like closures as they can capture variables..awaitexpressions are treated like calls as they jump to the execution of anasyncblock or function.