From 39854a3f7b270e12b5fa21dbf9b209ac04bcba83 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Fri, 28 Jun 2019 10:15:35 +0200 Subject: [PATCH 1/2] C++ IR: guard against cycles in operand graph This doesn't fix the underlying problem that for some reason there are cycles in the operand graph on our snapshots of the Linux kernel, but it ensures that the cycles don't lead to non-termination of `ConstantAnalysis` and `ValueNumbering`. --- .../ir/implementation/aliased_ssa/Operand.qll | 26 +++++++++++++++++-- .../cpp/ir/implementation/raw/Operand.qll | 26 +++++++++++++++++-- .../implementation/unaliased_ssa/Operand.qll | 26 +++++++++++++++++-- 3 files changed, 72 insertions(+), 6 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Operand.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Operand.qll index 84562760140..1f20a2c1677 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Operand.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Operand.qll @@ -8,15 +8,37 @@ private import semmle.code.cpp.ir.internal.OperandTag private newtype TOperand = TRegisterOperand(Instruction useInstr, RegisterOperandTag tag, Instruction defInstr) { - defInstr = Construction::getRegisterOperandDefinition(useInstr, tag) + defInstr = Construction::getRegisterOperandDefinition(useInstr, tag) and + not isInCycle(useInstr) } or TNonPhiMemoryOperand(Instruction useInstr, MemoryOperandTag tag, Instruction defInstr, Overlap overlap) { - defInstr = Construction::getMemoryOperandDefinition(useInstr, tag, overlap) + defInstr = Construction::getMemoryOperandDefinition(useInstr, tag, overlap) and + not isInCycle(useInstr) } or TPhiOperand(PhiInstruction useInstr, Instruction defInstr, IRBlock predecessorBlock, Overlap overlap) { defInstr = Construction::getPhiOperandDefinition(useInstr, predecessorBlock, overlap) } +/** Gets a non-phi instruction that defines an operand of `instr`. */ +private Instruction getNonPhiOperandDef(Instruction instr) { + result = Construction::getRegisterOperandDefinition(instr, _) + or + result = Construction::getMemoryOperandDefinition(instr, _, _) +} + +/** + * Holds if `instr` is part of a cycle in the operand graph that doesn't go + * through a phi instruction and therefore should be impossible. + * + * If such cycles are present, either due to a programming error in the IR + * generation or due to a malformed database, it can cause infinite loops in + * analyses that assume a cycle-free graph of non-phi operands. Therefore it's + * better to remove these operands than to leave cycles in the operand graph. + */ +private predicate isInCycle(Instruction instr) { + getNonPhiOperandDef+(instr) = instr +} + /** * A source operand of an `Instruction`. The operand represents a value consumed by the instruction. */ diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Operand.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Operand.qll index 84562760140..1f20a2c1677 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Operand.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Operand.qll @@ -8,15 +8,37 @@ private import semmle.code.cpp.ir.internal.OperandTag private newtype TOperand = TRegisterOperand(Instruction useInstr, RegisterOperandTag tag, Instruction defInstr) { - defInstr = Construction::getRegisterOperandDefinition(useInstr, tag) + defInstr = Construction::getRegisterOperandDefinition(useInstr, tag) and + not isInCycle(useInstr) } or TNonPhiMemoryOperand(Instruction useInstr, MemoryOperandTag tag, Instruction defInstr, Overlap overlap) { - defInstr = Construction::getMemoryOperandDefinition(useInstr, tag, overlap) + defInstr = Construction::getMemoryOperandDefinition(useInstr, tag, overlap) and + not isInCycle(useInstr) } or TPhiOperand(PhiInstruction useInstr, Instruction defInstr, IRBlock predecessorBlock, Overlap overlap) { defInstr = Construction::getPhiOperandDefinition(useInstr, predecessorBlock, overlap) } +/** Gets a non-phi instruction that defines an operand of `instr`. */ +private Instruction getNonPhiOperandDef(Instruction instr) { + result = Construction::getRegisterOperandDefinition(instr, _) + or + result = Construction::getMemoryOperandDefinition(instr, _, _) +} + +/** + * Holds if `instr` is part of a cycle in the operand graph that doesn't go + * through a phi instruction and therefore should be impossible. + * + * If such cycles are present, either due to a programming error in the IR + * generation or due to a malformed database, it can cause infinite loops in + * analyses that assume a cycle-free graph of non-phi operands. Therefore it's + * better to remove these operands than to leave cycles in the operand graph. + */ +private predicate isInCycle(Instruction instr) { + getNonPhiOperandDef+(instr) = instr +} + /** * A source operand of an `Instruction`. The operand represents a value consumed by the instruction. */ diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Operand.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Operand.qll index 84562760140..1f20a2c1677 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Operand.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Operand.qll @@ -8,15 +8,37 @@ private import semmle.code.cpp.ir.internal.OperandTag private newtype TOperand = TRegisterOperand(Instruction useInstr, RegisterOperandTag tag, Instruction defInstr) { - defInstr = Construction::getRegisterOperandDefinition(useInstr, tag) + defInstr = Construction::getRegisterOperandDefinition(useInstr, tag) and + not isInCycle(useInstr) } or TNonPhiMemoryOperand(Instruction useInstr, MemoryOperandTag tag, Instruction defInstr, Overlap overlap) { - defInstr = Construction::getMemoryOperandDefinition(useInstr, tag, overlap) + defInstr = Construction::getMemoryOperandDefinition(useInstr, tag, overlap) and + not isInCycle(useInstr) } or TPhiOperand(PhiInstruction useInstr, Instruction defInstr, IRBlock predecessorBlock, Overlap overlap) { defInstr = Construction::getPhiOperandDefinition(useInstr, predecessorBlock, overlap) } +/** Gets a non-phi instruction that defines an operand of `instr`. */ +private Instruction getNonPhiOperandDef(Instruction instr) { + result = Construction::getRegisterOperandDefinition(instr, _) + or + result = Construction::getMemoryOperandDefinition(instr, _, _) +} + +/** + * Holds if `instr` is part of a cycle in the operand graph that doesn't go + * through a phi instruction and therefore should be impossible. + * + * If such cycles are present, either due to a programming error in the IR + * generation or due to a malformed database, it can cause infinite loops in + * analyses that assume a cycle-free graph of non-phi operands. Therefore it's + * better to remove these operands than to leave cycles in the operand graph. + */ +private predicate isInCycle(Instruction instr) { + getNonPhiOperandDef+(instr) = instr +} + /** * A source operand of an `Instruction`. The operand represents a value consumed by the instruction. */ From 523fc9c1ceabb4b0ced148c3a83def6b72ece64f Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Tue, 9 Jul 2019 15:29:30 +0200 Subject: [PATCH 2/2] C++ IR: make `isInCycle` fast Without this `pragma[noopt]`, `isInCycle` gets compiled into RA that unpacks every tuple of the fast TC: 0 ~0% {2} r1 = SELECT #Operand::getNonPhiOperandDef#3#ffPlus ON FIELDS #Operand::getNonPhiOperandDef#3#ffPlus.<0>=#Operand::getNonPhiOperandDef#3#ffPlus.<1> 0 ~0% {1} r2 = SCAN r1 OUTPUT FIELDS {r1.<0>} return r2 With this change, it just becomes one lookup in the fast TC data structure per instruction. --- .../semmle/code/cpp/ir/implementation/aliased_ssa/Operand.qll | 2 ++ cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Operand.qll | 2 ++ .../semmle/code/cpp/ir/implementation/unaliased_ssa/Operand.qll | 2 ++ 3 files changed, 6 insertions(+) diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Operand.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Operand.qll index 1f20a2c1677..8cebecae678 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Operand.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Operand.qll @@ -35,7 +35,9 @@ private Instruction getNonPhiOperandDef(Instruction instr) { * analyses that assume a cycle-free graph of non-phi operands. Therefore it's * better to remove these operands than to leave cycles in the operand graph. */ +pragma[noopt] private predicate isInCycle(Instruction instr) { + instr instanceof Instruction and getNonPhiOperandDef+(instr) = instr } diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Operand.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Operand.qll index 1f20a2c1677..8cebecae678 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Operand.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Operand.qll @@ -35,7 +35,9 @@ private Instruction getNonPhiOperandDef(Instruction instr) { * analyses that assume a cycle-free graph of non-phi operands. Therefore it's * better to remove these operands than to leave cycles in the operand graph. */ +pragma[noopt] private predicate isInCycle(Instruction instr) { + instr instanceof Instruction and getNonPhiOperandDef+(instr) = instr } diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Operand.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Operand.qll index 1f20a2c1677..8cebecae678 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Operand.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Operand.qll @@ -35,7 +35,9 @@ private Instruction getNonPhiOperandDef(Instruction instr) { * analyses that assume a cycle-free graph of non-phi operands. Therefore it's * better to remove these operands than to leave cycles in the operand graph. */ +pragma[noopt] private predicate isInCycle(Instruction instr) { + instr instanceof Instruction and getNonPhiOperandDef+(instr) = instr }