Skip to content

Conversation

MathiasVP
Copy link
Contributor

This PR adds CFG support for C++20 coroutines. It's basically complete, except for the fact that we don't yet have an instruction to represent "awaiting a value". So instead we're simply emitting a NoOp instruction for now.

Eventually, that NoOp instruction should be replaced with a new instruction, and subsequent analyses should decide what to do with it. (For example, dataflow should use field flow to ensure that yielded values propagate to the caller.)

Commit-by-commit review recommended.

@MathiasVP MathiasVP requested a review from a team as a code owner April 11, 2024 14:48
@github-actions github-actions bot added the C++ label Apr 11, 2024
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Apr 11, 2024
@MathiasVP MathiasVP requested a review from geoffw0 April 12, 2024 13:15
Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

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

Two questions about this.

// TODO: Use a new opcode to represent "awaiting the value"
expr instanceof CoAwaitExpr and result instanceof Opcode::CopyValue
or
// TODO: Use a new opcode to represent "awaiting the value"
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be:

Suggested change
// TODO: Use a new opcode to represent "awaiting the value"
// TODO: Use a new opcode to represent "yielding a value"

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually no. co_yield is just syntactic sugar for:

co_await promise.yield_value(expr)

(see here) and the "yielding" is done by the call to yield_value.

So whatever instruction we come up with to use the above TODO should also be used here since a co_yield ultimately does something equivalent to a special kind of co_await.

#-----| m0_9(promise_type) = Chi : total:m87_14, partial:m0_8
# 88| v88_1(void) = NoOp :
#-----| v0_10(void) = NoOp :
#-----| Goto (back edge) -> Block 1
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm somewhat confused by this. Why are there two blocks here and why is this Goto marked as a back edge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I didn't spot this either. I'm also confused by this. Will investigate!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should've checked the raw IR before opening this PR >.< The IR is still very messed up! I'll fix that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha! I've found the problem:

private predicate foo(UnaryOperation uo) {
  not exists(uo.getOperand())
}

has 4 results in the co_return_void function 🤔 So I think there's some extractor issue going on here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've created an issue for this now.

Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

Looks plausible (I'm out of my depth this deep in the IR).

@@ -1501,3 +1501,41 @@ class TranslatedVlaDeclarationStmt extends TranslatedStmt {

override Instruction getChildSuccessorInternal(TranslatedElement child, EdgeKind kind) { none() }
}

class TranslatedCoReturnStmt extends TranslatedStmt {
override CoReturnStmt stmt;
Copy link
Contributor

Choose a reason for hiding this comment

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

I did not know you can do this (override the type of a member variable in QL).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's extremely useful! We do it a lot in both C/C++ IR construction and in the Swift CFG construction code

Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

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

LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants