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

C++: Extend barrier guards to handle phi inputs #16677

Merged
merged 18 commits into from
Jun 6, 2024

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Jun 4, 2024

This is the C/C++ version of #15985. The strategy is completely identical to the Ruby version: create a new dataflow node branch for phi inputs, and modify dataflow so that we do def -> phi input -> phi instead of def -> phi. We then extend the barrier guard implementation so that it can put barriers on these new phi input nodes.

I've also done some cleanup along the way.

Commit-by-commit review recommended.

@github-actions github-actions bot added the C++ label Jun 4, 2024
@MathiasVP
Copy link
Contributor Author

DCA results:

  • Three new results for cpp/invalid-pointer-deref. They're all in totally identical code that is copied to multiple places, and I think they're FPs caused by very complex integer ranges.
  • One FP removed from cpp/unbounded-write. We don't use barrier guards in very many places, so I'm happy that the only query where we use these are actually getting an impact from this.

Going forward, I think we should strive to use more barrier guards instead of adding manual barrier logic.

@MathiasVP MathiasVP marked this pull request as ready for review June 5, 2024 09:10
@MathiasVP MathiasVP requested a review from a team as a code owner June 5, 2024 09:10
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Jun 5, 2024
Comment on lines 80 to 87
int x = source();

if(!guarded(x)) {
x = 0;
}
sink(x); // $ SPURIOUS: ast
}
Copy link
Contributor

@jketema jketema Jun 6, 2024

Choose a reason for hiding this comment

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

How am I supposed to read tests like these?

Copy link
Contributor Author

@MathiasVP MathiasVP Jun 6, 2024

Choose a reason for hiding this comment

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

guarded is a bad name for the barrier guard in this case. The function should really be read as isSafe. So this is a standard dataflow test, except that there's a function guarded (read: isSafe) to check if the value produced by source() is allowed to flow to sink.

So in this case, the value produced by source() is either safe (i.e., guarded(x) is true), or the value is reassigned to 0.

And after these changes, IR dataflow realizes that the value of x is always either 0 or guarded(x) holds when reaching sink(x).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation. That helps!

Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

Some simple QL docs comments. Also, does C++ use phi-reads like Ruby? In all cases, perhaps add a test similar to the one from Ruby.

@MathiasVP
Copy link
Contributor Author

Some simple QL docs comments. Also, does C++ use phi-reads like Ruby? In all cases, perhaps add a test similar to the one from Ruby.

We use phi-reads in C++ like in Ruby, yep. I'll add that test right away!

@MathiasVP
Copy link
Contributor Author

Some simple QL docs comments. Also, does C++ use phi-reads like Ruby? In all cases, perhaps add a test similar to the one from Ruby.

I've added that test in f7b2d98, and everything now works 🎉

I think the problem with !guarded(x) is a problem with the way our test is using guard conditions. I've added a simpler guard with no negation that demonstrates that everything is working, and I think I'll delay why the negation doesn't work to a future PR if that's okay with you @jketema?

exists(SourceVariable sv, IRBlock bb2, int i2 |
adjustForPointerArith(pun, sv, bb2, i2) and
useToNode(bb2, i2, sv, n)
// The reason for this predicate is a bit annoying:
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not really a predicate anymore, I think.

Copy link
Contributor Author

@MathiasVP MathiasVP Jun 6, 2024

Choose a reason for hiding this comment

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

Agreed. I've just removed the line now in 8aaa2a1. I've reached a state of acceptance of this complication 😂

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 if DCA comes back happy.

@MathiasVP
Copy link
Contributor Author

DCA is mostly unchanged compared to the previous run. We get rid of one more lost result on erlang, and that seems to be a FP that we exclude for exactly the right reason 🎉

I don't quite understand why we didn't exclude it before, but it certainly looks like it should be excluded from this change. So I'm happy with this 🎉 Merging!

@MathiasVP MathiasVP merged commit 9f4c138 into github:main Jun 6, 2024
15 checks passed
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