From 38f7ec7d181879b4f48ac489157e98b7d7757732 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Tue, 22 Jan 2019 13:07:35 +0100 Subject: [PATCH 01/12] C++: Initial implementation of back-edge detection --- .../ir/implementation/aliased_ssa/IRBlock.qll | 17 +++- .../aliased_ssa/Instruction.qll | 10 +++ .../aliased_ssa/internal/SSAConstruction.qll | 8 ++ .../cpp/ir/implementation/raw/IRBlock.qll | 17 +++- .../cpp/ir/implementation/raw/Instruction.qll | 10 +++ .../raw/internal/IRConstruction.qll | 87 +++++++++++++++++++ .../raw/internal/TranslatedStmt.qll | 2 +- .../implementation/unaliased_ssa/IRBlock.qll | 17 +++- .../unaliased_ssa/Instruction.qll | 10 +++ .../internal/SSAConstruction.qll | 8 ++ 10 files changed, 182 insertions(+), 4 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/IRBlock.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/IRBlock.qll index f94fad4f527c..c1354724a7f1 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/IRBlock.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/IRBlock.qll @@ -90,6 +90,10 @@ class IRBlock extends IRBlockBase { blockSuccessor(this, result, kind) } + final IRBlock getBackEdgeSuccessor(EdgeKind kind) { + backEdgeSuccessor(this, result, kind) + } + final predicate immediatelyDominates(IRBlock block) { blockImmediatelyDominates(this, block) } @@ -132,7 +136,10 @@ private predicate startsBasicBlock(Instruction instr) { exists(Instruction predecessor, EdgeKind kind | instr = predecessor.getSuccessor(kind) and not kind instanceof GotoEdge - ) // Incoming edge is not a GotoEdge + ) or // Incoming edge is not a GotoEdge + exists(Instruction predecessor | + instr = predecessor.getBackEdgeSuccessor(_) + ) // A back edge enters this instruction ) } @@ -184,6 +191,14 @@ private cached module Cached { ) } + cached predicate backEdgeSuccessor(TIRBlock pred, TIRBlock succ, EdgeKind kind) { + exists(Instruction predLast, Instruction succFirst | + predLast = getInstruction(pred, getInstructionCount(pred) - 1) and + succFirst = predLast.getBackEdgeSuccessor(kind) and + succ = MkIRBlock(succFirst) + ) + } + cached predicate blockSuccessor(TIRBlock pred, TIRBlock succ) { blockSuccessor(pred, succ, _) } diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll index dcad960d8dfd..ed370bc30887 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll @@ -463,6 +463,16 @@ class Instruction extends Construction::TInstruction { result = Construction::getInstructionSuccessor(this, kind) } + /** + * Gets the a _back-edge successor_ of this instruction along the control + * flow edge specified by `kind`. A back edge in the control-flow graph is + * intuitively the edge that goes back around a loop. If all back edges are + * removed from the control-flow graph, it becomes acyclic. + */ + final Instruction getBackEdgeSuccessor(EdgeKind kind) { + result = Construction::getInstructionBackEdgeSuccessor(this, kind) + } + /** * Gets all direct successors of this instruction. */ diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/SSAConstruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/SSAConstruction.qll index 88914e21c3ab..292f2c520651 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/SSAConstruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/SSAConstruction.qll @@ -278,6 +278,14 @@ cached private module Cached { ) } + cached Instruction getInstructionBackEdgeSuccessor(Instruction instruction, EdgeKind kind) { + exists(OldInstruction oldInstruction | + oldInstruction = getOldInstruction(instruction) and + result = getNewInstruction(oldInstruction.getBackEdgeSuccessor(kind)) and + not Reachability::isInfeasibleInstructionSuccessor(oldInstruction, kind) + ) + } + cached IRVariable getInstructionVariable(Instruction instruction) { result = getNewIRVariable(getOldInstruction(instruction).(OldIR::VariableInstruction).getVariable()) } diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/IRBlock.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/IRBlock.qll index f94fad4f527c..c1354724a7f1 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/IRBlock.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/IRBlock.qll @@ -90,6 +90,10 @@ class IRBlock extends IRBlockBase { blockSuccessor(this, result, kind) } + final IRBlock getBackEdgeSuccessor(EdgeKind kind) { + backEdgeSuccessor(this, result, kind) + } + final predicate immediatelyDominates(IRBlock block) { blockImmediatelyDominates(this, block) } @@ -132,7 +136,10 @@ private predicate startsBasicBlock(Instruction instr) { exists(Instruction predecessor, EdgeKind kind | instr = predecessor.getSuccessor(kind) and not kind instanceof GotoEdge - ) // Incoming edge is not a GotoEdge + ) or // Incoming edge is not a GotoEdge + exists(Instruction predecessor | + instr = predecessor.getBackEdgeSuccessor(_) + ) // A back edge enters this instruction ) } @@ -184,6 +191,14 @@ private cached module Cached { ) } + cached predicate backEdgeSuccessor(TIRBlock pred, TIRBlock succ, EdgeKind kind) { + exists(Instruction predLast, Instruction succFirst | + predLast = getInstruction(pred, getInstructionCount(pred) - 1) and + succFirst = predLast.getBackEdgeSuccessor(kind) and + succ = MkIRBlock(succFirst) + ) + } + cached predicate blockSuccessor(TIRBlock pred, TIRBlock succ) { blockSuccessor(pred, succ, _) } diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll index dcad960d8dfd..ed370bc30887 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll @@ -463,6 +463,16 @@ class Instruction extends Construction::TInstruction { result = Construction::getInstructionSuccessor(this, kind) } + /** + * Gets the a _back-edge successor_ of this instruction along the control + * flow edge specified by `kind`. A back edge in the control-flow graph is + * intuitively the edge that goes back around a loop. If all back edges are + * removed from the control-flow graph, it becomes acyclic. + */ + final Instruction getBackEdgeSuccessor(EdgeKind kind) { + result = Construction::getInstructionBackEdgeSuccessor(this, kind) + } + /** * Gets all direct successors of this instruction. */ diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/IRConstruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/IRConstruction.qll index 64a2a539ca28..089ec10e7637 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/IRConstruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/IRConstruction.qll @@ -5,6 +5,7 @@ private import semmle.code.cpp.ir.internal.TempVariableTag private import InstructionTag private import TranslatedElement private import TranslatedExpr +private import TranslatedStmt private import TranslatedFunction class InstructionTagType extends TInstructionTag { @@ -104,6 +105,92 @@ cached private module Cached { instruction.getTag(), kind) } + // This predicate has pragma[noopt] because otherwise the `getAChild*` calls + // get joined too early. The join order for the loop cases goes like this: + // - Find all loops of that type (tens of thousands). + // - Find all edges into the start of the loop (x 2). + // - Restrict to edges that originate within the loop (/ 2). + pragma[noopt] + cached Instruction getInstructionBackEdgeSuccessor(Instruction instruction, EdgeKind kind) { + // Any edge from within the body of the loop to the condition of the loop + // is a back edge. This includes edges from `continue` and the fall-through + // edge(s) after the last instruction(s) in the body. + exists(TranslatedWhileStmt s | + s instanceof TranslatedWhileStmt and + result = s.getFirstConditionInstruction() and + exists(TranslatedElement inBody, InstructionTag tag | + result = inBody.getInstructionSuccessor(tag, kind) and + exists(TranslatedElement body | body = s.getBody() | inBody = body.getAChild*()) and + instruction = inBody.getInstruction(tag) + ) + ) + or + // The back edge should be the (unique) edge from the condition to the + // body. This ensures that it's the back edge that will be pruned in a `do + // { ... } while (0)` statement. Note that all `continue` statements in a + // do-while loop produce forward edges. + exists(TranslatedDoStmt s | + s instanceof TranslatedDoStmt and + exists(TranslatedStmt body | body = s.getBody() | result = body.getFirstInstruction()) and + exists(TranslatedElement inCondition, InstructionTag tag | + result = inCondition.getInstructionSuccessor(tag, kind) and + exists(TranslatedElement condition | condition = s.getCondition() | + inCondition = condition.getAChild*() + ) and + instruction = inCondition.getInstruction(tag) + ) + ) + or + // Any edge from within the body or update of the loop to the condition of + // the loop is a back edge. This includes edges from `continue` and the + // fall-through edge(s) after the last instruction(s) in the body. A `for` + // loop may not have a condition, in which case + // `getFirstConditionInstruction` returns the body instead. + exists(TranslatedForStmt s | + s instanceof TranslatedForStmt and + result = s.getFirstConditionInstruction() and + exists(TranslatedElement inLoop, InstructionTag tag | + result = inLoop.getInstructionSuccessor(tag, kind) and + exists(TranslatedElement bodyOrUpdate | + bodyOrUpdate = s.getBody() + or + bodyOrUpdate = s.getUpdate() + | + inLoop = bodyOrUpdate.getAChild*() + ) and + instruction = inLoop.getInstruction(tag) + ) + ) + or + // As a conservative approximation, any edge out of `goto` is a back edge + // unless it goes strictly forward in the program text. A `goto` whose + // source and target are both inside a macro will be seen as having the + // same location for source and target, so we conservatively assume that + // such a `goto` creates a back edge. + exists(TranslatedElement s, GotoStmt goto | + goto instanceof GotoStmt and + not isStrictlyForwardGoto(goto) and + goto = s.getAST() and + exists(InstructionTag tag | + result = s.getInstructionSuccessor(tag, kind) and + instruction = s.getInstruction(tag) + ) + ) + } + + /** Holds if `goto` jumps strictly forward in the program text. */ + private predicate isStrictlyForwardGoto(GotoStmt goto) { + exists(string gotoFile, int gotoLine, string targetFile, int targetLine | + goto.getLocation().hasLocationInfo(gotoFile, gotoLine, _, _, _) and + goto.getTarget().getLocation().hasLocationInfo(targetFile, targetLine, _, _, _) and + targetLine > gotoLine and + // We avoid `=` for equality here since that might tempt the join orderer + // into joining on the file name too early. + gotoFile >= targetFile and + gotoFile <= targetFile + ) + } + cached IRVariable getInstructionVariable(Instruction instruction) { result = getInstructionTranslatedElement(instruction).getInstructionVariable( instruction.getTag()) diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedStmt.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedStmt.qll index 6e4bb2c6d1e2..c4eb18b0e6bd 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedStmt.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedStmt.qll @@ -632,7 +632,7 @@ class TranslatedForStmt extends TranslatedLoop { exists(forStmt.getInitialization()) } - private TranslatedExpr getUpdate() { + TranslatedExpr getUpdate() { result = getTranslatedExpr(forStmt.getUpdate().getFullyConverted()) } diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/IRBlock.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/IRBlock.qll index f94fad4f527c..c1354724a7f1 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/IRBlock.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/IRBlock.qll @@ -90,6 +90,10 @@ class IRBlock extends IRBlockBase { blockSuccessor(this, result, kind) } + final IRBlock getBackEdgeSuccessor(EdgeKind kind) { + backEdgeSuccessor(this, result, kind) + } + final predicate immediatelyDominates(IRBlock block) { blockImmediatelyDominates(this, block) } @@ -132,7 +136,10 @@ private predicate startsBasicBlock(Instruction instr) { exists(Instruction predecessor, EdgeKind kind | instr = predecessor.getSuccessor(kind) and not kind instanceof GotoEdge - ) // Incoming edge is not a GotoEdge + ) or // Incoming edge is not a GotoEdge + exists(Instruction predecessor | + instr = predecessor.getBackEdgeSuccessor(_) + ) // A back edge enters this instruction ) } @@ -184,6 +191,14 @@ private cached module Cached { ) } + cached predicate backEdgeSuccessor(TIRBlock pred, TIRBlock succ, EdgeKind kind) { + exists(Instruction predLast, Instruction succFirst | + predLast = getInstruction(pred, getInstructionCount(pred) - 1) and + succFirst = predLast.getBackEdgeSuccessor(kind) and + succ = MkIRBlock(succFirst) + ) + } + cached predicate blockSuccessor(TIRBlock pred, TIRBlock succ) { blockSuccessor(pred, succ, _) } diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll index dcad960d8dfd..ed370bc30887 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll @@ -463,6 +463,16 @@ class Instruction extends Construction::TInstruction { result = Construction::getInstructionSuccessor(this, kind) } + /** + * Gets the a _back-edge successor_ of this instruction along the control + * flow edge specified by `kind`. A back edge in the control-flow graph is + * intuitively the edge that goes back around a loop. If all back edges are + * removed from the control-flow graph, it becomes acyclic. + */ + final Instruction getBackEdgeSuccessor(EdgeKind kind) { + result = Construction::getInstructionBackEdgeSuccessor(this, kind) + } + /** * Gets all direct successors of this instruction. */ diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SSAConstruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SSAConstruction.qll index 88914e21c3ab..292f2c520651 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SSAConstruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SSAConstruction.qll @@ -278,6 +278,14 @@ cached private module Cached { ) } + cached Instruction getInstructionBackEdgeSuccessor(Instruction instruction, EdgeKind kind) { + exists(OldInstruction oldInstruction | + oldInstruction = getOldInstruction(instruction) and + result = getNewInstruction(oldInstruction.getBackEdgeSuccessor(kind)) and + not Reachability::isInfeasibleInstructionSuccessor(oldInstruction, kind) + ) + } + cached IRVariable getInstructionVariable(Instruction instruction) { result = getNewIRVariable(getOldInstruction(instruction).(OldIR::VariableInstruction).getVariable()) } From b40accee6ff2079bd053b0c31c165f67448514cf Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Wed, 23 Jan 2019 08:15:26 +0100 Subject: [PATCH 02/12] C++: sanity checks for back edges --- .../aliased_ssa/Instruction.qll | 48 +++++++++++++++++++ .../cpp/ir/implementation/raw/Instruction.qll | 48 +++++++++++++++++++ .../unaliased_ssa/Instruction.qll | 48 +++++++++++++++++++ .../ir/ir/aliased_ssa_sanity.expected | 3 ++ .../library-tests/ir/ir/raw_sanity.expected | 3 ++ .../ir/ir/unaliased_ssa_sanity.expected | 3 ++ 6 files changed, 153 insertions(+) diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll index ed370bc30887..164a3cf4548f 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll @@ -131,6 +131,54 @@ module InstructionSanity { blockCount = count(instr.getBlock()) and blockCount != 1 } + + private predicate forwardEdge(IRBlock b1, IRBlock b2) { + b1.getASuccessor() = b2 and + not b1.getBackEdgeSuccessor(_) = b2 + } + + /** + * Holds if `f` contains a loop in which no edge is a back edge. + * + * This check ensures we don't have too _few_ back edges. + */ + query predicate containsLoopOfForwardEdges(FunctionIR f) { + exists(IRBlock block | + forwardEdge+(block, block) and + block.getFunctionIR() = f + ) + } + + /** + * Holds if `block` is reachable from its function entry point but would not + * be reachable by traversing only forward edges. This check is skipped for + * functions containing `goto` statements as the property does not generally + * hold there. + * + * This check ensures we don't have too _many_ back edges. + */ + query predicate lostReachability(IRBlock block) { + exists(FunctionIR f, IRBlock entry | + entry = f.getEntryBlock() and + entry.getASuccessor+() = block and + not forwardEdge+(entry, block) and + not exists(GotoStmt s | s.getEnclosingFunction() = f.getFunction()) + ) + } + + /** + * Holds if the number of back edges differs between the `Instruction` graph + * and the `IRBlock` graph. + */ + query predicate backEdgeCountMismatch(Function f, int fromInstr, int fromBlock) { + fromInstr = count(Instruction i1, Instruction i2 | + i1.getFunction() = f and i1.getBackEdgeSuccessor(_) = i2 + ) and + fromBlock = count(IRBlock b1, IRBlock b2 | + b1.getFunction() = f and b1.getBackEdgeSuccessor(_) = b2 + ) and + fromInstr != fromBlock + } } /** diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll index ed370bc30887..164a3cf4548f 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll @@ -131,6 +131,54 @@ module InstructionSanity { blockCount = count(instr.getBlock()) and blockCount != 1 } + + private predicate forwardEdge(IRBlock b1, IRBlock b2) { + b1.getASuccessor() = b2 and + not b1.getBackEdgeSuccessor(_) = b2 + } + + /** + * Holds if `f` contains a loop in which no edge is a back edge. + * + * This check ensures we don't have too _few_ back edges. + */ + query predicate containsLoopOfForwardEdges(FunctionIR f) { + exists(IRBlock block | + forwardEdge+(block, block) and + block.getFunctionIR() = f + ) + } + + /** + * Holds if `block` is reachable from its function entry point but would not + * be reachable by traversing only forward edges. This check is skipped for + * functions containing `goto` statements as the property does not generally + * hold there. + * + * This check ensures we don't have too _many_ back edges. + */ + query predicate lostReachability(IRBlock block) { + exists(FunctionIR f, IRBlock entry | + entry = f.getEntryBlock() and + entry.getASuccessor+() = block and + not forwardEdge+(entry, block) and + not exists(GotoStmt s | s.getEnclosingFunction() = f.getFunction()) + ) + } + + /** + * Holds if the number of back edges differs between the `Instruction` graph + * and the `IRBlock` graph. + */ + query predicate backEdgeCountMismatch(Function f, int fromInstr, int fromBlock) { + fromInstr = count(Instruction i1, Instruction i2 | + i1.getFunction() = f and i1.getBackEdgeSuccessor(_) = i2 + ) and + fromBlock = count(IRBlock b1, IRBlock b2 | + b1.getFunction() = f and b1.getBackEdgeSuccessor(_) = b2 + ) and + fromInstr != fromBlock + } } /** diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll index ed370bc30887..164a3cf4548f 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll @@ -131,6 +131,54 @@ module InstructionSanity { blockCount = count(instr.getBlock()) and blockCount != 1 } + + private predicate forwardEdge(IRBlock b1, IRBlock b2) { + b1.getASuccessor() = b2 and + not b1.getBackEdgeSuccessor(_) = b2 + } + + /** + * Holds if `f` contains a loop in which no edge is a back edge. + * + * This check ensures we don't have too _few_ back edges. + */ + query predicate containsLoopOfForwardEdges(FunctionIR f) { + exists(IRBlock block | + forwardEdge+(block, block) and + block.getFunctionIR() = f + ) + } + + /** + * Holds if `block` is reachable from its function entry point but would not + * be reachable by traversing only forward edges. This check is skipped for + * functions containing `goto` statements as the property does not generally + * hold there. + * + * This check ensures we don't have too _many_ back edges. + */ + query predicate lostReachability(IRBlock block) { + exists(FunctionIR f, IRBlock entry | + entry = f.getEntryBlock() and + entry.getASuccessor+() = block and + not forwardEdge+(entry, block) and + not exists(GotoStmt s | s.getEnclosingFunction() = f.getFunction()) + ) + } + + /** + * Holds if the number of back edges differs between the `Instruction` graph + * and the `IRBlock` graph. + */ + query predicate backEdgeCountMismatch(Function f, int fromInstr, int fromBlock) { + fromInstr = count(Instruction i1, Instruction i2 | + i1.getFunction() = f and i1.getBackEdgeSuccessor(_) = i2 + ) and + fromBlock = count(IRBlock b1, IRBlock b2 | + b1.getFunction() = f and b1.getBackEdgeSuccessor(_) = b2 + ) and + fromInstr != fromBlock + } } /** diff --git a/cpp/ql/test/library-tests/ir/ir/aliased_ssa_sanity.expected b/cpp/ql/test/library-tests/ir/ir/aliased_ssa_sanity.expected index 95989fc88090..220255b41e94 100644 --- a/cpp/ql/test/library-tests/ir/ir/aliased_ssa_sanity.expected +++ b/cpp/ql/test/library-tests/ir/ir/aliased_ssa_sanity.expected @@ -6,3 +6,6 @@ instructionWithoutSuccessor unnecessaryPhiInstruction operandAcrossFunctions instructionWithoutUniqueBlock +containsLoopOfForwardEdges +lostReachability +backEdgeCountMismatch diff --git a/cpp/ql/test/library-tests/ir/ir/raw_sanity.expected b/cpp/ql/test/library-tests/ir/ir/raw_sanity.expected index 95989fc88090..220255b41e94 100644 --- a/cpp/ql/test/library-tests/ir/ir/raw_sanity.expected +++ b/cpp/ql/test/library-tests/ir/ir/raw_sanity.expected @@ -6,3 +6,6 @@ instructionWithoutSuccessor unnecessaryPhiInstruction operandAcrossFunctions instructionWithoutUniqueBlock +containsLoopOfForwardEdges +lostReachability +backEdgeCountMismatch diff --git a/cpp/ql/test/library-tests/ir/ir/unaliased_ssa_sanity.expected b/cpp/ql/test/library-tests/ir/ir/unaliased_ssa_sanity.expected index 95989fc88090..220255b41e94 100644 --- a/cpp/ql/test/library-tests/ir/ir/unaliased_ssa_sanity.expected +++ b/cpp/ql/test/library-tests/ir/ir/unaliased_ssa_sanity.expected @@ -6,3 +6,6 @@ instructionWithoutSuccessor unnecessaryPhiInstruction operandAcrossFunctions instructionWithoutUniqueBlock +containsLoopOfForwardEdges +lostReachability +backEdgeCountMismatch From bb7369e8443760c744aa08fdfdd74f0222348531 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Tue, 22 Jan 2019 19:16:58 +0100 Subject: [PATCH 03/12] C++: Use new back-edge def. in range analysis By using this new definition of back edges, the range analysis should work on code that uses unstructured `goto`s. --- cpp/ql/src/semmle/code/cpp/rangeanalysis/RangeUtils.qll | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/rangeanalysis/RangeUtils.qll b/cpp/ql/src/semmle/code/cpp/rangeanalysis/RangeUtils.qll index b02914033a74..f791bcf6277e 100644 --- a/cpp/ql/src/semmle/code/cpp/rangeanalysis/RangeUtils.qll +++ b/cpp/ql/src/semmle/code/cpp/rangeanalysis/RangeUtils.qll @@ -81,6 +81,5 @@ predicate isReducibleCFG(Function 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 + phi.getBlock() = op.getPredecessorBlock().getBackEdgeSuccessor(_) +} From 6d09a9b3242405b596ac252d34220307332e2b47 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Fri, 25 Jan 2019 09:31:07 +0100 Subject: [PATCH 04/12] C++: Enable range analysis for irreducible CFGs This adds one new test result (`i >= 0` on line 130). --- .../code/cpp/rangeanalysis/RangeAnalysis.qll | 81 +++++++++---------- .../code/cpp/rangeanalysis/RangeUtils.qll | 16 ---- .../rangeanalysis/RangeAnalysis.expected | 1 + 3 files changed, 40 insertions(+), 58 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/rangeanalysis/RangeAnalysis.qll b/cpp/ql/src/semmle/code/cpp/rangeanalysis/RangeAnalysis.qll index 32b3d4ee4fcd..7811c86730fb 100644 --- a/cpp/ql/src/semmle/code/cpp/rangeanalysis/RangeAnalysis.qll +++ b/cpp/ql/src/semmle/code/cpp/rangeanalysis/RangeAnalysis.qll @@ -559,47 +559,44 @@ private predicate boundedCastExpr( private predicate boundedInstruction( Instruction i, Bound b, int delta, boolean upper, boolean fromBackEdge, int origdelta, Reason 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) - ) + 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) ) } diff --git a/cpp/ql/src/semmle/code/cpp/rangeanalysis/RangeUtils.qll b/cpp/ql/src/semmle/code/cpp/rangeanalysis/RangeUtils.qll index f791bcf6277e..04a3d2499e43 100644 --- a/cpp/ql/src/semmle/code/cpp/rangeanalysis/RangeUtils.qll +++ b/cpp/ql/src/semmle/code/cpp/rangeanalysis/RangeUtils.qll @@ -63,22 +63,6 @@ predicate valueFlowStep(Instruction i, Operand op, int delta) { ) } -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() = op.getPredecessorBlock().getBackEdgeSuccessor(_) diff --git a/cpp/ql/test/library-tests/rangeanalysis/rangeanalysis/RangeAnalysis.expected b/cpp/ql/test/library-tests/rangeanalysis/rangeanalysis/RangeAnalysis.expected index d48b37bbaf19..7161fdb86892 100644 --- a/cpp/ql/test/library-tests/rangeanalysis/rangeanalysis/RangeAnalysis.expected +++ b/cpp/ql/test/library-tests/rangeanalysis/rangeanalysis/RangeAnalysis.expected @@ -36,6 +36,7 @@ | 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: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:130:10:130:10 | Load: i | file://:0:0:0:0 | 0 | 0 | false | NoReason | file://:0:0:0:0 | file://:0:0:0:0 | | 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 | From 34659422dbfa949fd9565c9ac0bf24831ae3b82c Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Fri, 25 Jan 2019 09:59:28 +0100 Subject: [PATCH 05/12] C++: Simplify isStrictlyForwardGoto We had an existing `Location.isBefore` predicate that was just right for this use case. Performance is great thanks to magic. --- .../ir/implementation/raw/internal/IRConstruction.qll | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/IRConstruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/IRConstruction.qll index 089ec10e7637..435f0896c55e 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/IRConstruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/IRConstruction.qll @@ -180,15 +180,7 @@ cached private module Cached { /** Holds if `goto` jumps strictly forward in the program text. */ private predicate isStrictlyForwardGoto(GotoStmt goto) { - exists(string gotoFile, int gotoLine, string targetFile, int targetLine | - goto.getLocation().hasLocationInfo(gotoFile, gotoLine, _, _, _) and - goto.getTarget().getLocation().hasLocationInfo(targetFile, targetLine, _, _, _) and - targetLine > gotoLine and - // We avoid `=` for equality here since that might tempt the join orderer - // into joining on the file name too early. - gotoFile >= targetFile and - gotoFile <= targetFile - ) + goto.getLocation().isBefore(goto.getTarget().getLocation()) } cached IRVariable getInstructionVariable(Instruction instruction) { From 5b2b961a4469cdfc3e805da93c56c2d5b538bee2 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Fri, 25 Jan 2019 11:28:23 +0100 Subject: [PATCH 06/12] C++: Fix comment (edge is not unique) --- .../code/cpp/ir/implementation/raw/internal/IRConstruction.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/IRConstruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/IRConstruction.qll index 435f0896c55e..1240ce757964 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/IRConstruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/IRConstruction.qll @@ -125,7 +125,7 @@ cached private module Cached { ) ) or - // The back edge should be the (unique) edge from the condition to the + // The back edge should be the edge(s) from the condition to the // body. This ensures that it's the back edge that will be pruned in a `do // { ... } while (0)` statement. Note that all `continue` statements in a // do-while loop produce forward edges. From 62509ffb69ab3d85df4333a6cf879038cd472041 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Fri, 25 Jan 2019 12:12:31 +0100 Subject: [PATCH 07/12] C++: Add a back-edge safeguard This prevents loops of non-back-edges on ChakraCore (see #811). --- .../ir/implementation/aliased_ssa/IRBlock.qll | 31 +++++++++++++++++-- .../aliased_ssa/Instruction.qll | 10 +++++- .../cpp/ir/implementation/raw/IRBlock.qll | 31 +++++++++++++++++-- .../cpp/ir/implementation/raw/Instruction.qll | 10 +++++- .../implementation/unaliased_ssa/IRBlock.qll | 31 +++++++++++++++++-- .../unaliased_ssa/Instruction.qll | 10 +++++- 6 files changed, 114 insertions(+), 9 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/IRBlock.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/IRBlock.qll index c1354724a7f1..f1fd13392051 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/IRBlock.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/IRBlock.qll @@ -138,7 +138,7 @@ private predicate startsBasicBlock(Instruction instr) { not kind instanceof GotoEdge ) or // Incoming edge is not a GotoEdge exists(Instruction predecessor | - instr = predecessor.getBackEdgeSuccessor(_) + instr = Construction::getInstructionBackEdgeSuccessor(predecessor, _) ) // A back edge enters this instruction ) } @@ -192,9 +192,36 @@ private cached module Cached { } cached predicate backEdgeSuccessor(TIRBlock pred, TIRBlock succ, EdgeKind kind) { + backEdgeSuccessorRaw(pred, succ, kind) + or + forwardEdgeRaw+(pred, pred) and + blockSuccessor(pred, succ, kind) + } + + /** + * Holds if there is an edge from `pred` to `succ` that is not a back edge. + */ + private predicate forwardEdgeRaw(TIRBlock pred, TIRBlock succ) { + exists(EdgeKind kind | + blockSuccessor(pred, succ, kind) and + not backEdgeSuccessorRaw(pred, succ, kind) + ) + } + + /** + * Holds if the `kind`-edge from `pred` to `succ` is a back edge according to + * `Construction`. + * + * There could be loops of non-back-edges if there is a flaw in the IR + * construction or back-edge detection, and this could cause non-termination + * of subsequent analysis. To prevent that, a subsequent predicate further + * classifies all edges as back edges if they are involved in a loop of + * non-back-edges. + */ + private predicate backEdgeSuccessorRaw(TIRBlock pred, TIRBlock succ, EdgeKind kind) { exists(Instruction predLast, Instruction succFirst | predLast = getInstruction(pred, getInstructionCount(pred) - 1) and - succFirst = predLast.getBackEdgeSuccessor(kind) and + succFirst = Construction::getInstructionBackEdgeSuccessor(predLast, kind) and succ = MkIRBlock(succFirst) ) } diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll index 164a3cf4548f..1fe57bd53232 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll @@ -518,7 +518,15 @@ class Instruction extends Construction::TInstruction { * removed from the control-flow graph, it becomes acyclic. */ final Instruction getBackEdgeSuccessor(EdgeKind kind) { - result = Construction::getInstructionBackEdgeSuccessor(this, kind) + // We don't take these edges from + // `Construction::getInstructionBackEdgeSuccessor` since that relation has + // not been treated to remove any loops that might be left over due to + // flaws in the IR construction or back-edge detection. + exists(IRBlock block | + block = this.getBlock() and + this = block.getLastInstruction() and + result = block.getBackEdgeSuccessor(kind).getFirstInstruction() + ) } /** diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/IRBlock.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/IRBlock.qll index c1354724a7f1..f1fd13392051 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/IRBlock.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/IRBlock.qll @@ -138,7 +138,7 @@ private predicate startsBasicBlock(Instruction instr) { not kind instanceof GotoEdge ) or // Incoming edge is not a GotoEdge exists(Instruction predecessor | - instr = predecessor.getBackEdgeSuccessor(_) + instr = Construction::getInstructionBackEdgeSuccessor(predecessor, _) ) // A back edge enters this instruction ) } @@ -192,9 +192,36 @@ private cached module Cached { } cached predicate backEdgeSuccessor(TIRBlock pred, TIRBlock succ, EdgeKind kind) { + backEdgeSuccessorRaw(pred, succ, kind) + or + forwardEdgeRaw+(pred, pred) and + blockSuccessor(pred, succ, kind) + } + + /** + * Holds if there is an edge from `pred` to `succ` that is not a back edge. + */ + private predicate forwardEdgeRaw(TIRBlock pred, TIRBlock succ) { + exists(EdgeKind kind | + blockSuccessor(pred, succ, kind) and + not backEdgeSuccessorRaw(pred, succ, kind) + ) + } + + /** + * Holds if the `kind`-edge from `pred` to `succ` is a back edge according to + * `Construction`. + * + * There could be loops of non-back-edges if there is a flaw in the IR + * construction or back-edge detection, and this could cause non-termination + * of subsequent analysis. To prevent that, a subsequent predicate further + * classifies all edges as back edges if they are involved in a loop of + * non-back-edges. + */ + private predicate backEdgeSuccessorRaw(TIRBlock pred, TIRBlock succ, EdgeKind kind) { exists(Instruction predLast, Instruction succFirst | predLast = getInstruction(pred, getInstructionCount(pred) - 1) and - succFirst = predLast.getBackEdgeSuccessor(kind) and + succFirst = Construction::getInstructionBackEdgeSuccessor(predLast, kind) and succ = MkIRBlock(succFirst) ) } diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll index 164a3cf4548f..1fe57bd53232 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Instruction.qll @@ -518,7 +518,15 @@ class Instruction extends Construction::TInstruction { * removed from the control-flow graph, it becomes acyclic. */ final Instruction getBackEdgeSuccessor(EdgeKind kind) { - result = Construction::getInstructionBackEdgeSuccessor(this, kind) + // We don't take these edges from + // `Construction::getInstructionBackEdgeSuccessor` since that relation has + // not been treated to remove any loops that might be left over due to + // flaws in the IR construction or back-edge detection. + exists(IRBlock block | + block = this.getBlock() and + this = block.getLastInstruction() and + result = block.getBackEdgeSuccessor(kind).getFirstInstruction() + ) } /** diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/IRBlock.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/IRBlock.qll index c1354724a7f1..f1fd13392051 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/IRBlock.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/IRBlock.qll @@ -138,7 +138,7 @@ private predicate startsBasicBlock(Instruction instr) { not kind instanceof GotoEdge ) or // Incoming edge is not a GotoEdge exists(Instruction predecessor | - instr = predecessor.getBackEdgeSuccessor(_) + instr = Construction::getInstructionBackEdgeSuccessor(predecessor, _) ) // A back edge enters this instruction ) } @@ -192,9 +192,36 @@ private cached module Cached { } cached predicate backEdgeSuccessor(TIRBlock pred, TIRBlock succ, EdgeKind kind) { + backEdgeSuccessorRaw(pred, succ, kind) + or + forwardEdgeRaw+(pred, pred) and + blockSuccessor(pred, succ, kind) + } + + /** + * Holds if there is an edge from `pred` to `succ` that is not a back edge. + */ + private predicate forwardEdgeRaw(TIRBlock pred, TIRBlock succ) { + exists(EdgeKind kind | + blockSuccessor(pred, succ, kind) and + not backEdgeSuccessorRaw(pred, succ, kind) + ) + } + + /** + * Holds if the `kind`-edge from `pred` to `succ` is a back edge according to + * `Construction`. + * + * There could be loops of non-back-edges if there is a flaw in the IR + * construction or back-edge detection, and this could cause non-termination + * of subsequent analysis. To prevent that, a subsequent predicate further + * classifies all edges as back edges if they are involved in a loop of + * non-back-edges. + */ + private predicate backEdgeSuccessorRaw(TIRBlock pred, TIRBlock succ, EdgeKind kind) { exists(Instruction predLast, Instruction succFirst | predLast = getInstruction(pred, getInstructionCount(pred) - 1) and - succFirst = predLast.getBackEdgeSuccessor(kind) and + succFirst = Construction::getInstructionBackEdgeSuccessor(predLast, kind) and succ = MkIRBlock(succFirst) ) } diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll index 164a3cf4548f..1fe57bd53232 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll @@ -518,7 +518,15 @@ class Instruction extends Construction::TInstruction { * removed from the control-flow graph, it becomes acyclic. */ final Instruction getBackEdgeSuccessor(EdgeKind kind) { - result = Construction::getInstructionBackEdgeSuccessor(this, kind) + // We don't take these edges from + // `Construction::getInstructionBackEdgeSuccessor` since that relation has + // not been treated to remove any loops that might be left over due to + // flaws in the IR construction or back-edge detection. + exists(IRBlock block | + block = this.getBlock() and + this = block.getLastInstruction() and + result = block.getBackEdgeSuccessor(kind).getFirstInstruction() + ) } /** From 9963270d637b6733b13aacf81e74351ad5abb080 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Fri, 25 Jan 2019 14:16:45 +0100 Subject: [PATCH 08/12] C++: Annotate back edges in IR debug output --- .../ir/implementation/aliased_ssa/PrintIR.qll | 4 +- .../cpp/ir/implementation/raw/PrintIR.qll | 4 +- .../raw/internal/IRConstruction.qll | 12 ++++-- .../implementation/unaliased_ssa/PrintIR.qll | 4 +- .../ir/ir/aliased_ssa_ir.expected | 36 +++++++++--------- .../test/library-tests/ir/ir/raw_ir.expected | 38 +++++++++---------- .../ir/ir/unaliased_ssa_ir.expected | 36 +++++++++--------- 7 files changed, 72 insertions(+), 62 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/PrintIR.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/PrintIR.qll index 8fb776214262..748faeee590c 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/PrintIR.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/PrintIR.qll @@ -287,7 +287,9 @@ query predicate edges(PrintableIRBlock pred, PrintableIRBlock succ, string key, ( ( key = "semmle.label" and - value = kind.toString() + if predBlock.getBackEdgeSuccessor(kind) = succBlock + then value = kind.toString() + " (back edge)" + else value = kind.toString() ) or ( key = "semmle.order" and diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/PrintIR.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/PrintIR.qll index 8fb776214262..748faeee590c 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/PrintIR.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/PrintIR.qll @@ -287,7 +287,9 @@ query predicate edges(PrintableIRBlock pred, PrintableIRBlock succ, string key, ( ( key = "semmle.label" and - value = kind.toString() + if predBlock.getBackEdgeSuccessor(kind) = succBlock + then value = kind.toString() + " (back edge)" + else value = kind.toString() ) or ( key = "semmle.order" and diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/IRConstruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/IRConstruction.qll index 1240ce757964..d328ebdad06d 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/IRConstruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/IRConstruction.qll @@ -112,6 +112,7 @@ cached private module Cached { // - Restrict to edges that originate within the loop (/ 2). pragma[noopt] cached Instruction getInstructionBackEdgeSuccessor(Instruction instruction, EdgeKind kind) { + // While loop: // Any edge from within the body of the loop to the condition of the loop // is a back edge. This includes edges from `continue` and the fall-through // edge(s) after the last instruction(s) in the body. @@ -125,6 +126,7 @@ cached private module Cached { ) ) or + // Do-while loop: // The back edge should be the edge(s) from the condition to the // body. This ensures that it's the back edge that will be pruned in a `do // { ... } while (0)` statement. Note that all `continue` statements in a @@ -141,11 +143,12 @@ cached private module Cached { ) ) or + // For loop: // Any edge from within the body or update of the loop to the condition of - // the loop is a back edge. This includes edges from `continue` and the - // fall-through edge(s) after the last instruction(s) in the body. A `for` - // loop may not have a condition, in which case - // `getFirstConditionInstruction` returns the body instead. + // the loop is a back edge. When there is no loop update expression, this + // includes edges from `continue` and the fall-through edge(s) after the + // last instruction(s) in the body. A for loop may not have a condition, in + // which case `getFirstConditionInstruction` returns the body instead. exists(TranslatedForStmt s | s instanceof TranslatedForStmt and result = s.getFirstConditionInstruction() and @@ -162,6 +165,7 @@ cached private module Cached { ) ) or + // Goto statement: // As a conservative approximation, any edge out of `goto` is a back edge // unless it goes strictly forward in the program text. A `goto` whose // source and target are both inside a macro will be seen as having the diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/PrintIR.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/PrintIR.qll index 8fb776214262..748faeee590c 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/PrintIR.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/PrintIR.qll @@ -287,7 +287,9 @@ query predicate edges(PrintableIRBlock pred, PrintableIRBlock succ, string key, ( ( key = "semmle.label" and - value = kind.toString() + if predBlock.getBackEdgeSuccessor(kind) = succBlock + then value = kind.toString() + " (back edge)" + else value = kind.toString() ) or ( key = "semmle.order" and diff --git a/cpp/ql/test/library-tests/ir/ir/aliased_ssa_ir.expected b/cpp/ql/test/library-tests/ir/ir/aliased_ssa_ir.expected index 74b2b67f6cd2..01e6f3b78f03 100644 --- a/cpp/ql/test/library-tests/ir/ir/aliased_ssa_ir.expected +++ b/cpp/ql/test/library-tests/ir/ir/aliased_ssa_ir.expected @@ -1116,7 +1116,7 @@ ir.cpp: # 255| r1_2(int) = Load : r1_1, m3_0 # 255| r1_3(int) = Sub : r1_2, r1_0 # 255| m1_4(int) = Store : r1_1, r1_3 -#-----| Goto -> Block 3 +#-----| Goto (back edge) -> Block 3 # 257| Block 2 # 257| v2_0(void) = NoOp : @@ -1156,7 +1156,7 @@ ir.cpp: # 262| r1_9(bool) = CompareGT : r1_7, r1_8 # 262| v1_10(void) = ConditionalBranch : r1_9 #-----| False -> Block 2 -#-----| True -> Block 1 +#-----| True (back edge) -> Block 1 # 263| Block 2 # 263| v2_0(void) = NoOp : @@ -1175,7 +1175,7 @@ ir.cpp: # 268| Block 1 # 268| v1_0(void) = NoOp : -#-----| Goto -> Block 1 +#-----| Goto (back edge) -> Block 1 # 272| For_Init() -> void # 272| Block 0 @@ -1189,7 +1189,7 @@ ir.cpp: # 274| Block 1 # 274| v1_0(void) = NoOp : -#-----| Goto -> Block 1 +#-----| Goto (back edge) -> Block 1 # 278| For_Condition() -> void # 278| Block 0 @@ -1212,7 +1212,7 @@ ir.cpp: # 281| Block 2 # 281| v2_0(void) = NoOp : -#-----| Goto -> Block 1 +#-----| Goto (back edge) -> Block 1 # 278| Block 3 # 278| v3_0(void) = Unreached : @@ -1235,7 +1235,7 @@ ir.cpp: # 287| r1_4(int) = Load : r1_3, m1_0 # 287| r1_5(int) = Add : r1_4, r1_2 # 287| m1_6(int) = Store : r1_3, r1_5 -#-----| Goto -> Block 1 +#-----| Goto (back edge) -> Block 1 # 292| For_InitCondition() -> void # 292| Block 0 @@ -1258,7 +1258,7 @@ ir.cpp: # 294| Block 2 # 294| v2_0(void) = NoOp : -#-----| Goto -> Block 1 +#-----| Goto (back edge) -> Block 1 # 292| Block 3 # 292| v3_0(void) = Unreached : @@ -1281,7 +1281,7 @@ ir.cpp: # 299| r1_4(int) = Load : r1_3, m1_0 # 299| r1_5(int) = Add : r1_4, r1_2 # 299| m1_6(int) = Store : r1_3, r1_5 -#-----| Goto -> Block 1 +#-----| Goto (back edge) -> Block 1 # 304| For_ConditionUpdate() -> void # 304| Block 0 @@ -1310,7 +1310,7 @@ ir.cpp: # 306| r2_3(int) = Load : r2_2, m1_0 # 306| r2_4(int) = Add : r2_3, r2_1 # 306| m2_5(int) = Store : r2_2, r2_4 -#-----| Goto -> Block 1 +#-----| Goto (back edge) -> Block 1 # 309| Block 3 # 309| v3_0(void) = NoOp : @@ -1345,7 +1345,7 @@ ir.cpp: # 312| r2_3(int) = Load : r2_2, m1_0 # 312| r2_4(int) = Add : r2_3, r2_1 # 312| m2_5(int) = Store : r2_2, r2_4 -#-----| Goto -> Block 1 +#-----| Goto (back edge) -> Block 1 # 315| Block 3 # 315| v3_0(void) = NoOp : @@ -1379,7 +1379,7 @@ ir.cpp: # 318| r2_2(int) = Load : r2_1, m1_0 # 318| r2_3(int) = Add : r2_2, r2_0 # 318| m2_4(int) = Store : r2_1, r2_3 -#-----| Goto -> Block 1 +#-----| Goto (back edge) -> Block 1 # 319| Block 3 # 319| r3_0(glval) = VariableAddress[i] : @@ -1441,7 +1441,7 @@ ir.cpp: # 326| r4_3(int) = Load : r4_2, m1_0 # 326| r4_4(int) = Add : r4_3, r4_1 # 326| m4_5(int) = Store : r4_2, r4_4 -#-----| Goto -> Block 1 +#-----| Goto (back edge) -> Block 1 # 331| Block 5 # 331| v5_0(void) = NoOp : @@ -1479,7 +1479,7 @@ ir.cpp: # 334| Block 3 # 334| v3_0(void) = NoOp : -#-----| Goto -> Block 1 +#-----| Goto (back edge) -> Block 1 # 333| Block 4 # 333| v4_0(void) = Unreached : @@ -1547,7 +1547,7 @@ ir.cpp: # 356| r3_2(int) = Load : r3_1, m5_0 # 356| r3_3(int) = Sub : r3_2, r3_0 # 356| m3_4(int) = Store : r3_1, r3_3 -#-----| Goto -> Block 5 +#-----| Goto (back edge) -> Block 5 # 357| Block 4 # 357| v4_0(void) = NoOp : @@ -1606,7 +1606,7 @@ ir.cpp: # 366| r4_5(bool) = CompareGT : r4_3, r4_4 # 366| v4_6(void) = ConditionalBranch : r4_5 #-----| False -> Block 5 -#-----| True -> Block 1 +#-----| True (back edge) -> Block 1 # 367| Block 5 # 367| v5_0(void) = NoOp : @@ -4395,7 +4395,7 @@ ir.cpp: # 979| Block 1 # 979| v1_0(void) = NoOp : -#-----| Goto -> Block 7 +#-----| Goto (back edge) -> Block 7 # 981| Block 2 # 981| r2_0(glval) = VariableAddress[z] : @@ -4415,7 +4415,7 @@ ir.cpp: # 981| Block 3 # 981| v3_0(void) = NoOp : -#-----| Goto -> Block 2 +#-----| Goto (back edge) -> Block 2 # 983| Block 4 # 983| r4_0(glval) = VariableAddress[p] : @@ -4431,7 +4431,7 @@ ir.cpp: # 983| Block 5 # 983| v5_0(void) = NoOp : -#-----| Goto -> Block 4 +#-----| Goto (back edge) -> Block 4 # 985| Block 6 # 985| v6_0(void) = NoOp : diff --git a/cpp/ql/test/library-tests/ir/ir/raw_ir.expected b/cpp/ql/test/library-tests/ir/ir/raw_ir.expected index b7e4d6928a87..6b9ace79ac00 100644 --- a/cpp/ql/test/library-tests/ir/ir/raw_ir.expected +++ b/cpp/ql/test/library-tests/ir/ir/raw_ir.expected @@ -1108,7 +1108,7 @@ ir.cpp: # 255| r1_2(int) = Load : r1_1, mu0_2 # 255| r1_3(int) = Sub : r1_2, r1_0 # 255| mu1_4(int) = Store : r1_1, r1_3 -#-----| Goto -> Block 3 +#-----| Goto (back edge) -> Block 3 # 257| Block 2 # 257| v2_0(void) = NoOp : @@ -1146,7 +1146,7 @@ ir.cpp: # 262| r1_8(bool) = CompareGT : r1_6, r1_7 # 262| v1_9(void) = ConditionalBranch : r1_8 #-----| False -> Block 2 -#-----| True -> Block 1 +#-----| True (back edge) -> Block 1 # 263| Block 2 # 263| v2_0(void) = NoOp : @@ -1170,7 +1170,7 @@ ir.cpp: # 268| Block 2 # 268| v2_0(void) = NoOp : -#-----| Goto -> Block 2 +#-----| Goto (back edge) -> Block 2 # 272| For_Init() -> void # 272| Block 0 @@ -1189,7 +1189,7 @@ ir.cpp: # 274| Block 2 # 274| v2_0(void) = NoOp : -#-----| Goto -> Block 2 +#-----| Goto (back edge) -> Block 2 # 278| For_Condition() -> void # 278| Block 0 @@ -1212,7 +1212,7 @@ ir.cpp: # 281| Block 2 # 281| v2_0(void) = NoOp : -#-----| Goto -> Block 1 +#-----| Goto (back edge) -> Block 1 # 283| Block 3 # 283| v3_0(void) = NoOp : @@ -1242,7 +1242,7 @@ ir.cpp: # 287| r2_3(int) = Load : r2_2, mu0_2 # 287| r2_4(int) = Add : r2_3, r2_1 # 287| mu2_5(int) = Store : r2_2, r2_4 -#-----| Goto -> Block 2 +#-----| Goto (back edge) -> Block 2 # 292| For_InitCondition() -> void # 292| Block 0 @@ -1265,7 +1265,7 @@ ir.cpp: # 294| Block 2 # 294| v2_0(void) = NoOp : -#-----| Goto -> Block 1 +#-----| Goto (back edge) -> Block 1 # 296| Block 3 # 296| v3_0(void) = NoOp : @@ -1295,7 +1295,7 @@ ir.cpp: # 299| r2_3(int) = Load : r2_2, mu0_2 # 299| r2_4(int) = Add : r2_3, r2_1 # 299| mu2_5(int) = Store : r2_2, r2_4 -#-----| Goto -> Block 2 +#-----| Goto (back edge) -> Block 2 # 304| For_ConditionUpdate() -> void # 304| Block 0 @@ -1323,7 +1323,7 @@ ir.cpp: # 306| r2_3(int) = Load : r2_2, mu0_2 # 306| r2_4(int) = Add : r2_3, r2_1 # 306| mu2_5(int) = Store : r2_2, r2_4 -#-----| Goto -> Block 1 +#-----| Goto (back edge) -> Block 1 # 309| Block 3 # 309| v3_0(void) = NoOp : @@ -1357,7 +1357,7 @@ ir.cpp: # 312| r2_3(int) = Load : r2_2, mu0_2 # 312| r2_4(int) = Add : r2_3, r2_1 # 312| mu2_5(int) = Store : r2_2, r2_4 -#-----| Goto -> Block 1 +#-----| Goto (back edge) -> Block 1 # 315| Block 3 # 315| v3_0(void) = NoOp : @@ -1390,7 +1390,7 @@ ir.cpp: # 318| r2_2(int) = Load : r2_1, mu0_2 # 318| r2_3(int) = Add : r2_2, r2_0 # 318| mu2_4(int) = Store : r2_1, r2_3 -#-----| Goto -> Block 1 +#-----| Goto (back edge) -> Block 1 # 319| Block 3 # 319| r3_0(glval) = VariableAddress[i] : @@ -1451,7 +1451,7 @@ ir.cpp: # 326| r4_3(int) = Load : r4_2, mu0_2 # 326| r4_4(int) = Add : r4_3, r4_1 # 326| mu4_5(int) = Store : r4_2, r4_4 -#-----| Goto -> Block 1 +#-----| Goto (back edge) -> Block 1 # 331| Block 5 # 331| v5_0(void) = NoOp : @@ -1493,7 +1493,7 @@ ir.cpp: # 334| Block 4 # 334| v4_0(void) = NoOp : -#-----| Goto -> Block 1 +#-----| Goto (back edge) -> Block 1 # 339| Block 5 # 339| v5_0(void) = NoOp : @@ -1563,7 +1563,7 @@ ir.cpp: # 356| r3_2(int) = Load : r3_1, mu0_2 # 356| r3_3(int) = Sub : r3_2, r3_0 # 356| mu3_4(int) = Store : r3_1, r3_3 -#-----| Goto -> Block 5 +#-----| Goto (back edge) -> Block 5 # 357| Block 4 # 357| v4_0(void) = NoOp : @@ -1619,7 +1619,7 @@ ir.cpp: # 366| r4_4(bool) = CompareGT : r4_2, r4_3 # 366| v4_5(void) = ConditionalBranch : r4_4 #-----| False -> Block 5 -#-----| True -> Block 1 +#-----| True (back edge) -> Block 1 # 367| Block 5 # 367| v5_0(void) = NoOp : @@ -4282,7 +4282,7 @@ ir.cpp: # 979| Block 1 # 979| v1_0(void) = NoOp : -#-----| Goto -> Block 7 +#-----| Goto (back edge) -> Block 7 # 981| Block 2 # 981| r2_0(glval) = VariableAddress[z] : @@ -4302,7 +4302,7 @@ ir.cpp: # 981| Block 3 # 981| v3_0(void) = NoOp : -#-----| Goto -> Block 2 +#-----| Goto (back edge) -> Block 2 # 983| Block 4 # 983| r4_0(glval) = VariableAddress[p] : @@ -4318,7 +4318,7 @@ ir.cpp: # 983| Block 5 # 983| v5_0(void) = NoOp : -#-----| Goto -> Block 4 +#-----| Goto (back edge) -> Block 4 # 985| Block 6 # 985| v6_0(void) = NoOp : @@ -4533,7 +4533,7 @@ ir.cpp: # 1053| r1_5(bool) = Constant[0] : # 1053| v1_6(void) = ConditionalBranch : r1_5 #-----| False -> Block 2 -#-----| True -> Block 1 +#-----| True (back edge) -> Block 1 # 1055| Block 2 # 1055| r2_0(glval) = VariableAddress[#return] : diff --git a/cpp/ql/test/library-tests/ir/ir/unaliased_ssa_ir.expected b/cpp/ql/test/library-tests/ir/ir/unaliased_ssa_ir.expected index ff5b79febb37..1b7da6e128c1 100644 --- a/cpp/ql/test/library-tests/ir/ir/unaliased_ssa_ir.expected +++ b/cpp/ql/test/library-tests/ir/ir/unaliased_ssa_ir.expected @@ -1109,7 +1109,7 @@ ir.cpp: # 255| r1_2(int) = Load : r1_1, m3_0 # 255| r1_3(int) = Sub : r1_2, r1_0 # 255| m1_4(int) = Store : r1_1, r1_3 -#-----| Goto -> Block 3 +#-----| Goto (back edge) -> Block 3 # 257| Block 2 # 257| v2_0(void) = NoOp : @@ -1149,7 +1149,7 @@ ir.cpp: # 262| r1_9(bool) = CompareGT : r1_7, r1_8 # 262| v1_10(void) = ConditionalBranch : r1_9 #-----| False -> Block 2 -#-----| True -> Block 1 +#-----| True (back edge) -> Block 1 # 263| Block 2 # 263| v2_0(void) = NoOp : @@ -1168,7 +1168,7 @@ ir.cpp: # 268| Block 1 # 268| v1_0(void) = NoOp : -#-----| Goto -> Block 1 +#-----| Goto (back edge) -> Block 1 # 272| For_Init() -> void # 272| Block 0 @@ -1182,7 +1182,7 @@ ir.cpp: # 274| Block 1 # 274| v1_0(void) = NoOp : -#-----| Goto -> Block 1 +#-----| Goto (back edge) -> Block 1 # 278| For_Condition() -> void # 278| Block 0 @@ -1205,7 +1205,7 @@ ir.cpp: # 281| Block 2 # 281| v2_0(void) = NoOp : -#-----| Goto -> Block 1 +#-----| Goto (back edge) -> Block 1 # 283| Block 3 # 283| v3_0(void) = NoOp : @@ -1231,7 +1231,7 @@ ir.cpp: # 287| r1_4(int) = Load : r1_3, m1_0 # 287| r1_5(int) = Add : r1_4, r1_2 # 287| m1_6(int) = Store : r1_3, r1_5 -#-----| Goto -> Block 1 +#-----| Goto (back edge) -> Block 1 # 292| For_InitCondition() -> void # 292| Block 0 @@ -1254,7 +1254,7 @@ ir.cpp: # 294| Block 2 # 294| v2_0(void) = NoOp : -#-----| Goto -> Block 1 +#-----| Goto (back edge) -> Block 1 # 296| Block 3 # 296| v3_0(void) = NoOp : @@ -1280,7 +1280,7 @@ ir.cpp: # 299| r1_4(int) = Load : r1_3, m1_0 # 299| r1_5(int) = Add : r1_4, r1_2 # 299| m1_6(int) = Store : r1_3, r1_5 -#-----| Goto -> Block 1 +#-----| Goto (back edge) -> Block 1 # 304| For_ConditionUpdate() -> void # 304| Block 0 @@ -1309,7 +1309,7 @@ ir.cpp: # 306| r2_3(int) = Load : r2_2, m1_0 # 306| r2_4(int) = Add : r2_3, r2_1 # 306| m2_5(int) = Store : r2_2, r2_4 -#-----| Goto -> Block 1 +#-----| Goto (back edge) -> Block 1 # 309| Block 3 # 309| v3_0(void) = NoOp : @@ -1344,7 +1344,7 @@ ir.cpp: # 312| r2_3(int) = Load : r2_2, m1_0 # 312| r2_4(int) = Add : r2_3, r2_1 # 312| m2_5(int) = Store : r2_2, r2_4 -#-----| Goto -> Block 1 +#-----| Goto (back edge) -> Block 1 # 315| Block 3 # 315| v3_0(void) = NoOp : @@ -1378,7 +1378,7 @@ ir.cpp: # 318| r2_2(int) = Load : r2_1, m1_0 # 318| r2_3(int) = Add : r2_2, r2_0 # 318| m2_4(int) = Store : r2_1, r2_3 -#-----| Goto -> Block 1 +#-----| Goto (back edge) -> Block 1 # 319| Block 3 # 319| r3_0(glval) = VariableAddress[i] : @@ -1440,7 +1440,7 @@ ir.cpp: # 326| r4_3(int) = Load : r4_2, m1_0 # 326| r4_4(int) = Add : r4_3, r4_1 # 326| m4_5(int) = Store : r4_2, r4_4 -#-----| Goto -> Block 1 +#-----| Goto (back edge) -> Block 1 # 331| Block 5 # 331| v5_0(void) = NoOp : @@ -1482,7 +1482,7 @@ ir.cpp: # 334| Block 4 # 334| v4_0(void) = NoOp : -#-----| Goto -> Block 1 +#-----| Goto (back edge) -> Block 1 # 339| Block 5 # 339| v5_0(void) = NoOp : @@ -1552,7 +1552,7 @@ ir.cpp: # 356| r3_2(int) = Load : r3_1, m5_0 # 356| r3_3(int) = Sub : r3_2, r3_0 # 356| m3_4(int) = Store : r3_1, r3_3 -#-----| Goto -> Block 5 +#-----| Goto (back edge) -> Block 5 # 357| Block 4 # 357| v4_0(void) = NoOp : @@ -1611,7 +1611,7 @@ ir.cpp: # 366| r4_5(bool) = CompareGT : r4_3, r4_4 # 366| v4_6(void) = ConditionalBranch : r4_5 #-----| False -> Block 5 -#-----| True -> Block 1 +#-----| True (back edge) -> Block 1 # 367| Block 5 # 367| v5_0(void) = NoOp : @@ -4263,7 +4263,7 @@ ir.cpp: # 979| Block 1 # 979| v1_0(void) = NoOp : -#-----| Goto -> Block 7 +#-----| Goto (back edge) -> Block 7 # 981| Block 2 # 981| r2_0(glval) = VariableAddress[z] : @@ -4283,7 +4283,7 @@ ir.cpp: # 981| Block 3 # 981| v3_0(void) = NoOp : -#-----| Goto -> Block 2 +#-----| Goto (back edge) -> Block 2 # 983| Block 4 # 983| r4_0(glval) = VariableAddress[p] : @@ -4299,7 +4299,7 @@ ir.cpp: # 983| Block 5 # 983| v5_0(void) = NoOp : -#-----| Goto -> Block 4 +#-----| Goto (back edge) -> Block 4 # 985| Block 6 # 985| v6_0(void) = NoOp : From 560dbdf9845b0e4447536d8bcf2ec734ac9a969f Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Fri, 25 Jan 2019 15:28:53 +0100 Subject: [PATCH 09/12] C++: Test demonstrating chi node back edge bug This test shows that the back-edge detection does not properly account for chi nodes in the translation to aliased SSA. --- .../library-tests/ir/ir/PrintAST.expected | 44 +++++++++++++++++++ .../ir/ir/aliased_ssa_ir.expected | 43 ++++++++++++++++++ .../ir/ir/aliased_ssa_sanity.expected | 2 + cpp/ql/test/library-tests/ir/ir/ir.cpp | 5 +++ .../test/library-tests/ir/ir/raw_ir.expected | 39 ++++++++++++++++ .../ir/ir/ssa_block_count.expected | 1 + .../ir/ir/unaliased_ssa_ir.expected | 41 +++++++++++++++++ 7 files changed, 175 insertions(+) diff --git a/cpp/ql/test/library-tests/ir/ir/PrintAST.expected b/cpp/ql/test/library-tests/ir/ir/PrintAST.expected index 3959da834498..59f4763a2f04 100644 --- a/cpp/ql/test/library-tests/ir/ir/PrintAST.expected +++ b/cpp/ql/test/library-tests/ir/ir/PrintAST.expected @@ -6782,3 +6782,47 @@ ir.cpp: # 1055| 0: i # 1055| Type = int # 1055| ValueCategory = prvalue(load) +# 1058| chiNodeAtEndOfLoop(int, char *) -> void +# 1058| params: +# 1058| 0: n +# 1058| Type = int +# 1058| 1: p +# 1058| Type = char * +# 1058| body: { ... } +# 1059| 0: while (...) ... +# 1059| 0: ... > ... +# 1059| Type = bool +# 1059| ValueCategory = prvalue +# 1059| 0: ... -- +# 1059| Type = int +# 1059| ValueCategory = prvalue +# 1059| 0: n +# 1059| Type = int +# 1059| ValueCategory = lvalue +# 1059| 1: 0 +# 1059| Type = int +# 1059| Value = 0 +# 1059| ValueCategory = prvalue +# 1060| 1: ExprStmt +# 1060| 0: ... = ... +# 1060| Type = char +# 1060| ValueCategory = lvalue +# 1060| 0: * ... +# 1060| Type = char +# 1060| ValueCategory = lvalue +# 1060| 0: ... ++ +# 1060| Type = char * +# 1060| ValueCategory = prvalue +# 1060| 0: p +# 1060| Type = char * +# 1060| ValueCategory = lvalue +# 1060| 1: (char)... +# 1060| Conversion = integral conversion +# 1060| Type = char +# 1060| Value = 0 +# 1060| ValueCategory = prvalue +# 1060| expr: 0 +# 1060| Type = int +# 1060| Value = 0 +# 1060| ValueCategory = prvalue +# 1061| 1: return ... diff --git a/cpp/ql/test/library-tests/ir/ir/aliased_ssa_ir.expected b/cpp/ql/test/library-tests/ir/ir/aliased_ssa_ir.expected index 01e6f3b78f03..d8911b71ee81 100644 --- a/cpp/ql/test/library-tests/ir/ir/aliased_ssa_ir.expected +++ b/cpp/ql/test/library-tests/ir/ir/aliased_ssa_ir.expected @@ -4646,3 +4646,46 @@ ir.cpp: # 1049| Block 2 # 1049| v2_0(void) = Unreached : + +# 1058| chiNodeAtEndOfLoop(int, char *) -> void +# 1058| Block 0 +# 1058| v0_0(void) = EnterFunction : +# 1058| m0_1(unknown) = AliasedDefinition : +# 1058| mu0_2(unknown) = UnmodeledDefinition : +# 1058| r0_3(glval) = VariableAddress[n] : +# 1058| m0_4(int) = InitializeParameter[n] : r0_3 +# 1058| r0_5(glval) = VariableAddress[p] : +# 1058| m0_6(char *) = InitializeParameter[p] : r0_5 +#-----| Goto -> Block 3 + +# 1060| Block 1 +# 1060| r1_0(char) = Constant[0] : +# 1060| r1_1(glval) = VariableAddress[p] : +# 1060| r1_2(char *) = Load : r1_1, m3_2 +# 1060| r1_3(int) = Constant[1] : +# 1060| r1_4(char *) = PointerAdd[1] : r1_2, r1_3 +# 1060| m1_5(char *) = Store : r1_1, r1_4 +# 1060| m1_6(char) = Store : r1_2, r1_0 +# 1060| m1_7(unknown) = Chi : m3_0, m1_6 +#-----| Goto (back edge) -> Block 3 + +# 1061| Block 2 +# 1061| v2_0(void) = NoOp : +# 1058| v2_1(void) = ReturnVoid : +# 1058| v2_2(void) = UnmodeledUse : mu* +# 1058| v2_3(void) = ExitFunction : + +# 1059| Block 3 +# 1059| m3_0(unknown) = Phi : from 0:m0_1, from 1:m1_7 +# 1059| m3_1(int) = Phi : from 0:m0_4, from 1:m3_7 +# 1059| m3_2(char *) = Phi : from 0:m0_6, from 1:m1_5 +# 1059| r3_3(glval) = VariableAddress[n] : +# 1059| r3_4(int) = Load : r3_3, m3_1 +# 1059| r3_5(int) = Constant[1] : +# 1059| r3_6(int) = Sub : r3_4, r3_5 +# 1059| m3_7(int) = Store : r3_3, r3_6 +# 1059| r3_8(int) = Constant[0] : +# 1059| r3_9(bool) = CompareGT : r3_4, r3_8 +# 1059| v3_10(void) = ConditionalBranch : r3_9 +#-----| False (back edge) -> Block 2 +#-----| True (back edge) -> Block 1 diff --git a/cpp/ql/test/library-tests/ir/ir/aliased_ssa_sanity.expected b/cpp/ql/test/library-tests/ir/ir/aliased_ssa_sanity.expected index 220255b41e94..20b7fcbcbef4 100644 --- a/cpp/ql/test/library-tests/ir/ir/aliased_ssa_sanity.expected +++ b/cpp/ql/test/library-tests/ir/ir/aliased_ssa_sanity.expected @@ -8,4 +8,6 @@ operandAcrossFunctions instructionWithoutUniqueBlock containsLoopOfForwardEdges lostReachability +| ir.cpp:1060:12:1060:12 | Constant: (char)... | +| ir.cpp:1061:1:1061:1 | NoOp: return ... | backEdgeCountMismatch diff --git a/cpp/ql/test/library-tests/ir/ir/ir.cpp b/cpp/ql/test/library-tests/ir/ir/ir.cpp index 7872a58ee6fa..396d5603cc00 100644 --- a/cpp/ql/test/library-tests/ir/ir/ir.cpp +++ b/cpp/ql/test/library-tests/ir/ir/ir.cpp @@ -1055,4 +1055,9 @@ int DoWhileFalse() { return i; } +void chiNodeAtEndOfLoop(int n, char *p) { + while (n-- > 0) + *p++ = 0; +} + // semmle-extractor-options: -std=c++17 diff --git a/cpp/ql/test/library-tests/ir/ir/raw_ir.expected b/cpp/ql/test/library-tests/ir/ir/raw_ir.expected index 6b9ace79ac00..d0b60813b1d1 100644 --- a/cpp/ql/test/library-tests/ir/ir/raw_ir.expected +++ b/cpp/ql/test/library-tests/ir/ir/raw_ir.expected @@ -4544,3 +4544,42 @@ ir.cpp: # 1049| v2_5(void) = ReturnValue : r2_4, mu0_2 # 1049| v2_6(void) = UnmodeledUse : mu* # 1049| v2_7(void) = ExitFunction : + +# 1058| chiNodeAtEndOfLoop(int, char *) -> void +# 1058| Block 0 +# 1058| v0_0(void) = EnterFunction : +# 1058| mu0_1(unknown) = AliasedDefinition : +# 1058| mu0_2(unknown) = UnmodeledDefinition : +# 1058| r0_3(glval) = VariableAddress[n] : +# 1058| mu0_4(int) = InitializeParameter[n] : r0_3 +# 1058| r0_5(glval) = VariableAddress[p] : +# 1058| mu0_6(char *) = InitializeParameter[p] : r0_5 +#-----| Goto -> Block 3 + +# 1060| Block 1 +# 1060| r1_0(char) = Constant[0] : +# 1060| r1_1(glval) = VariableAddress[p] : +# 1060| r1_2(char *) = Load : r1_1, mu0_2 +# 1060| r1_3(int) = Constant[1] : +# 1060| r1_4(char *) = PointerAdd[1] : r1_2, r1_3 +# 1060| mu1_5(char *) = Store : r1_1, r1_4 +# 1060| mu1_6(char) = Store : r1_2, r1_0 +#-----| Goto (back edge) -> Block 3 + +# 1061| Block 2 +# 1061| v2_0(void) = NoOp : +# 1058| v2_1(void) = ReturnVoid : +# 1058| v2_2(void) = UnmodeledUse : mu* +# 1058| v2_3(void) = ExitFunction : + +# 1059| Block 3 +# 1059| r3_0(glval) = VariableAddress[n] : +# 1059| r3_1(int) = Load : r3_0, mu0_2 +# 1059| r3_2(int) = Constant[1] : +# 1059| r3_3(int) = Sub : r3_1, r3_2 +# 1059| mu3_4(int) = Store : r3_0, r3_3 +# 1059| r3_5(int) = Constant[0] : +# 1059| r3_6(bool) = CompareGT : r3_1, r3_5 +# 1059| v3_7(void) = ConditionalBranch : r3_6 +#-----| False -> Block 2 +#-----| True -> Block 1 diff --git a/cpp/ql/test/library-tests/ir/ir/ssa_block_count.expected b/cpp/ql/test/library-tests/ir/ir/ssa_block_count.expected index 83add17211f2..f2a6f5709a31 100644 --- a/cpp/ql/test/library-tests/ir/ir/ssa_block_count.expected +++ b/cpp/ql/test/library-tests/ir/ir/ssa_block_count.expected @@ -101,6 +101,7 @@ | IR: VirtualMemberFunction | 1 | | IR: WhileStatements | 4 | | IR: WhileStmtWithDeclaration | 8 | +| IR: chiNodeAtEndOfLoop | 4 | | IR: designatedInit | 1 | | IR: min | 4 | | IR: operator= | 1 | diff --git a/cpp/ql/test/library-tests/ir/ir/unaliased_ssa_ir.expected b/cpp/ql/test/library-tests/ir/ir/unaliased_ssa_ir.expected index 1b7da6e128c1..57b328394b99 100644 --- a/cpp/ql/test/library-tests/ir/ir/unaliased_ssa_ir.expected +++ b/cpp/ql/test/library-tests/ir/ir/unaliased_ssa_ir.expected @@ -4517,3 +4517,44 @@ ir.cpp: # 1049| Block 2 # 1049| v2_0(void) = Unreached : + +# 1058| chiNodeAtEndOfLoop(int, char *) -> void +# 1058| Block 0 +# 1058| v0_0(void) = EnterFunction : +# 1058| mu0_1(unknown) = AliasedDefinition : +# 1058| mu0_2(unknown) = UnmodeledDefinition : +# 1058| r0_3(glval) = VariableAddress[n] : +# 1058| m0_4(int) = InitializeParameter[n] : r0_3 +# 1058| r0_5(glval) = VariableAddress[p] : +# 1058| m0_6(char *) = InitializeParameter[p] : r0_5 +#-----| Goto -> Block 3 + +# 1060| Block 1 +# 1060| r1_0(char) = Constant[0] : +# 1060| r1_1(glval) = VariableAddress[p] : +# 1060| r1_2(char *) = Load : r1_1, m3_1 +# 1060| r1_3(int) = Constant[1] : +# 1060| r1_4(char *) = PointerAdd[1] : r1_2, r1_3 +# 1060| m1_5(char *) = Store : r1_1, r1_4 +# 1060| mu1_6(char) = Store : r1_2, r1_0 +#-----| Goto (back edge) -> Block 3 + +# 1061| Block 2 +# 1061| v2_0(void) = NoOp : +# 1058| v2_1(void) = ReturnVoid : +# 1058| v2_2(void) = UnmodeledUse : mu* +# 1058| v2_3(void) = ExitFunction : + +# 1059| Block 3 +# 1059| m3_0(int) = Phi : from 0:m0_4, from 1:m3_6 +# 1059| m3_1(char *) = Phi : from 0:m0_6, from 1:m1_5 +# 1059| r3_2(glval) = VariableAddress[n] : +# 1059| r3_3(int) = Load : r3_2, m3_0 +# 1059| r3_4(int) = Constant[1] : +# 1059| r3_5(int) = Sub : r3_3, r3_4 +# 1059| m3_6(int) = Store : r3_2, r3_5 +# 1059| r3_7(int) = Constant[0] : +# 1059| r3_8(bool) = CompareGT : r3_3, r3_7 +# 1059| v3_9(void) = ConditionalBranch : r3_8 +#-----| False -> Block 2 +#-----| True -> Block 1 From ba8bf94d7b0b635a992ba65bd3faf17c23e6a48d Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Fri, 25 Jan 2019 15:32:19 +0100 Subject: [PATCH 10/12] C++: Account for chi nodes in back-edge detection --- .../aliased_ssa/internal/SSAConstruction.qll | 15 ++++++++++++--- .../unaliased_ssa/internal/SSAConstruction.qll | 15 ++++++++++++--- .../library-tests/ir/ir/aliased_ssa_ir.expected | 4 ++-- .../ir/ir/aliased_ssa_sanity.expected | 2 -- 4 files changed, 26 insertions(+), 10 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/SSAConstruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/SSAConstruction.qll index 292f2c520651..41932972ef4f 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/SSAConstruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/SSAConstruction.qll @@ -280,9 +280,18 @@ cached private module Cached { cached Instruction getInstructionBackEdgeSuccessor(Instruction instruction, EdgeKind kind) { exists(OldInstruction oldInstruction | - oldInstruction = getOldInstruction(instruction) and - result = getNewInstruction(oldInstruction.getBackEdgeSuccessor(kind)) and - not Reachability::isInfeasibleInstructionSuccessor(oldInstruction, kind) + not Reachability::isInfeasibleInstructionSuccessor(oldInstruction, kind) and + // There is only one case for the translation into `result` because the + // SSA construction never inserts extra instructions _before_ an existing + // instruction. + getOldInstruction(result) = oldInstruction.getBackEdgeSuccessor(kind) and + // There are two cases for the translation into `instruction` because the + // SSA construction might have inserted a chi node _after_ + // `oldInstruction`, in which case the back edge should come out of the + // chi node instead. + if hasChiNode(_, oldInstruction) + then instruction = getChiInstruction(oldInstruction) + else instruction = getNewInstruction(oldInstruction) ) } diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SSAConstruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SSAConstruction.qll index 292f2c520651..41932972ef4f 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SSAConstruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SSAConstruction.qll @@ -280,9 +280,18 @@ cached private module Cached { cached Instruction getInstructionBackEdgeSuccessor(Instruction instruction, EdgeKind kind) { exists(OldInstruction oldInstruction | - oldInstruction = getOldInstruction(instruction) and - result = getNewInstruction(oldInstruction.getBackEdgeSuccessor(kind)) and - not Reachability::isInfeasibleInstructionSuccessor(oldInstruction, kind) + not Reachability::isInfeasibleInstructionSuccessor(oldInstruction, kind) and + // There is only one case for the translation into `result` because the + // SSA construction never inserts extra instructions _before_ an existing + // instruction. + getOldInstruction(result) = oldInstruction.getBackEdgeSuccessor(kind) and + // There are two cases for the translation into `instruction` because the + // SSA construction might have inserted a chi node _after_ + // `oldInstruction`, in which case the back edge should come out of the + // chi node instead. + if hasChiNode(_, oldInstruction) + then instruction = getChiInstruction(oldInstruction) + else instruction = getNewInstruction(oldInstruction) ) } diff --git a/cpp/ql/test/library-tests/ir/ir/aliased_ssa_ir.expected b/cpp/ql/test/library-tests/ir/ir/aliased_ssa_ir.expected index d8911b71ee81..4f9d11ddf58f 100644 --- a/cpp/ql/test/library-tests/ir/ir/aliased_ssa_ir.expected +++ b/cpp/ql/test/library-tests/ir/ir/aliased_ssa_ir.expected @@ -4687,5 +4687,5 @@ ir.cpp: # 1059| r3_8(int) = Constant[0] : # 1059| r3_9(bool) = CompareGT : r3_4, r3_8 # 1059| v3_10(void) = ConditionalBranch : r3_9 -#-----| False (back edge) -> Block 2 -#-----| True (back edge) -> Block 1 +#-----| False -> Block 2 +#-----| True -> Block 1 diff --git a/cpp/ql/test/library-tests/ir/ir/aliased_ssa_sanity.expected b/cpp/ql/test/library-tests/ir/ir/aliased_ssa_sanity.expected index 20b7fcbcbef4..220255b41e94 100644 --- a/cpp/ql/test/library-tests/ir/ir/aliased_ssa_sanity.expected +++ b/cpp/ql/test/library-tests/ir/ir/aliased_ssa_sanity.expected @@ -8,6 +8,4 @@ operandAcrossFunctions instructionWithoutUniqueBlock containsLoopOfForwardEdges lostReachability -| ir.cpp:1060:12:1060:12 | Constant: (char)... | -| ir.cpp:1061:1:1061:1 | NoOp: return ... | backEdgeCountMismatch From 9decbd9c9faf8f99055f1d365018fc212a858ef0 Mon Sep 17 00:00:00 2001 From: Robert Marsh Date: Fri, 25 Jan 2019 13:12:40 -0800 Subject: [PATCH 11/12] C++: new irreducible CFG test for range analysis --- .../rangeanalysis/RangeAnalysis.expected | 5 +++++ .../rangeanalysis/rangeanalysis/test.cpp | 16 ++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/cpp/ql/test/library-tests/rangeanalysis/rangeanalysis/RangeAnalysis.expected b/cpp/ql/test/library-tests/rangeanalysis/rangeanalysis/RangeAnalysis.expected index 7161fdb86892..cb3366e91b5a 100644 --- a/cpp/ql/test/library-tests/rangeanalysis/rangeanalysis/RangeAnalysis.expected +++ b/cpp/ql/test/library-tests/rangeanalysis/rangeanalysis/RangeAnalysis.expected @@ -55,3 +55,8 @@ | test.cpp:167:12:167:12 | Load: x | test.cpp:153:23:153:23 | InitializeParameter: y | 1 | false | CompareEQ: ... == ... | test.cpp:166:9:166:16 | test.cpp:166:9:166:16 | | test.cpp:167:12:167:12 | Load: x | test.cpp:153:23:153:23 | InitializeParameter: y | 1 | true | CompareEQ: ... == ... | test.cpp:166:9:166:16 | test.cpp:166:9:166:16 | | test.cpp:169:12:169:12 | Load: x | test.cpp:153:23:153:23 | InitializeParameter: y | 0 | false | CompareLT: ... < ... | test.cpp:154:6:154:10 | test.cpp:154:6:154:10 | +| test.cpp:177:10:177:10 | Load: i | test.cpp:175:23:175:23 | InitializeParameter: x | 1 | false | CompareLT: ... < ... | test.cpp:176:7:176:11 | test.cpp:176:7:176:11 | +| test.cpp:179:10:179:10 | Load: i | test.cpp:175:23:175:23 | InitializeParameter: x | 0 | true | CompareLT: ... < ... | test.cpp:176:7:176:11 | test.cpp:176:7:176:11 | +| test.cpp:183:10:183:10 | Load: i | test.cpp:175:23:175:23 | InitializeParameter: x | -1 | true | CompareLT: ... < ... | test.cpp:182:9:182:13 | test.cpp:182:9:182:13 | +| test.cpp:185:10:185:10 | Load: i | test.cpp:175:23:175:23 | InitializeParameter: x | 0 | true | CompareLT: ... < ... | test.cpp:176:7:176:11 | test.cpp:176:7:176:11 | +| test.cpp:187:10:187:10 | Store: i | test.cpp:175:23:175:23 | InitializeParameter: x | 0 | false | CompareLT: ... < ... | test.cpp:182:9:182:13 | test.cpp:182:9:182:13 | diff --git a/cpp/ql/test/library-tests/rangeanalysis/rangeanalysis/test.cpp b/cpp/ql/test/library-tests/rangeanalysis/rangeanalysis/test.cpp index 24f95205fcf8..bb0f23cea1b6 100644 --- a/cpp/ql/test/library-tests/rangeanalysis/rangeanalysis/test.cpp +++ b/cpp/ql/test/library-tests/rangeanalysis/rangeanalysis/test.cpp @@ -170,3 +170,19 @@ int test14(int x, int y) { } } } + +// more interesting bounds with irreducible CFG +int test15(int i, int x) { + if (x < i) { + sink(i); // i >= x + 1 + } else { + sink(i); // i <= x + goto inLoop; + } + for(; i < x; i++) { + sink(i); // i <= x - 1 + inLoop: + sink(i); // i <= x + } + return i; +} From b55573ebe31b80f1faf7a2d1ec530cadf3d294fc Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Thu, 31 Jan 2019 10:08:16 +0100 Subject: [PATCH 12/12] C++: Accept test changes in ir_gvn.expected --- .../valuenumbering/GlobalValueNumbering/ir_gvn.expected | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/ql/test/library-tests/valuenumbering/GlobalValueNumbering/ir_gvn.expected b/cpp/ql/test/library-tests/valuenumbering/GlobalValueNumbering/ir_gvn.expected index 877cdb36750b..484a26bd3874 100644 --- a/cpp/ql/test/library-tests/valuenumbering/GlobalValueNumbering/ir_gvn.expected +++ b/cpp/ql/test/library-tests/valuenumbering/GlobalValueNumbering/ir_gvn.expected @@ -446,7 +446,7 @@ test.cpp: # 56| valnum = r5_3 # 56| m5_4(char *) = Store : r5_0, r5_3 # 56| valnum = r5_3 -#-----| Goto -> Block 3 +#-----| Goto (back edge) -> Block 3 # 59| Block 6 # 59| r6_0(glval) = VariableAddress[ptr] : @@ -480,7 +480,7 @@ test.cpp: # 62| valnum = r8_3 # 62| m8_4(unsigned int) = Store : r8_0, r8_3 # 62| valnum = r8_3 -#-----| Goto -> Block 1 +#-----| Goto (back edge) -> Block 1 # 63| Block 9 # 63| v9_0(void) = NoOp :