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

Dataflow: update fieldFlowBranchLimit semantics #15599

Merged
merged 8 commits into from
Apr 23, 2024

Conversation

aschackmull
Copy link
Contributor

@aschackmull aschackmull commented Feb 13, 2024

This makes two changes to the fieldFlowBranchLimit interpretation:

  • The count is adjusted to properly count virtual dispatch instead of nodes. This will block less flow and hence result in more computation and more alerts - hopefully fixing some FNs.
  • The blocking condition on return edges is changed to only care about virtual dispatch count and not the number of call sites. This will block more flow and hopefully reduce FPs and performance problems based on uncertain dispatch. This has anecdotally been identified as the core issue in a couple of poorly performing cases.

For the return edge condition, special care is taken to still follow call edges that are determined by the call context.

All qltests are also updated to use the default fieldFlowBranchLimit instead of an inflated value to better reflect what's actually used in queries.

@aschackmull aschackmull force-pushed the dataflow/fieldflowbranchlimit-v2 branch from 0844619 to 912c8fa Compare February 21, 2024 11:07
@aschackmull aschackmull force-pushed the dataflow/fieldflowbranchlimit-v2 branch from 15a07b2 to 26c3de5 Compare February 26, 2024 10:04
@aschackmull aschackmull changed the title Dataflow: wip test of fieldflowbranchlimit adjustment Dataflow: update fieldFlowBranchLimit semantics Feb 26, 2024
@aschackmull aschackmull force-pushed the dataflow/fieldflowbranchlimit-v2 branch from 26c3de5 to a6419dc Compare February 26, 2024 13:37
@aschackmull aschackmull force-pushed the dataflow/fieldflowbranchlimit-v2 branch from bdfd86b to 3abae04 Compare March 5, 2024 13:10
@aschackmull aschackmull force-pushed the dataflow/fieldflowbranchlimit-v2 branch 2 times, most recently from db93e59 to 3a7d795 Compare March 7, 2024 11:02
@aschackmull aschackmull force-pushed the dataflow/fieldflowbranchlimit-v2 branch from 0adaa99 to d6338b9 Compare April 11, 2024 09:01
@aschackmull aschackmull force-pushed the dataflow/fieldflowbranchlimit-v2 branch from d6338b9 to 3c69f8f Compare April 15, 2024 13:18
@aschackmull aschackmull marked this pull request as ready for review April 19, 2024 06:46
@aschackmull aschackmull requested review from a team as code owners April 19, 2024 06:46
result = n.asInstruction() or
result = n.asOperand().getUse() or
result = n.(SsaPhiNode).getPhiNode().getBasicBlock().getFirstInstruction() or
n.(IndirectInstruction).hasInstructionAndIndirectionIndex(result, _) or
Copy link
Contributor

Choose a reason for hiding this comment

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

n.hasInstructionAndIndirectionIndex(instr, index) may actually hold for multiple (instr, index) pairs, so this predicate really should be called getAnInstruction. Additionally, this seems to miss a case for IndirectOperands which I guess means that slightly fewer nodes than expected will have a second-level scope. However, since the shared library handles missing second-level scopes it's probably not a big deal.

The C/C++ team will probably fix this once this PR has been merged 👍 There are some more follow-ups that we want to do, and I'll write these up as an internal C/C++ issue.

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.

LGTM, one question.

private int ctxDispatchFanoutOnReturn(NodeEx out, DataFlowCall ctx) {
exists(DataFlowCall call, DataFlowCallable c |
simpleDispatchFanoutOnReturn(call, out) > 1 and
not Stage1::revFlow(out, false) and
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this restriction needed?

simpleDispatchFanoutOnReturn(call, out) > 1 and
not Stage1::revFlow(out, false) and
call.getEnclosingCallable() = c and
returnCallEdge1(c, _, ctx, _) and
Copy link
Contributor

Choose a reason for hiding this comment

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

So we are only considering contexts ctx that we also return to; why is that? (and I guess that answers my question above).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This ensures that a call-context is always associated with the dispatch fanout at call. Either this is nested flow-through, in which case the context will be active in the initial flow-in when entering call, or this is returning flow that didn't come from a parameter in which case the return-call-context will be the thing that reduces the eventual fanout. This does miss the case where we enter the surrounding scope with a call-context and flow-through at call without returning further, but I think that's ok - for the "MaD model does lambda callback" case this would only be problematic if there were two lambdas being passed in with flow through the first and then entering the second. If we used the dual constraint not fwdFlow(out, false) to ensure a call-context from a parameter instead, then we'd miss the important case where a source is inside the lambda callback. So to cover this case we'd need a separate range on contexts to count, and I don't think that's worth the effort.

@aschackmull aschackmull merged commit b2f0994 into github:main Apr 23, 2024
55 checks passed
@aschackmull aschackmull deleted the dataflow/fieldflowbranchlimit-v2 branch April 23, 2024 08:08
MathiasVP added a commit to MathiasVP/ql that referenced this pull request May 1, 2024
…ldflowbranchlimit-v2"

This reverts commit b2f0994, reversing
changes made to 19974f0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants