Skip to content

C++: Fix dataflow node <> expression problem on prvalues #15918

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Mar 14, 2024
Merged

C++: Fix dataflow node <> expression problem on prvalues #15918

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a839c92
C++: Fix mapping issue between dataflow nodes and expressions when th…
MathiasVP Mar 13, 2024
c375497
Update cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll
MathiasVP Mar 14, 2024
a24432b
Update cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll
MathiasVP Mar 14, 2024
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
27 changes: 26 additions & 1 deletion cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1331,6 +1331,7 @@ private import GetConvertedResultExpression

/** Holds if `node` is an `OperandNode` that should map `node.asExpr()` to `e`. */
predicate exprNodeShouldBeOperand(OperandNode node, Expr e, int n) {
not exprNodeShouldBeIndirectOperand(_, e, n) and
exists(Instruction def |
unique( | | getAUse(def)) = node.getOperand() and
e = getConvertedResultExpression(def, n)
Expand All @@ -1347,6 +1348,22 @@ private predicate indirectExprNodeShouldBeIndirectOperand(
)
}

/** Holds if `node` should be an `IndirectOperand` that maps `node.asExpr()` to `e`. */
private predicate exprNodeShouldBeIndirectOperand(IndirectOperand node, Expr e, int n) {
exists(ArgumentOperand operand |
// When an argument (qualifier or positional) is a prvalue and the
// parameter (qualifier or positional) is a (const) reference, IR
// construction introduces a temporary `IRVariable`. The `VariableAddress`
// instruction has the argument as its `getConvertedResultExpression`
// result. However, the instruction actually represents the _address_ of
// the argument. So to fix this mismatch, we have the indirection of the
// `VariableAddressInstruction` map to the expression.
node.hasOperandAndIndirectionIndex(operand, 1) and
e = getConvertedResultExpression(operand.getDef(), n) and
operand.getDef().(VariableAddressInstruction).getIRVariable() instanceof IRTempVariable
)
}

private predicate exprNodeShouldBeIndirectOutNode(IndirectArgumentOutNode node, Expr e, int n) {
exists(CallInstruction call |
call.getStaticCallTarget() instanceof Constructor and
Expand All @@ -1359,6 +1376,7 @@ private predicate exprNodeShouldBeIndirectOutNode(IndirectArgumentOutNode node,
predicate exprNodeShouldBeInstruction(Node node, Expr e, int n) {
not exprNodeShouldBeOperand(_, e, n) and
not exprNodeShouldBeIndirectOutNode(_, e, n) and
not exprNodeShouldBeIndirectOperand(_, e, n) and
e = getConvertedResultExpression(node.asInstruction(), n)
}

Expand Down Expand Up @@ -1391,7 +1409,8 @@ abstract private class ExprNodeBase extends Node {
private predicate exprNodeShouldBe(Expr e, int n) {
exprNodeShouldBeInstruction(_, e, n) or
exprNodeShouldBeOperand(_, e, n) or
exprNodeShouldBeIndirectOutNode(_, e, n)
exprNodeShouldBeIndirectOutNode(_, e, n) or
exprNodeShouldBeIndirectOperand(_, e, n)
}

private class InstructionExprNode extends ExprNodeBase, InstructionNode {
Expand Down Expand Up @@ -1533,6 +1552,12 @@ private class IndirectArgumentOutExprNode extends ExprNodeBase, IndirectArgument
final override Expr getConvertedExpr(int n) { exprNodeShouldBeIndirectOutNode(this, result, n) }
}

private class IndirectTemporaryExpr extends ExprNodeBase instanceof IndirectOperand {
IndirectTemporaryExpr() { exprNodeShouldBeIndirectOperand(this, _, _) }
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like there's a mismatch between this class, which claims that the node is a temporary expr, with the QLDoc on exprNodeShouldBeIndirectOperand, which only claims the node is an IndirectOperand (that should have a .asExpr()). Its probably just be me, missing a connection here.

Copy link
Contributor Author

@MathiasVP MathiasVP Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That's fair. I'll rename the class to something more generic. It's currently true because exprNodeShouldBeIndirectOperand restricts the node to be an indirection of an operand whose definition is related to a temporary IR variable. But that's not guaranteed to be true if we find other uses of this class.

If it's okay with you, I'll do this in a subsequent PR so that you can get this change into #15371 without waiting for another round of CI


final override Expr getConvertedExpr(int n) { exprNodeShouldBeIndirectOperand(this, result, n) }
}

/**
* An expression, viewed as a node in a data flow graph.
*/
Expand Down