From 8c9c316e1b998963ba696884dff6575e305ff4c4 Mon Sep 17 00:00:00 2001 From: Robert Marsh Date: Fri, 14 Dec 2018 14:59:25 -0800 Subject: [PATCH] C++: performance and termination fixes --- .../aliased_ssa/gvn/ValueNumbering.qll | 2 +- .../semmle/code/cpp/rangeanalysis/Bound.qll | 3 +- .../code/cpp/rangeanalysis/RangeAnalysis.qll | 109 ++++++++++-------- .../code/cpp/rangeanalysis/RangeUtils.qll | 22 ++++ .../rangeanalysis/RangeAnalysis.expected | 11 +- .../rangeanalysis/rangeanalysis/test.cpp | 33 ++++++ 6 files changed, 127 insertions(+), 53 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/gvn/ValueNumbering.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/gvn/ValueNumbering.qll index 9a14a8288fa..c03ddf99162 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/gvn/ValueNumbering.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/gvn/ValueNumbering.qll @@ -216,7 +216,7 @@ private predicate uniqueValueNumber(Instruction instr, FunctionIR funcIR) { /** * Gets the value number assigned to `instr`, if any. Returns at most one result. */ -ValueNumber valueNumber(Instruction instr) { +cached ValueNumber valueNumber(Instruction instr) { result = nonUniqueValueNumber(instr) or exists(FunctionIR funcIR | uniqueValueNumber(instr, funcIR) and diff --git a/cpp/ql/src/semmle/code/cpp/rangeanalysis/Bound.qll b/cpp/ql/src/semmle/code/cpp/rangeanalysis/Bound.qll index 8d2fb592b90..54a825e2ffc 100644 --- a/cpp/ql/src/semmle/code/cpp/rangeanalysis/Bound.qll +++ b/cpp/ql/src/semmle/code/cpp/rangeanalysis/Bound.qll @@ -10,7 +10,8 @@ private newtype TBound = ( i.getResultType() instanceof IntegralType or i.getResultType() instanceof PointerType - ) + ) and + not vn.getAnInstruction() instanceof ConstantInstruction | i instanceof PhiInstruction or diff --git a/cpp/ql/src/semmle/code/cpp/rangeanalysis/RangeAnalysis.qll b/cpp/ql/src/semmle/code/cpp/rangeanalysis/RangeAnalysis.qll index 1eea467a28e..6c133425166 100644 --- a/cpp/ql/src/semmle/code/cpp/rangeanalysis/RangeAnalysis.qll +++ b/cpp/ql/src/semmle/code/cpp/rangeanalysis/RangeAnalysis.qll @@ -197,6 +197,7 @@ class CondReason extends Reason, TCondReason { * Holds if a cast from `fromtyp` to `totyp` can be ignored for the purpose of * range analysis. */ +pragma[inline] private predicate safeCast(IntegralType fromtyp, IntegralType totyp) { fromtyp.getSize() < totyp.getSize() and ( @@ -256,7 +257,7 @@ private class NarrowingCastInstruction extends ConvertInstruction { * - `upper = true` : `i <= op + delta` * - `upper = false` : `i >= op + delta` */ -private predicate boundFlowStep(Instruction i, Operand op, int delta, boolean upper) { +private predicate boundFlowStep(Instruction i, NonPhiOperand op, int delta, boolean upper) { valueFlowStep(i, op, delta) and (upper = true or upper = false) or @@ -425,12 +426,27 @@ private predicate boundedPhiOperand( ) } +/** Holds if `v != e + delta` at `pos`. */ +private predicate unequalFlowStep( + Operand op1, Operand op2, int delta, Reason reason +) { + exists(IRGuardCondition guard, boolean testIsTrue | + guard = eqFlowCond(valueNumberOfOperand(op1), op2, delta, false, testIsTrue) and + guard.controls(op1.getInstruction().getBlock(), testIsTrue) and + reason = TCondReason(guard) + ) +} + /** * Holds if `op != b + delta` at `pos`. */ private predicate unequalOperand(Operand op, Bound b, int delta, Reason reason) { - // TODO: implement this - none() + exists(Operand op2, int d1, int d2 | + unequalFlowStep(op, op2, delta, reason) and + boundedNonPhiOperand(op2, b, d2, true, _, _, _) and + boundedNonPhiOperand(op2, b, d2, true, _, _, _) and + delta = d1 + d2 + ) } private predicate boundedPhiCandValidForEdge( @@ -544,50 +560,47 @@ private predicate boundedCastExpr( private predicate boundedInstruction( Instruction i, Bound b, int delta, boolean upper, boolean fromBackEdge, int origdelta, Reason reason ) { - i instanceof PhiInstruction and - forex(PhiOperand op | op = i.getAnOperand() | - boundedPhiCandValidForEdge(i, b, delta, upper, fromBackEdge, origdelta, reason, op) - ) - or - i = b.getInstruction(delta) and - (upper = true or upper = false) and - fromBackEdge = false and - origdelta = delta and - reason = TNoReason() - or - exists(Operand mid, int d1, int d2 | - boundFlowStep(i, mid, d1, upper) and - boundedNonPhiOperand(mid, b, d2, upper, fromBackEdge, origdelta, reason) and - delta = d1 + d2 and - not exists(getValue(getConstantValue(i))) - ) - or - exists(Operand mid, int factor, int d | - boundFlowStepMul(i, mid, factor) and - boundedNonPhiOperand(mid, b, d, upper, fromBackEdge, origdelta, reason) and - b instanceof ZeroBound and - delta = d*factor and - not exists(getValue(getConstantValue(i))) - ) - or - exists(Operand mid, int factor, int d | - boundFlowStepDiv(i, mid, factor) and - boundedNonPhiOperand(mid, b, d, upper, fromBackEdge, origdelta, reason) and - d >= 0 and - b instanceof ZeroBound and - delta = d / factor and - not exists(getValue(getConstantValue(i))) - ) - or - exists(NarrowingCastInstruction cast | - cast = i and - safeNarrowingCast(cast, upper.booleanNot()) and - boundedCastExpr(cast, b, delta, upper, fromBackEdge, origdelta, reason) + isReducibleCFG(i.getFunction()) and + ( + i instanceof PhiInstruction and + forex(PhiOperand op | op = i.getAnOperand() | + boundedPhiCandValidForEdge(i, b, delta, upper, fromBackEdge, origdelta, reason, op) + ) + or + i = b.getInstruction(delta) and + (upper = true or upper = false) and + fromBackEdge = false and + origdelta = delta and + reason = TNoReason() + or + exists(Operand mid, int d1, int d2 | + boundFlowStep(i, mid, d1, upper) and + boundedNonPhiOperand(mid, b, d2, upper, fromBackEdge, origdelta, reason) and + delta = d1 + d2 and + not exists(getValue(getConstantValue(i))) + ) + or + exists(Operand mid, int factor, int d | + boundFlowStepMul(i, mid, factor) and + boundedNonPhiOperand(mid, b, d, upper, fromBackEdge, origdelta, reason) and + b instanceof ZeroBound and + delta = d*factor and + not exists(getValue(getConstantValue(i))) + ) + or + exists(Operand mid, int factor, int d | + boundFlowStepDiv(i, mid, factor) and + boundedNonPhiOperand(mid, b, d, upper, fromBackEdge, origdelta, reason) and + d >= 0 and + b instanceof ZeroBound and + delta = d / factor and + not exists(getValue(getConstantValue(i))) + ) + or + exists(NarrowingCastInstruction cast | + cast = i and + safeNarrowingCast(cast, upper.booleanNot()) and + boundedCastExpr(cast, b, delta, upper, fromBackEdge, origdelta, reason) + ) ) } - -predicate backEdge(PhiInstruction phi, PhiOperand op) { - phi.getAnOperand() = op and - phi.getBlock().dominates(op.getPredecessorBlock()) - // TODO: identify backedges during IR construction -} \ No newline at end of file diff --git a/cpp/ql/src/semmle/code/cpp/rangeanalysis/RangeUtils.qll b/cpp/ql/src/semmle/code/cpp/rangeanalysis/RangeUtils.qll index 4f76fbb6f89..b02914033a7 100644 --- a/cpp/ql/src/semmle/code/cpp/rangeanalysis/RangeUtils.qll +++ b/cpp/ql/src/semmle/code/cpp/rangeanalysis/RangeUtils.qll @@ -62,3 +62,25 @@ predicate valueFlowStep(Instruction i, Operand op, int delta) { -getValue(getConstantValue(x.getDefinitionInstruction())) ) } + +predicate isReducibleCFG(Function f) { + not exists(LabelStmt l, GotoStmt goto | + goto.getTarget() = l and + l.getLocation().isBefore(goto.getLocation()) and + l.getEnclosingFunction() = f + ) and + not exists(LabelStmt ls, Loop l | + ls.getParent*() = l and + l.getEnclosingFunction() = f + ) and + not exists(SwitchCase cs | + cs.getSwitchStmt().getStmt() != cs.getParentStmt() and + cs.getEnclosingFunction() = f + ) +} + +predicate backEdge(PhiInstruction phi, PhiOperand op) { + phi.getAnOperand() = op and + phi.getBlock().dominates(op.getPredecessorBlock()) + // TODO: identify backedges during IR construction +} \ No newline at end of file diff --git a/cpp/ql/test/library-tests/rangeanalysis/rangeanalysis/RangeAnalysis.expected b/cpp/ql/test/library-tests/rangeanalysis/rangeanalysis/RangeAnalysis.expected index 5baab29f442..24429da0362 100644 --- a/cpp/ql/test/library-tests/rangeanalysis/rangeanalysis/RangeAnalysis.expected +++ b/cpp/ql/test/library-tests/rangeanalysis/rangeanalysis/RangeAnalysis.expected @@ -35,6 +35,11 @@ | test.cpp:97:10:97:10 | Load: x | file://:0:0:0:0 | 0 | 1 | false | CompareLT: ... < ... | test.cpp:94:7:94:11 | test.cpp:94:7:94:11 | | test.cpp:100:10:100:10 | Load: x | file://:0:0:0:0 | 0 | 1 | true | CompareLE: ... <= ... | test.cpp:99:7:99:12 | test.cpp:99:7:99:12 | | test.cpp:102:10:102:10 | Load: x | file://:0:0:0:0 | 0 | 2 | false | CompareLE: ... <= ... | test.cpp:99:7:99:12 | test.cpp:99:7:99:12 | -| test.cpp:106:5:106:10 | Phi: test10 | test.cpp:113:3:113:6 | Phi: call to sink | -1 | true | CompareLT: ... < ... | test.cpp:114:18:114:22 | test.cpp:114:18:114:22 | -| test.cpp:106:5:106:10 | Phi: test10 | test.cpp:114:18:114:18 | Phi: i | 0 | false | NoReason | file://:0:0:0:0 | file://:0:0:0:0 | -| test.cpp:106:5:106:10 | Phi: test10 | test.cpp:114:18:114:18 | Phi: i | 0 | true | NoReason | file://:0:0:0:0 | file://:0:0:0:0 | +| test.cpp:107:5:107:10 | Phi: test10 | test.cpp:114:3:114:6 | Phi: call to sink | -1 | true | CompareLT: ... < ... | test.cpp:115:18:115:22 | test.cpp:115:18:115:22 | +| test.cpp:140:10:140:10 | Store: i | file://:0:0:0:0 | 0 | 1 | false | NoReason | file://:0:0:0:0 | file://:0:0:0:0 | +| test.cpp:140:10:140:10 | Store: i | test.cpp:135:16:135:16 | InitializeParameter: x | 0 | false | CompareLT: ... < ... | test.cpp:139:11:139:15 | test.cpp:139:11:139:15 | +| test.cpp:140:10:140:10 | Store: i | test.cpp:138:5:138:5 | Phi: i | 1 | false | NoReason | file://:0:0:0:0 | file://:0:0:0:0 | +| test.cpp:140:10:140:10 | Store: i | test.cpp:138:5:138:5 | Phi: i | 1 | true | NoReason | file://:0:0:0:0 | file://:0:0:0:0 | +| test.cpp:149:10:149:10 | Store: i | file://:0:0:0:0 | 0 | 1 | false | NoReason | file://:0:0:0:0 | file://:0:0:0:0 | +| test.cpp:149:10:149:10 | Store: i | test.cpp:147:5:147:5 | Phi: i | 1 | false | NoReason | file://:0:0:0:0 | file://:0:0:0:0 | +| test.cpp:149:10:149:10 | Store: i | test.cpp:147:5:147:5 | Phi: i | 1 | true | NoReason | file://:0:0:0:0 | file://:0:0:0:0 | diff --git a/cpp/ql/test/library-tests/rangeanalysis/rangeanalysis/test.cpp b/cpp/ql/test/library-tests/rangeanalysis/rangeanalysis/test.cpp index daf53764d99..a9407b6e602 100644 --- a/cpp/ql/test/library-tests/rangeanalysis/rangeanalysis/test.cpp +++ b/cpp/ql/test/library-tests/rangeanalysis/rangeanalysis/test.cpp @@ -103,6 +103,7 @@ void test9(int x) { } } +// Phi nodes as bounds int test10(int y, int z, bool use_y) { int x; if(use_y) { @@ -115,3 +116,35 @@ int test10(int y, int z, bool use_y) { return i; } } + +// Irreducible CFGs +int test11(int y, int x) { + int i = 0; + if (x < y) { + x = y; + } else { + goto inLoop; + } + for(i = 0; i < x; i++) { + inLoop: + sink(i); + } +} + +// do-while +int test12(int x) { + int i = 0; + do { + i++; + } while(i < x); + return i; +} + +// do while false +int test13(int x) { + int i = 0; + do { + i++; + } while(false); + return i; +}