Skip to content

Commit 1667051

Browse files
MathiasVPMathiasVP
authored
Merge pull request #21239 from MathiasVP/logical-binary-fix-guards-cpp
C++: Ensure that there are AST `GuardCondition`s for `||` and `&&`
2 parents 8c0baef + 5f079c1 commit 1667051

File tree

5 files changed

+53
-68
lines changed

5 files changed

+53
-68
lines changed

‎cpp/ql/lib/change-notes/2026-01-30-guard-condition-logical-operators.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: fix
3+
---
4+
* Fixed a bug in the `GuardCondition` library which sometimes prevented binary logical operators from being recognized as guard conditions. As a result, queries using `GuardCondition` may see improved results.

‎cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll‎

Lines changed: 27 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,8 @@ import semmle.code.cpp.ir.IR
88
private import codeql.util.Void
99
private import codeql.controlflow.Guards as SharedGuards
1010
private import semmle.code.cpp.ir.ValueNumbering
11-
private import semmle.code.cpp.ir.implementation.raw.internal.TranslatedExpr
11+
private import semmle.code.cpp.ir.implementation.raw.internal.TranslatedExpr as TE
12+
private import semmle.code.cpp.ir.implementation.raw.internal.TranslatedFunction as TF
1213
private import semmle.code.cpp.ir.implementation.raw.internal.InstructionTag
1314

1415
private class BasicBlock = IRCfg::BasicBlock;
@@ -683,24 +684,26 @@ final class GuardCondition = GuardConditionImpl;
683684
*/
684685
private class GuardConditionFromBinaryLogicalOperator extends GuardConditionImpl instanceof Cpp::BinaryLogicalOperation
685686
{
687+
GuardConditionImpl l;
688+
GuardConditionImpl r;
689+
690+
GuardConditionFromBinaryLogicalOperator() {
691+
super.getLeftOperand() = l and
692+
super.getRightOperand() = r
693+
}
694+
686695
override predicate valueControls(Cpp::BasicBlock controlled, GuardValue v) {
687-
exists(Cpp::BinaryLogicalOperation binop, GuardCondition lhs, GuardCondition rhs |
688-
this = binop and
689-
lhs = binop.getLeftOperand() and
690-
rhs = binop.getRightOperand() and
691-
lhs.valueControls(controlled, v) and
692-
rhs.valueControls(controlled, v)
693-
)
696+
// `l || r` does not control `r` even though `l` does.
697+
not r.(Cpp::Expr).getBasicBlock() = controlled and
698+
l.valueControls(controlled, v)
699+
or
700+
r.valueControls(controlled, v)
694701
}
695702

696703
override predicate valueControlsEdge(Cpp::BasicBlock pred, Cpp::BasicBlock succ, GuardValue v) {
697-
exists(Cpp::BinaryLogicalOperation binop, GuardCondition lhs, GuardCondition rhs |
698-
this = binop and
699-
lhs = binop.getLeftOperand() and
700-
rhs = binop.getRightOperand() and
701-
lhs.valueControlsEdge(pred, succ, v) and
702-
rhs.valueControlsEdge(pred, succ, v)
703-
)
704+
l.valueControlsEdge(pred, succ, v)
705+
or
706+
r.valueControlsEdge(pred, succ, v)
704707
}
705708

706709
pragma[nomagic]
@@ -1026,7 +1029,7 @@ private class GuardConditionFromIR extends GuardConditionImpl {
10261029

10271030
private predicate excludeAsControlledInstruction(Instruction instr) {
10281031
// Exclude the temporaries generated by a ternary expression.
1029-
exists(TranslatedConditionalExpr tce |
1032+
exists(TE::TranslatedConditionalExpr tce |
10301033
instr = tce.getInstruction(ConditionValueFalseStoreTag())
10311034
or
10321035
instr = tce.getInstruction(ConditionValueTrueStoreTag())
@@ -1038,6 +1041,14 @@ private predicate excludeAsControlledInstruction(Instruction instr) {
10381041
or
10391042
// Exclude unreached instructions, as their AST is the whole function and not a block.
10401043
instr instanceof UnreachedInstruction
1044+
or
1045+
// Exclude instructions generated by a translated function as they map to the function itself
1046+
// and the function is considered the last basic block of a function body.
1047+
any(TF::TranslatedFunction tf).getInstruction(_) = instr
1048+
or
1049+
// `ChiInstruction`s generated by instructions in the above case don't come from `getInstruction` (since they are generated by AliasedSSA)
1050+
// so we need to special case them.
1051+
excludeAsControlledInstruction(instr.(ChiInstruction).getPartial())
10411052
}
10421053

10431054
/**

‎cpp/ql/test/library-tests/controlflow/guards-ir/tests.expected‎

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -384,7 +384,9 @@ astGuardsControl
384384
| test.c:126:7:126:7 | 1 | true | 131 | 132 |
385385
| test.c:126:7:126:7 | 1 | true | 134 | 123 |
386386
| test.c:126:7:126:28 | ... && ... | true | 126 | 128 |
387+
| test.c:126:7:126:28 | ... && ... | true | 131 | 131 |
387388
| test.c:126:7:126:28 | ... && ... | true | 131 | 132 |
389+
| test.c:126:7:126:28 | ... && ... | true | 134 | 123 |
388390
| test.c:126:12:126:26 | call to test3_condition | true | 126 | 128 |
389391
| test.c:126:12:126:26 | call to test3_condition | true | 131 | 132 |
390392
| test.c:131:7:131:7 | b | true | 131 | 132 |
@@ -405,9 +407,7 @@ astGuardsControl
405407
| test.c:181:9:181:9 | x | true | 181 | 182 |
406408
| test.c:181:9:181:9 | x | true | 186 | 180 |
407409
| test.cpp:18:8:18:10 | call to get | true | 19 | 19 |
408-
| test.cpp:31:7:31:13 | ... == ... | false | 30 | 30 |
409410
| test.cpp:31:7:31:13 | ... == ... | false | 34 | 34 |
410-
| test.cpp:31:7:31:13 | ... == ... | true | 30 | 30 |
411411
| test.cpp:31:7:31:13 | ... == ... | true | 31 | 32 |
412412
| test.cpp:42:13:42:20 | call to getABool | true | 43 | 45 |
413413
astGuardsEnsure
@@ -589,13 +589,9 @@ astGuardsEnsure
589589
| test.c:175:13:175:32 | ... == ... | test.c:175:13:175:15 | call to foo | == | test.c:175:32:175:32 | 0 | 0 | 175 | 175 |
590590
| test.c:175:13:175:32 | ... == ... | test.c:175:32:175:32 | 0 | != | test.c:175:13:175:15 | call to foo | 0 | 175 | 175 |
591591
| test.c:175:13:175:32 | ... == ... | test.c:175:32:175:32 | 0 | == | test.c:175:13:175:15 | call to foo | 0 | 175 | 175 |
592-
| test.cpp:31:7:31:13 | ... == ... | test.cpp:31:7:31:7 | x | != | test.cpp:31:12:31:13 | - ... | 0 | 30 | 30 |
593592
| test.cpp:31:7:31:13 | ... == ... | test.cpp:31:7:31:7 | x | != | test.cpp:31:12:31:13 | - ... | 0 | 34 | 34 |
594-
| test.cpp:31:7:31:13 | ... == ... | test.cpp:31:7:31:7 | x | == | test.cpp:31:12:31:13 | - ... | 0 | 30 | 30 |
595593
| test.cpp:31:7:31:13 | ... == ... | test.cpp:31:7:31:7 | x | == | test.cpp:31:12:31:13 | - ... | 0 | 31 | 32 |
596-
| test.cpp:31:7:31:13 | ... == ... | test.cpp:31:12:31:13 | - ... | != | test.cpp:31:7:31:7 | x | 0 | 30 | 30 |
597594
| test.cpp:31:7:31:13 | ... == ... | test.cpp:31:12:31:13 | - ... | != | test.cpp:31:7:31:7 | x | 0 | 34 | 34 |
598-
| test.cpp:31:7:31:13 | ... == ... | test.cpp:31:12:31:13 | - ... | == | test.cpp:31:7:31:7 | x | 0 | 30 | 30 |
599595
| test.cpp:31:7:31:13 | ... == ... | test.cpp:31:12:31:13 | - ... | == | test.cpp:31:7:31:7 | x | 0 | 31 | 32 |
600596
astGuardsEnsure_const
601597
| test.c:7:9:7:13 | ... > ... | test.c:7:9:7:13 | ... > ... | != | 0 | 7 | 9 |
@@ -793,13 +789,21 @@ astGuardsEnsure_const
793789
| test.c:126:7:126:7 | 1 | test.c:126:7:126:7 | 1 | == | 1 | 131 | 132 |
794790
| test.c:126:7:126:7 | 1 | test.c:126:7:126:7 | 1 | == | 1 | 134 | 123 |
795791
| test.c:126:7:126:28 | ... && ... | test.c:126:7:126:7 | 1 | != | 0 | 126 | 128 |
792+
| test.c:126:7:126:28 | ... && ... | test.c:126:7:126:7 | 1 | != | 0 | 131 | 131 |
796793
| test.c:126:7:126:28 | ... && ... | test.c:126:7:126:7 | 1 | != | 0 | 131 | 132 |
794+
| test.c:126:7:126:28 | ... && ... | test.c:126:7:126:7 | 1 | != | 0 | 134 | 123 |
797795
| test.c:126:7:126:28 | ... && ... | test.c:126:7:126:7 | 1 | == | 1 | 126 | 128 |
796+
| test.c:126:7:126:28 | ... && ... | test.c:126:7:126:7 | 1 | == | 1 | 131 | 131 |
798797
| test.c:126:7:126:28 | ... && ... | test.c:126:7:126:7 | 1 | == | 1 | 131 | 132 |
798+
| test.c:126:7:126:28 | ... && ... | test.c:126:7:126:7 | 1 | == | 1 | 134 | 123 |
799799
| test.c:126:7:126:28 | ... && ... | test.c:126:12:126:26 | call to test3_condition | != | 0 | 126 | 128 |
800+
| test.c:126:7:126:28 | ... && ... | test.c:126:12:126:26 | call to test3_condition | != | 0 | 131 | 131 |
800801
| test.c:126:7:126:28 | ... && ... | test.c:126:12:126:26 | call to test3_condition | != | 0 | 131 | 132 |
802+
| test.c:126:7:126:28 | ... && ... | test.c:126:12:126:26 | call to test3_condition | != | 0 | 134 | 123 |
801803
| test.c:126:7:126:28 | ... && ... | test.c:126:12:126:26 | call to test3_condition | == | 1 | 126 | 128 |
804+
| test.c:126:7:126:28 | ... && ... | test.c:126:12:126:26 | call to test3_condition | == | 1 | 131 | 131 |
802805
| test.c:126:7:126:28 | ... && ... | test.c:126:12:126:26 | call to test3_condition | == | 1 | 131 | 132 |
806+
| test.c:126:7:126:28 | ... && ... | test.c:126:12:126:26 | call to test3_condition | == | 1 | 134 | 123 |
803807
| test.c:126:12:126:26 | call to test3_condition | test.c:126:12:126:26 | call to test3_condition | != | 0 | 126 | 128 |
804808
| test.c:126:12:126:26 | call to test3_condition | test.c:126:12:126:26 | call to test3_condition | != | 0 | 131 | 132 |
805809
| test.c:126:12:126:26 | call to test3_condition | test.c:126:12:126:26 | call to test3_condition | == | 1 | 126 | 128 |
@@ -846,17 +850,11 @@ astGuardsEnsure_const
846850
| test.c:181:9:181:9 | x | test.c:181:9:181:9 | x | == | 1 | 186 | 180 |
847851
| test.cpp:18:8:18:10 | call to get | test.cpp:18:8:18:10 | call to get | != | 0 | 19 | 19 |
848852
| test.cpp:18:8:18:10 | call to get | test.cpp:18:8:18:10 | call to get | == | 1 | 19 | 19 |
849-
| test.cpp:31:7:31:13 | ... == ... | test.cpp:31:7:31:7 | x | != | -1 | 30 | 30 |
850853
| test.cpp:31:7:31:13 | ... == ... | test.cpp:31:7:31:7 | x | != | -1 | 34 | 34 |
851-
| test.cpp:31:7:31:13 | ... == ... | test.cpp:31:7:31:7 | x | == | -1 | 30 | 30 |
852854
| test.cpp:31:7:31:13 | ... == ... | test.cpp:31:7:31:7 | x | == | -1 | 31 | 32 |
853-
| test.cpp:31:7:31:13 | ... == ... | test.cpp:31:7:31:13 | ... == ... | != | 0 | 30 | 30 |
854855
| test.cpp:31:7:31:13 | ... == ... | test.cpp:31:7:31:13 | ... == ... | != | 0 | 31 | 32 |
855-
| test.cpp:31:7:31:13 | ... == ... | test.cpp:31:7:31:13 | ... == ... | != | 1 | 30 | 30 |
856856
| test.cpp:31:7:31:13 | ... == ... | test.cpp:31:7:31:13 | ... == ... | != | 1 | 34 | 34 |
857-
| test.cpp:31:7:31:13 | ... == ... | test.cpp:31:7:31:13 | ... == ... | == | 0 | 30 | 30 |
858857
| test.cpp:31:7:31:13 | ... == ... | test.cpp:31:7:31:13 | ... == ... | == | 0 | 34 | 34 |
859-
| test.cpp:31:7:31:13 | ... == ... | test.cpp:31:7:31:13 | ... == ... | == | 1 | 30 | 30 |
860858
| test.cpp:31:7:31:13 | ... == ... | test.cpp:31:7:31:13 | ... == ... | == | 1 | 31 | 32 |
861859
| test.cpp:42:13:42:20 | call to getABool | test.cpp:42:13:42:20 | call to getABool | != | 0 | 43 | 45 |
862860
| test.cpp:42:13:42:20 | call to getABool | test.cpp:42:13:42:20 | call to getABool | == | 1 | 43 | 45 |

‎cpp/ql/test/library-tests/controlflow/guards/GuardsControl.expected‎

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -105,9 +105,13 @@
105105
| test.c:126:7:126:7 | 1 | true | test.c:131:10:132:16 | { ... } |
106106
| test.c:126:7:126:7 | 1 | true | test.c:134:1:123:10 | return ... |
107107
| test.c:126:7:126:28 | ... && ... | not 0 | test.c:126:31:128:16 | { ... } |
108+
| test.c:126:7:126:28 | ... && ... | not 0 | test.c:131:3:131:7 | if (...) ... |
108109
| test.c:126:7:126:28 | ... && ... | not 0 | test.c:131:10:132:16 | { ... } |
110+
| test.c:126:7:126:28 | ... && ... | not 0 | test.c:134:1:123:10 | return ... |
109111
| test.c:126:7:126:28 | ... && ... | true | test.c:126:31:128:16 | { ... } |
112+
| test.c:126:7:126:28 | ... && ... | true | test.c:131:3:131:7 | if (...) ... |
110113
| test.c:126:7:126:28 | ... && ... | true | test.c:131:10:132:16 | { ... } |
114+
| test.c:126:7:126:28 | ... && ... | true | test.c:134:1:123:10 | return ... |
111115
| test.c:126:12:126:26 | call to test3_condition | not 0 | test.c:126:31:128:16 | { ... } |
112116
| test.c:126:12:126:26 | call to test3_condition | not 0 | test.c:131:10:132:16 | { ... } |
113117
| test.c:126:12:126:26 | call to test3_condition | true | test.c:126:31:128:16 | { ... } |
@@ -162,17 +166,11 @@
162166
| test.c:219:9:219:22 | call to __builtin_expect | true | test.c:219:25:221:5 | { ... } |
163167
| test.cpp:18:8:18:10 | call to get | not null | test.cpp:19:5:19:14 | ExprStmt |
164168
| test.cpp:18:8:18:10 | call to get | true | test.cpp:19:5:19:14 | ExprStmt |
165-
| test.cpp:30:22:30:22 | x | -1 | test.cpp:30:6:30:16 | doSomething |
166169
| test.cpp:30:22:30:22 | x | -1 | test.cpp:31:16:32:21 | { ... } |
167-
| test.cpp:30:22:30:22 | x | not -1 | test.cpp:30:6:30:16 | doSomething |
168170
| test.cpp:30:22:30:22 | x | not -1 | test.cpp:34:1:34:1 | return ... |
169-
| test.cpp:31:7:31:7 | x | -1 | test.cpp:30:6:30:16 | doSomething |
170171
| test.cpp:31:7:31:7 | x | -1 | test.cpp:31:16:32:21 | { ... } |
171-
| test.cpp:31:7:31:7 | x | not -1 | test.cpp:30:6:30:16 | doSomething |
172172
| test.cpp:31:7:31:7 | x | not -1 | test.cpp:34:1:34:1 | return ... |
173-
| test.cpp:31:7:31:13 | ... == ... | false | test.cpp:30:6:30:16 | doSomething |
174173
| test.cpp:31:7:31:13 | ... == ... | false | test.cpp:34:1:34:1 | return ... |
175-
| test.cpp:31:7:31:13 | ... == ... | true | test.cpp:30:6:30:16 | doSomething |
176174
| test.cpp:31:7:31:13 | ... == ... | true | test.cpp:31:16:32:21 | { ... } |
177175
| test.cpp:42:13:42:20 | call to getABool | true | test.cpp:43:9:45:23 | { ... } |
178176
| test.cpp:60:31:60:31 | i | 0 | test.cpp:62:5:64:12 | case ...: |
@@ -408,19 +406,11 @@
408406
| test.cpp:400:11:400:25 | call to testEnumWrapper | 2 | test.cpp:404:5:406:12 | case ...: |
409407
| test.cpp:400:27:400:27 | b | false | test.cpp:404:5:406:12 | case ...: |
410408
| test.cpp:400:27:400:27 | b | true | test.cpp:401:5:403:12 | case ...: |
411-
| test.cpp:410:26:410:26 | o | not null | test.cpp:410:6:410:18 | ensureNotNull |
412409
| test.cpp:410:26:410:26 | o | not null | test.cpp:412:1:412:1 | return ... |
413-
| test.cpp:410:26:410:26 | o | null | test.cpp:410:6:410:18 | ensureNotNull |
414410
| test.cpp:410:26:410:26 | o | null | test.cpp:411:11:411:18 | ExprStmt |
415-
| test.cpp:411:7:411:8 | ! ... | false | test.cpp:410:6:410:18 | ensureNotNull |
416411
| test.cpp:411:7:411:8 | ! ... | false | test.cpp:412:1:412:1 | return ... |
417-
| test.cpp:411:7:411:8 | ! ... | true | test.cpp:410:6:410:18 | ensureNotNull |
418412
| test.cpp:411:7:411:8 | ! ... | true | test.cpp:411:11:411:18 | ExprStmt |
419-
| test.cpp:411:8:411:8 | o | false | test.cpp:410:6:410:18 | ensureNotNull |
420413
| test.cpp:411:8:411:8 | o | false | test.cpp:411:11:411:18 | ExprStmt |
421-
| test.cpp:411:8:411:8 | o | not null | test.cpp:410:6:410:18 | ensureNotNull |
422414
| test.cpp:411:8:411:8 | o | not null | test.cpp:412:1:412:1 | return ... |
423-
| test.cpp:411:8:411:8 | o | null | test.cpp:410:6:410:18 | ensureNotNull |
424415
| test.cpp:411:8:411:8 | o | null | test.cpp:411:11:411:18 | ExprStmt |
425-
| test.cpp:411:8:411:8 | o | true | test.cpp:410:6:410:18 | ensureNotNull |
426416
| test.cpp:411:8:411:8 | o | true | test.cpp:412:1:412:1 | return ... |

0 commit comments

Comments
 (0)