Skip to content

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented May 5, 2023

This removes all the cases of a dataflow node stepping to itself.

Commit-by-commit review recommended.

@MathiasVP MathiasVP requested a review from a team as a code owner May 5, 2023 15:04
@github-actions github-actions bot added the C++ label May 5, 2023
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label May 5, 2023
then preUpdate = [nFrom, getAPriorDefinition(defOrUse)]
else preUpdate = nFrom
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain what the change is here / why nodeFrom != nodeTo above is insufficient on its own?

Otherwise LGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. This was also my initial idea, but then @hvitved pointed out that for flow out of post-update nodes specifically we need to allow SSA identity-steps. To see why, consider this example:

void setTaint(x) {
  sink(x.field);
  x.field = taint;
}

while (b) {
  setTaint(y);
}

the flow after leaving setTaint (i.e., from [post] y) to y is implemented as an SSA step from [post] y's pre-update node (which is exactly y) to the next use of y, but the next use of y is y itself! So in order to have flow from [post] y to y the ssaFlow predicate must include steps from y to y itself in order to support flow out of post-update nodes.

So the final creates a new postUpdateFlow predicate whose implementation is equivalent to ssaFlow, but allow identity steps.

Does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense, yep. Thanks for the explanation!

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.

👍

@MathiasVP MathiasVP merged commit 720586c into github:main May 10, 2023
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.

2 participants