Skip to content

Commit 697b2dc

Browse files
author
Dave Bartolomeo
committed
C++: Add missing store step for single-field struct use
We have special code to handle field flow for single-field structs, but that special case was too specific. Some `Store`s to single-field structs have no `Chi` instruction, which is the case that we handled already. However, it is possible for the `Store` to have a `Chi` instruction (e.g. for `{AllAliased}`), but still have a use of the result of the `Store` directly. We now add a `PostUpdateNode` for the result of the `Store` itself in those cases, just like we already did if the `Store` had no `Chi`.
1 parent 0a86642 commit 697b2dc

File tree

7 files changed

+93
-35
lines changed

7 files changed

+93
-35
lines changed

‎cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -362,15 +362,22 @@ private class ExplicitFieldStoreQualifierNode extends PartialDefinitionNode {
362362

363363
/**
364364
* Not every store instruction generates a chi instruction that we can attach a PostUpdateNode to.
365-
* For instance, an update to a field of a struct containing only one field. For these cases we
366-
* attach the PostUpdateNode to the store instruction. There's no obvious pre update node for this case
367-
* (as the entire memory is updated), so `getPreUpdateNode` is implemented as `none()`.
365+
* For instance, an update to a field of a struct containing only one field. Even if the store does
366+
* have a chi instruction, a subsequent use of the result of the store may be linked directly to the
367+
* result of the store as an inexact definition if the store totally overlaps the use. For these
368+
* cases we attach the PostUpdateNode to the store instruction. There's no obvious pre update node
369+
* for this case (as the entire memory is updated), so `getPreUpdateNode` is implemented as
370+
* `none()`.
368371
*/
369372
private class ExplicitSingleFieldStoreQualifierNode extends PartialDefinitionNode {
370373
override StoreInstruction instr;
371374

372375
ExplicitSingleFieldStoreQualifierNode() {
373-
not exists(ChiInstruction chi | chi.getPartial() = instr) and
376+
(
377+
instr.getAUse().isDefinitionInexact()
378+
or
379+
not exists(ChiInstruction chi | chi.getPartial() = instr)
380+
) and
374381
// Without this condition any store would create a `PostUpdateNode`.
375382
instr.getDestinationAddress() instanceof FieldAddressInstruction
376383
}

‎cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/PrintIRLocalFlow.qll

Lines changed: 1 addition & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -6,34 +6,7 @@ private import semmle.code.cpp.ir.ValueNumbering
66
private import semmle.code.cpp.ir.IR
77
private import semmle.code.cpp.ir.dataflow.DataFlow
88
private import semmle.code.cpp.ir.dataflow.internal.DataFlowUtil
9-
10-
/**
11-
* Gets a short ID for an IR dataflow node.
12-
* - For `Instruction`s, this is just the result ID of the instruction (e.g. `m128`).
13-
* - For `Operand`s, this is the label of the operand, prefixed with the result ID of the
14-
* instruction and a dot (e.g. `m128.left`).
15-
* - For `Variable`s, this is the qualified name of the variable.
16-
*/
17-
private string nodeId(DataFlow::Node node, int order1, int order2) {
18-
exists(Instruction instruction | instruction = node.asInstruction() |
19-
result = instruction.getResultId() and
20-
order1 = instruction.getBlock().getDisplayIndex() and
21-
order2 = instruction.getDisplayIndexInBlock()
22-
)
23-
or
24-
exists(Operand operand, Instruction instruction |
25-
operand = node.asOperand() and
26-
instruction = operand.getUse()
27-
|
28-
result = instruction.getResultId() + "." + operand.getDumpId() and
29-
order1 = instruction.getBlock().getDisplayIndex() and
30-
order2 = instruction.getDisplayIndexInBlock()
31-
)
32-
or
33-
result = "var(" + node.asVariable().getQualifiedName() + ")" and
34-
order1 = 1000000 and
35-
order2 = 0
36-
}
9+
private import PrintIRUtilities
3710

3811
/**
3912
* Gets the local dataflow from other nodes in the same function to this node.

‎cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/PrintIRStoreSteps.qll

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
/**
2+
* Print the dataflow local store steps in IR dumps.
3+
*/
4+
5+
private import cpp
6+
// The `ValueNumbering` library has to be imported right after `cpp` to ensure
7+
// that the cached IR gets the same checksum here as it does in queries that use
8+
// `ValueNumbering` without `DataFlow`.
9+
private import semmle.code.cpp.ir.ValueNumbering
10+
private import semmle.code.cpp.ir.IR
11+
private import semmle.code.cpp.ir.dataflow.DataFlow
12+
private import semmle.code.cpp.ir.dataflow.internal.DataFlowUtil
13+
private import semmle.code.cpp.ir.dataflow.internal.DataFlowPrivate
14+
private import PrintIRUtilities
15+
16+
/**
17+
* Property provider for local IR dataflow store steps.
18+
*/
19+
class LocalFlowPropertyProvider extends IRPropertyProvider {
20+
override string getInstructionProperty(Instruction instruction, string key) {
21+
exists(DataFlow::Node objectNode, Content content |
22+
key = "content[" + content.toString() + "]" and
23+
instruction = objectNode.asInstruction() and
24+
result =
25+
strictconcat(string element, DataFlow::Node fieldNode |
26+
storeStep(fieldNode, content, objectNode) and
27+
element = nodeId(fieldNode, _, _)
28+
|
29+
element, ", "
30+
)
31+
)
32+
}
33+
}

‎cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/PrintIRUtilities.qll

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
/**
2+
* Shared utilities used when printing dataflow annotations in IR dumps.
3+
*/
4+
5+
private import cpp
6+
// The `ValueNumbering` library has to be imported right after `cpp` to ensure
7+
// that the cached IR gets the same checksum here as it does in queries that use
8+
// `ValueNumbering` without `DataFlow`.
9+
private import semmle.code.cpp.ir.ValueNumbering
10+
private import semmle.code.cpp.ir.IR
11+
private import semmle.code.cpp.ir.dataflow.DataFlow
12+
13+
/**
14+
* Gets a short ID for an IR dataflow node.
15+
* - For `Instruction`s, this is just the result ID of the instruction (e.g. `m128`).
16+
* - For `Operand`s, this is the label of the operand, prefixed with the result ID of the
17+
* instruction and a dot (e.g. `m128.left`).
18+
* - For `Variable`s, this is the qualified name of the variable.
19+
*/
20+
string nodeId(DataFlow::Node node, int order1, int order2) {
21+
exists(Instruction instruction | instruction = node.asInstruction() |
22+
result = instruction.getResultId() and
23+
order1 = instruction.getBlock().getDisplayIndex() and
24+
order2 = instruction.getDisplayIndexInBlock()
25+
)
26+
or
27+
exists(Operand operand, Instruction instruction |
28+
operand = node.asOperand() and
29+
instruction = operand.getUse()
30+
|
31+
result = instruction.getResultId() + "." + operand.getDumpId() and
32+
order1 = instruction.getBlock().getDisplayIndex() and
33+
order2 = instruction.getDisplayIndexInBlock()
34+
)
35+
or
36+
result = "var(" + node.asVariable().getQualifiedName() + ")" and
37+
order1 = 1000000 and
38+
order2 = 0
39+
}

‎cpp/ql/test/library-tests/dataflow/dataflow-tests/dataflow-ir-consistency.expected

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ unreachableNodeCCtx
2626
localCallNodes
2727
postIsNotPre
2828
postHasUniquePre
29+
| test.cpp:373:5:373:20 | Store | PostUpdateNode should have one pre-update node but has 0. |
2930
uniquePostUpdate
3031
postIsInSameCallable
3132
reverseRead
@@ -82,4 +83,5 @@ postWithInFlow
8283
| test.cpp:125:3:125:11 | Chi | PostUpdateNode should not be the target of local flow. |
8384
| test.cpp:359:5:359:20 | Chi | PostUpdateNode should not be the target of local flow. |
8485
| test.cpp:373:5:373:20 | Chi | PostUpdateNode should not be the target of local flow. |
86+
| test.cpp:373:5:373:20 | Store | PostUpdateNode should not be the target of local flow. |
8587
| test.cpp:465:3:465:15 | Chi | PostUpdateNode should not be the target of local flow. |

‎cpp/ql/test/library-tests/dataflow/fields/dataflow-ir-consistency.expected

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,9 @@ unreachableNodeCCtx
2020
localCallNodes
2121
postIsNotPre
2222
postHasUniquePre
23+
| D.cpp:57:5:57:42 | Store | PostUpdateNode should have one pre-update node but has 0. |
2324
| simple.cpp:65:5:65:22 | Store | PostUpdateNode should have one pre-update node but has 0. |
25+
| simple.cpp:83:9:83:28 | Store | PostUpdateNode should have one pre-update node but has 0. |
2426
| simple.cpp:92:5:92:22 | Store | PostUpdateNode should have one pre-update node but has 0. |
2527
uniquePostUpdate
2628
postIsInSameCallable
@@ -54,6 +56,7 @@ postWithInFlow
5456
| D.cpp:49:15:49:24 | Chi | PostUpdateNode should not be the target of local flow. |
5557
| D.cpp:56:15:56:24 | Chi | PostUpdateNode should not be the target of local flow. |
5658
| D.cpp:57:5:57:42 | Chi | PostUpdateNode should not be the target of local flow. |
59+
| D.cpp:57:5:57:42 | Store | PostUpdateNode should not be the target of local flow. |
5760
| aliasing.cpp:9:3:9:22 | Chi | PostUpdateNode should not be the target of local flow. |
5861
| aliasing.cpp:13:3:13:21 | Chi | PostUpdateNode should not be the target of local flow. |
5962
| aliasing.cpp:17:3:17:21 | Chi | PostUpdateNode should not be the target of local flow. |
@@ -150,6 +153,7 @@ postWithInFlow
150153
| simple.cpp:23:35:23:35 | Chi | PostUpdateNode should not be the target of local flow. |
151154
| simple.cpp:65:5:65:22 | Store | PostUpdateNode should not be the target of local flow. |
152155
| simple.cpp:83:9:83:28 | Chi | PostUpdateNode should not be the target of local flow. |
156+
| simple.cpp:83:9:83:28 | Store | PostUpdateNode should not be the target of local flow. |
153157
| simple.cpp:92:5:92:22 | Store | PostUpdateNode should not be the target of local flow. |
154158
| struct_init.c:20:20:20:29 | Chi | PostUpdateNode should not be the target of local flow. |
155159
| struct_init.c:20:34:20:34 | Chi | PostUpdateNode should not be the target of local flow. |

‎cpp/ql/test/library-tests/dataflow/fields/ir-path-flow.expected

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -228,8 +228,8 @@ edges
228228
| simple.cpp:65:5:65:22 | Store [i] | simple.cpp:66:12:66:12 | Store [i] |
229229
| simple.cpp:65:11:65:20 | call to user_input | simple.cpp:65:5:65:22 | Store [i] |
230230
| simple.cpp:66:12:66:12 | Store [i] | simple.cpp:67:13:67:13 | i |
231-
| simple.cpp:83:9:83:28 | Chi [f1] | simple.cpp:84:14:84:20 | this indirection [f1] |
232-
| simple.cpp:83:17:83:26 | call to user_input | simple.cpp:83:9:83:28 | Chi [f1] |
231+
| simple.cpp:83:9:83:28 | Store [f1] | simple.cpp:84:14:84:20 | this indirection [f1] |
232+
| simple.cpp:83:17:83:26 | call to user_input | simple.cpp:83:9:83:28 | Store [f1] |
233233
| simple.cpp:84:14:84:20 | this indirection [f1] | simple.cpp:84:14:84:20 | call to getf2f1 |
234234
| simple.cpp:92:5:92:22 | Store [i] | simple.cpp:93:20:93:20 | Store [i] |
235235
| simple.cpp:92:11:92:20 | call to user_input | simple.cpp:92:5:92:22 | Store [i] |
@@ -494,7 +494,7 @@ nodes
494494
| simple.cpp:65:11:65:20 | call to user_input | semmle.label | call to user_input |
495495
| simple.cpp:66:12:66:12 | Store [i] | semmle.label | Store [i] |
496496
| simple.cpp:67:13:67:13 | i | semmle.label | i |
497-
| simple.cpp:83:9:83:28 | Chi [f1] | semmle.label | Chi [f1] |
497+
| simple.cpp:83:9:83:28 | Store [f1] | semmle.label | Store [f1] |
498498
| simple.cpp:83:17:83:26 | call to user_input | semmle.label | call to user_input |
499499
| simple.cpp:84:14:84:20 | call to getf2f1 | semmle.label | call to getf2f1 |
500500
| simple.cpp:84:14:84:20 | this indirection [f1] | semmle.label | this indirection [f1] |

0 commit comments

Comments
 (0)