Skip to content

Commit e1c3266

Browse files
MathiasVPMathiasVP
authored
Merge pull request #14894 from MathiasVP/dont-short-circuit-negation-in-conditions
C++: Don't short circuit logical negation in conditions
2 parents 6f9a704 + 9731985 commit e1c3266

File tree

10 files changed

+169
-177
lines changed

10 files changed

+169
-177
lines changed

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

Lines changed: 0 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -30,11 +30,6 @@ class GuardCondition extends Expr {
3030
or
3131
// no binary operators in the IR
3232
this.(BinaryLogicalOperation).getAnOperand() instanceof GuardCondition
33-
or
34-
// the IR short-circuits if(!x)
35-
// don't produce a guard condition for `y = !x` and other non-short-circuited cases
36-
not exists(Instruction inst | this.getFullyConverted() = inst.getAst()) and
37-
exists(IRGuardCondition ir | this.(NotExpr).getOperand() = ir.getAst())
3833
}
3934

4035
/**
@@ -140,39 +135,6 @@ private class GuardConditionFromBinaryLogicalOperator extends GuardCondition {
140135
}
141136
}
142137

143-
/**
144-
* A `!` operator in the AST that guards one or more basic blocks, and does not have a corresponding
145-
* IR instruction.
146-
*/
147-
private class GuardConditionFromShortCircuitNot extends GuardCondition, NotExpr {
148-
GuardConditionFromShortCircuitNot() {
149-
not exists(Instruction inst | this.getFullyConverted() = inst.getAst()) and
150-
exists(IRGuardCondition ir | this.getOperand() = ir.getAst())
151-
}
152-
153-
override predicate controls(BasicBlock controlled, boolean testIsTrue) {
154-
this.getOperand().(GuardCondition).controls(controlled, testIsTrue.booleanNot())
155-
}
156-
157-
override predicate comparesLt(Expr left, Expr right, int k, boolean isLessThan, boolean testIsTrue) {
158-
this.getOperand()
159-
.(GuardCondition)
160-
.comparesLt(left, right, k, isLessThan, testIsTrue.booleanNot())
161-
}
162-
163-
override predicate ensuresLt(Expr left, Expr right, int k, BasicBlock block, boolean isLessThan) {
164-
this.getOperand().(GuardCondition).ensuresLt(left, right, k, block, isLessThan.booleanNot())
165-
}
166-
167-
override predicate comparesEq(Expr left, Expr right, int k, boolean areEqual, boolean testIsTrue) {
168-
this.getOperand().(GuardCondition).comparesEq(left, right, k, areEqual, testIsTrue.booleanNot())
169-
}
170-
171-
override predicate ensuresEq(Expr left, Expr right, int k, BasicBlock block, boolean areEqual) {
172-
this.getOperand().(GuardCondition).ensuresEq(left, right, k, block, areEqual.booleanNot())
173-
}
174-
}
175-
176138
/**
177139
* A Boolean condition in the AST that guards one or more basic blocks and has a corresponding IR
178140
* instruction.

‎cpp/ql/lib/semmle/code/cpp/ir/implementation/aliased_ssa/constant/ConstantAnalysis.qll‎

Lines changed: 23 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,9 @@ int getConstantValue(Instruction instr) {
1212
or
1313
result = getConstantValue(instr.(CopyInstruction).getSourceValue())
1414
or
15+
getConstantValue(instr.(LogicalNotInstruction).getUnary()) != 0 and
16+
result = 0
17+
or
1518
exists(PhiInstruction phi |
1619
phi = instr and
1720
result = unique(Operand op | op = phi.getAnInputOperand() | getConstantValue(op.getDef()))
@@ -26,28 +29,25 @@ private predicate binaryInstructionOperands(BinaryInstruction instr, int left, i
2629

2730
pragma[noinline]
2831
private int getBinaryInstructionValue(BinaryInstruction instr) {
29-
exists(int left, int right |
30-
binaryInstructionOperands(instr, left, right) and
31-
(
32-
instr instanceof AddInstruction and result = add(left, right)
33-
or
34-
instr instanceof SubInstruction and result = sub(left, right)
35-
or
36-
instr instanceof MulInstruction and result = mul(left, right)
37-
or
38-
instr instanceof DivInstruction and result = div(left, right)
39-
or
40-
instr instanceof CompareEQInstruction and result = compareEQ(left, right)
41-
or
42-
instr instanceof CompareNEInstruction and result = compareNE(left, right)
43-
or
44-
instr instanceof CompareLTInstruction and result = compareLT(left, right)
45-
or
46-
instr instanceof CompareGTInstruction and result = compareGT(left, right)
47-
or
48-
instr instanceof CompareLEInstruction and result = compareLE(left, right)
49-
or
50-
instr instanceof CompareGEInstruction and result = compareGE(left, right)
51-
)
32+
exists(int left, int right | binaryInstructionOperands(instr, left, right) |
33+
instr instanceof AddInstruction and result = add(left, right)
34+
or
35+
instr instanceof SubInstruction and result = sub(left, right)
36+
or
37+
instr instanceof MulInstruction and result = mul(left, right)
38+
or
39+
instr instanceof DivInstruction and result = div(left, right)
40+
or
41+
instr instanceof CompareEQInstruction and result = compareEQ(left, right)
42+
or
43+
instr instanceof CompareNEInstruction and result = compareNE(left, right)
44+
or
45+
instr instanceof CompareLTInstruction and result = compareLT(left, right)
46+
or
47+
instr instanceof CompareGTInstruction and result = compareGT(left, right)
48+
or
49+
instr instanceof CompareLEInstruction and result = compareLE(left, right)
50+
or
51+
instr instanceof CompareGEInstruction and result = compareGE(left, right)
5252
)
5353
}

‎cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/constant/ConstantAnalysis.qll‎

Lines changed: 23 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,9 @@ int getConstantValue(Instruction instr) {
1212
or
1313
result = getConstantValue(instr.(CopyInstruction).getSourceValue())
1414
or
15+
getConstantValue(instr.(LogicalNotInstruction).getUnary()) != 0 and
16+
result = 0
17+
or
1518
exists(PhiInstruction phi |
1619
phi = instr and
1720
result = unique(Operand op | op = phi.getAnInputOperand() | getConstantValue(op.getDef()))
@@ -26,28 +29,25 @@ private predicate binaryInstructionOperands(BinaryInstruction instr, int left, i
2629

2730
pragma[noinline]
2831
private int getBinaryInstructionValue(BinaryInstruction instr) {
29-
exists(int left, int right |
30-
binaryInstructionOperands(instr, left, right) and
31-
(
32-
instr instanceof AddInstruction and result = add(left, right)
33-
or
34-
instr instanceof SubInstruction and result = sub(left, right)
35-
or
36-
instr instanceof MulInstruction and result = mul(left, right)
37-
or
38-
instr instanceof DivInstruction and result = div(left, right)
39-
or
40-
instr instanceof CompareEQInstruction and result = compareEQ(left, right)
41-
or
42-
instr instanceof CompareNEInstruction and result = compareNE(left, right)
43-
or
44-
instr instanceof CompareLTInstruction and result = compareLT(left, right)
45-
or
46-
instr instanceof CompareGTInstruction and result = compareGT(left, right)
47-
or
48-
instr instanceof CompareLEInstruction and result = compareLE(left, right)
49-
or
50-
instr instanceof CompareGEInstruction and result = compareGE(left, right)
51-
)
32+
exists(int left, int right | binaryInstructionOperands(instr, left, right) |
33+
instr instanceof AddInstruction and result = add(left, right)
34+
or
35+
instr instanceof SubInstruction and result = sub(left, right)
36+
or
37+
instr instanceof MulInstruction and result = mul(left, right)
38+
or
39+
instr instanceof DivInstruction and result = div(left, right)
40+
or
41+
instr instanceof CompareEQInstruction and result = compareEQ(left, right)
42+
or
43+
instr instanceof CompareNEInstruction and result = compareNE(left, right)
44+
or
45+
instr instanceof CompareLTInstruction and result = compareLT(left, right)
46+
or
47+
instr instanceof CompareGTInstruction and result = compareGT(left, right)
48+
or
49+
instr instanceof CompareLEInstruction and result = compareLE(left, right)
50+
or
51+
instr instanceof CompareGEInstruction and result = compareGE(left, right)
5252
)
5353
}

‎cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedCondition.qll‎

Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -77,24 +77,6 @@ class TranslatedParenthesisCondition extends TranslatedFlexibleCondition {
7777
}
7878
}
7979

80-
class TranslatedNotCondition extends TranslatedFlexibleCondition {
81-
override NotExpr expr;
82-
83-
override Instruction getChildTrueSuccessor(TranslatedCondition child) {
84-
child = this.getOperand() and
85-
result = this.getConditionContext().getChildFalseSuccessor(this)
86-
}
87-
88-
override Instruction getChildFalseSuccessor(TranslatedCondition child) {
89-
child = this.getOperand() and
90-
result = this.getConditionContext().getChildTrueSuccessor(this)
91-
}
92-
93-
override TranslatedCondition getOperand() {
94-
result = getTranslatedCondition(expr.getOperand().getFullyConverted())
95-
}
96-
}
97-
9880
abstract class TranslatedNativeCondition extends TranslatedCondition, TTranslatedNativeCondition {
9981
TranslatedNativeCondition() { this = TTranslatedNativeCondition(expr) }
10082

‎cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedElement.qll‎

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -190,10 +190,7 @@ private predicate isNativeCondition(Expr expr) {
190190
* depending on context.
191191
*/
192192
private predicate isFlexibleCondition(Expr expr) {
193-
(
194-
expr instanceof ParenthesisExpr or
195-
expr instanceof NotExpr
196-
) and
193+
expr instanceof ParenthesisExpr and
197194
usedAsCondition(expr) and
198195
not isIRConstant(expr)
199196
}
@@ -218,11 +215,6 @@ private predicate usedAsCondition(Expr expr) {
218215
condExpr.getCondition().getFullyConverted() = expr and not condExpr.isTwoOperand()
219216
)
220217
or
221-
exists(NotExpr notExpr |
222-
notExpr.getOperand().getFullyConverted() = expr and
223-
usedAsCondition(notExpr)
224-
)
225-
or
226218
exists(ParenthesisExpr paren |
227219
paren.getExpr() = expr and
228220
usedAsCondition(paren)

‎cpp/ql/lib/semmle/code/cpp/ir/implementation/unaliased_ssa/constant/ConstantAnalysis.qll‎

Lines changed: 23 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,9 @@ int getConstantValue(Instruction instr) {
1212
or
1313
result = getConstantValue(instr.(CopyInstruction).getSourceValue())
1414
or
15+
getConstantValue(instr.(LogicalNotInstruction).getUnary()) != 0 and
16+
result = 0
17+
or
1518
exists(PhiInstruction phi |
1619
phi = instr and
1720
result = unique(Operand op | op = phi.getAnInputOperand() | getConstantValue(op.getDef()))
@@ -26,28 +29,25 @@ private predicate binaryInstructionOperands(BinaryInstruction instr, int left, i
2629

2730
pragma[noinline]
2831
private int getBinaryInstructionValue(BinaryInstruction instr) {
29-
exists(int left, int right |
30-
binaryInstructionOperands(instr, left, right) and
31-
(
32-
instr instanceof AddInstruction and result = add(left, right)
33-
or
34-
instr instanceof SubInstruction and result = sub(left, right)
35-
or
36-
instr instanceof MulInstruction and result = mul(left, right)
37-
or
38-
instr instanceof DivInstruction and result = div(left, right)
39-
or
40-
instr instanceof CompareEQInstruction and result = compareEQ(left, right)
41-
or
42-
instr instanceof CompareNEInstruction and result = compareNE(left, right)
43-
or
44-
instr instanceof CompareLTInstruction and result = compareLT(left, right)
45-
or
46-
instr instanceof CompareGTInstruction and result = compareGT(left, right)
47-
or
48-
instr instanceof CompareLEInstruction and result = compareLE(left, right)
49-
or
50-
instr instanceof CompareGEInstruction and result = compareGE(left, right)
51-
)
32+
exists(int left, int right | binaryInstructionOperands(instr, left, right) |
33+
instr instanceof AddInstruction and result = add(left, right)
34+
or
35+
instr instanceof SubInstruction and result = sub(left, right)
36+
or
37+
instr instanceof MulInstruction and result = mul(left, right)
38+
or
39+
instr instanceof DivInstruction and result = div(left, right)
40+
or
41+
instr instanceof CompareEQInstruction and result = compareEQ(left, right)
42+
or
43+
instr instanceof CompareNEInstruction and result = compareNE(left, right)
44+
or
45+
instr instanceof CompareLTInstruction and result = compareLT(left, right)
46+
or
47+
instr instanceof CompareGTInstruction and result = compareGT(left, right)
48+
or
49+
instr instanceof CompareLEInstruction and result = compareLE(left, right)
50+
or
51+
instr instanceof CompareGEInstruction and result = compareGE(left, right)
5252
)
5353
}

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -450,6 +450,7 @@ irGuards
450450
| test.c:126:12:126:26 | Call: call to test3_condition |
451451
| test.c:131:7:131:7 | Load: b |
452452
| test.c:137:7:137:7 | Constant: 0 |
453+
| test.c:146:7:146:8 | LogicalNot: ! ... |
453454
| test.c:146:8:146:8 | Load: x |
454455
| test.c:152:10:152:10 | Load: x |
455456
| test.c:152:15:152:15 | Load: y |
@@ -640,6 +641,7 @@ irGuardsControl
640641
| test.c:126:12:126:26 | Call: call to test3_condition | true | 127 | 127 |
641642
| test.c:131:7:131:7 | Load: b | true | 132 | 132 |
642643
| test.c:137:7:137:7 | Constant: 0 | false | 142 | 142 |
644+
| test.c:146:7:146:8 | LogicalNot: ! ... | true | 147 | 147 |
643645
| test.c:146:8:146:8 | Load: x | false | 147 | 147 |
644646
| test.c:152:10:152:10 | Load: x | true | 152 | 152 |
645647
| test.c:152:15:152:15 | Load: y | true | 152 | 152 |

‎cpp/ql/test/library-tests/ir/ir/aliased_ir.expected‎

Lines changed: 41 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -2770,43 +2770,65 @@ ir.cpp:
27702770
# 462| m462_2(int) = Uninitialized[x] : &:r462_1
27712771
# 463| r463_1(glval<bool>) = VariableAddress[a] :
27722772
# 463| r463_2(bool) = Load[a] : &:r463_1, m461_6
2773-
# 463| v463_3(void) = ConditionalBranch : r463_2
2774-
#-----| False -> Block 1
2775-
#-----| True -> Block 2
2773+
# 463| r463_3(bool) = LogicalNot : r463_2
2774+
# 463| v463_4(void) = ConditionalBranch : r463_3
2775+
#-----| False -> Block 5
2776+
#-----| True -> Block 1
27762777

27772778
# 464| Block 1
27782779
# 464| r464_1(int) = Constant[1] :
27792780
# 464| r464_2(glval<int>) = VariableAddress[x] :
27802781
# 464| m464_3(int) = Store[x] : &:r464_2, r464_1
2781-
#-----| Goto -> Block 2
2782+
#-----| Goto -> Block 5
27822783

27832784
# 467| Block 2
2784-
# 467| r467_1(glval<bool>) = VariableAddress[a] :
2785-
# 467| r467_2(bool) = Load[a] : &:r467_1, m461_6
2786-
# 467| v467_3(void) = ConditionalBranch : r467_2
2787-
#-----| False -> Block 4
2788-
#-----| True -> Block 3
2785+
# 467| r467_1(glval<bool>) = VariableAddress[#temp467:11] :
2786+
# 467| r467_2(bool) = Constant[0] :
2787+
# 467| m467_3(bool) = Store[#temp467:11] : &:r467_1, r467_2
2788+
#-----| Goto -> Block 3
27892789

27902790
# 467| Block 3
2791-
# 467| r467_4(glval<bool>) = VariableAddress[b] :
2792-
# 467| r467_5(bool) = Load[b] : &:r467_4, m461_8
2793-
# 467| v467_6(void) = ConditionalBranch : r467_5
2794-
#-----| False -> Block 4
2795-
#-----| True -> Block 5
2791+
# 467| m467_4(bool) = Phi : from 2:m467_3, from 4:m467_11
2792+
# 467| r467_5(glval<bool>) = VariableAddress[#temp467:11] :
2793+
# 467| r467_6(bool) = Load[#temp467:11] : &:r467_5, m467_4
2794+
# 467| r467_7(bool) = LogicalNot : r467_6
2795+
# 467| v467_8(void) = ConditionalBranch : r467_7
2796+
#-----| False -> Block 8
2797+
#-----| True -> Block 7
27962798

2797-
# 468| Block 4
2799+
# 467| Block 4
2800+
# 467| r467_9(glval<bool>) = VariableAddress[#temp467:11] :
2801+
# 467| r467_10(bool) = Constant[1] :
2802+
# 467| m467_11(bool) = Store[#temp467:11] : &:r467_9, r467_10
2803+
#-----| Goto -> Block 3
2804+
2805+
# 467| Block 5
2806+
# 467| r467_12(glval<bool>) = VariableAddress[a] :
2807+
# 467| r467_13(bool) = Load[a] : &:r467_12, m461_6
2808+
# 467| v467_14(void) = ConditionalBranch : r467_13
2809+
#-----| False -> Block 2
2810+
#-----| True -> Block 6
2811+
2812+
# 467| Block 6
2813+
# 467| r467_15(glval<bool>) = VariableAddress[b] :
2814+
# 467| r467_16(bool) = Load[b] : &:r467_15, m461_8
2815+
# 467| v467_17(void) = ConditionalBranch : r467_16
2816+
#-----| False -> Block 2
2817+
#-----| True -> Block 4
2818+
2819+
# 468| Block 7
27982820
# 468| r468_1(int) = Constant[2] :
27992821
# 468| r468_2(glval<int>) = VariableAddress[x] :
28002822
# 468| m468_3(int) = Store[x] : &:r468_2, r468_1
2801-
#-----| Goto -> Block 6
2823+
#-----| Goto -> Block 9
28022824

2803-
# 471| Block 5
2825+
# 471| Block 8
28042826
# 471| r471_1(int) = Constant[3] :
28052827
# 471| r471_2(glval<int>) = VariableAddress[x] :
28062828
# 471| m471_3(int) = Store[x] : &:r471_2, r471_1
2807-
#-----| Goto -> Block 6
2829+
#-----| Goto -> Block 9
28082830

2809-
# 473| Block 6
2831+
# 473| Block 9
28102832
# 473| v473_1(void) = NoOp :
28112833
# 461| v461_9(void) = ReturnVoid :
28122834
# 461| v461_10(void) = AliasedUse : m461_3

0 commit comments

Comments
 (0)