C++: Synchronize product dataflow paths on function entry points #12997
Conversation
MathiasVP
force-pushed
the
sync-product-flow-across-calls
branch
from
d1ac07b to
7fa6894
Compare
| kind = TOutOf(call) and | ||
| succ1.getNode().(OutNode).getCall() = call and | ||
| pred1.getNode() instanceof ReturnNode |
There was a problem hiding this comment.
There can be other out-flow besides the OutNode-to-ReturnNode pairs. It can also be from a PostUpdateNode that has flow from a parameter to a PostUpdateNode for an argument.
There was a problem hiding this comment.
Oh, yes. That's a good point. I decided to blatantly ignore jump steps for now, but the PostUpdateNode -> PostUpdateNode flow caused by setter-like functions is probably too frequent of an occurrence to be ignored.
There was a problem hiding this comment.
You can probably account for jump-steps by having a kind "jump" and identify that as an interproc edge that's not in one of the other cases.
There was a problem hiding this comment.
Good idea. Hopefully I've done this in 0d6fdc6 🤞.
|
DCA confirms that the performance has been fixed by d3d706d 🎉 |
There was a problem hiding this comment.
LGTM. I don't know if @aschackmull has any further comments?
| TOutOf(DataFlowCall call) { | ||
| [any(Flow1::PathNode n).getNode(), any(Flow2::PathNode n).getNode()].(OutNode).getCall() = | ||
| call |
There was a problem hiding this comment.
This is too restrictive for the post-update out-flow to work.
There was a problem hiding this comment.
Oh, you're right. Will fix (and rerun DCA)
There was a problem hiding this comment.
Should hopefully be fixed by 26206a8 🤞
Glad I asked 😄 |
| intoImpl1(_, _, call) or | ||
| intoImpl2(_, _, call) | ||
| } or | ||
| TOutOf(DataFlowCall call) { | ||
| outImpl1(_, _, call) or | ||
| outImpl2(_, _, call) |
There was a problem hiding this comment.
Looks fine now. If you want, you can change these two disjunctions into conjunctions for a little bit of earlier filtering - might as well once it's written this way.
There was a problem hiding this comment.
Ah, wait, no, scratch that - then the jumps won't be identified correctly.
There was a problem hiding this comment.
Uh, that's clever! Will do that
There was a problem hiding this comment.
Oh, yes... that's true. I guess I won't do that 😂.
There was a problem hiding this comment.
Double-reversal: Conjunctions will work if you change the negations in the jump-identification from using into1 etc. to intoImpl1 etc.
There was a problem hiding this comment.
Oh, that's true. This is subtle enough that I think I'll just leave this as-is, and we can change it if it turns out that we get too many TKinds (which I really doubt since they're already restricted to the ones occurring in a PathNode).
|
DCA looks fine now 🎉. |
The experimental "product dataflow library" already synchronizes the two paths when the enclosing callable changes, but the synchronization point didn't check that we entered through the same call. This gave FPs such as the one I added in #12989.
This PR fixes this class of FPs by checking that we entered through the same function call whenever we flow into a function. Similarly, we check that we leave through the same function call in the two paths.