Skip to content

C++: Fix localStepsToExpr performance #12477

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

Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 26 additions & 2 deletions cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll
Original file line number Diff line number Diff line change
Expand Up @@ -1142,7 +1142,8 @@ private class IndirectOperandIndirectExprNode extends IndirectExprNodeBase, RawI
}

private class IndirectInstructionIndirectExprNode extends IndirectExprNodeBase,
RawIndirectInstruction {
RawIndirectInstruction
{
IndirectInstructionIndirectExprNode() { indirectExprNodeShouldBeIndirectInstruction(this, _) }

final override Expr getConvertedExpr(int index) {
Expand Down Expand Up @@ -1617,7 +1618,30 @@ private module ExprFlowCached {
*/
private predicate localStepFromNonExpr(Node n1, Node n2) {
not exists(n1.asExpr()) and
localFlowStep(n1, n2)
localFlowStep(n1, n2) and
// Remember that `localStepsToExpr(n1, n2, e2)` captures that `n2` is
// the first node after `n1` in the dataflow local step-relation that has a
// result for `asExpr()`. To do this, `localStepsToExpr` does a transitive
// closure over `localFlowStep`, but where `localFlowStep` is restricted to
// only take a step as long as there is no result for `asExpr()`.
// This restricted `localFlowStep` is exactly this predicate. Once we find a
// node for which `asExpr()` exists, the transitive closure stops.
// But if we've reached a `SsaPhiNode` node, it means that we're about to merge
// incoming data from multiple places. And each of those incoming places should
// have their own node for which `asExpr()` holds.
// Similarly, if we've reached a `InitialGlobalValue` it means we've stepped from
// a write to a global variable, and into the dataflow node from the global
// variable and from into the initial definition of the global variable at some
// function entry. That write to the global variable should have a result for
// `asExpr()` at the place where the global variable is written.
not n1 instanceof SsaPhiNode and
not n1 instanceof InitialGlobalValue and
// And dually to the above case for `InitialGlobalValue`: if we're at a
// `FinalGlobalValue` node it means we've reached the final value of a global
// variable as we're exiting a function. That global variable will have a final
// mention somewhere in the body of the function, and that mention will have a
// result for `asExpr()`.
not n2 instanceof FinalGlobalValue
Comment on lines +1638 to +1644
Copy link
Contributor

Choose a reason for hiding this comment

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

So when I originally commented. I thought that the comment above not n1 instanceof SsaPhiNode applied to both that and not n1 instanceof InitialGlobalValue, which it clearly does not. A combined comment for InitialGlobalValue and FinalGlobalValue might make slightly more sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that's a good point. I'll write a combined comment once I've figured out why this PR is breaking some query tests.

}

/**
Expand Down