From 8c06a68835bc43be3d521b4fcb7317d34d8ec79a Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Thu, 9 Aug 2018 15:19:04 +0200 Subject: [PATCH 1/3] C++ IR: Remove redundant check for same function The check that an instruction is in the same function as its operands is hopefully redundant and can be removed. Just to be sure, I've added the check to a sanity query. This check turned out to cause bad performance in the alias analysis because it got inlined into `AliasAnalysis::resultEscapes` and then pulled out to a loop-invariant predicate that got a bad join order. With this check removed, the `ssa/AliasAnalysis.qll` file is orders of magnitude faster. --- cpp/ql/src/semmle/code/cpp/ir/internal/Instruction.qll | 8 +++++++- .../code/cpp/ssa/internal/aliased_ssa/Instruction.qll | 8 +++++++- .../src/semmle/code/cpp/ssa/internal/ssa/Instruction.qll | 8 +++++++- 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/ir/internal/Instruction.qll b/cpp/ql/src/semmle/code/cpp/ir/internal/Instruction.qll index d00999f894aa..241f8cadd469 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/internal/Instruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/internal/Instruction.qll @@ -66,6 +66,13 @@ module InstructionSanity { count(instr.getOperand(tag)) > 1 and not tag instanceof UnmodeledUseOperand } + + query predicate operandAcrossFunctions( + Instruction op, Instruction operand, OperandTag tag + ) { + operand = op.getOperand(tag) and + operand.getFunctionIR() != op.getFunctionIR() + } } /** @@ -302,7 +309,6 @@ class Instruction extends Construction::TInstruction { * an operand with tag `useTag`. */ final predicate hasUse(Instruction useInstruction, OperandTag useTag) { - useInstruction.getFunctionIR() = funcIR and this = useInstruction.getOperand(useTag) } } diff --git a/cpp/ql/src/semmle/code/cpp/ssa/internal/aliased_ssa/Instruction.qll b/cpp/ql/src/semmle/code/cpp/ssa/internal/aliased_ssa/Instruction.qll index d00999f894aa..241f8cadd469 100644 --- a/cpp/ql/src/semmle/code/cpp/ssa/internal/aliased_ssa/Instruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ssa/internal/aliased_ssa/Instruction.qll @@ -66,6 +66,13 @@ module InstructionSanity { count(instr.getOperand(tag)) > 1 and not tag instanceof UnmodeledUseOperand } + + query predicate operandAcrossFunctions( + Instruction op, Instruction operand, OperandTag tag + ) { + operand = op.getOperand(tag) and + operand.getFunctionIR() != op.getFunctionIR() + } } /** @@ -302,7 +309,6 @@ class Instruction extends Construction::TInstruction { * an operand with tag `useTag`. */ final predicate hasUse(Instruction useInstruction, OperandTag useTag) { - useInstruction.getFunctionIR() = funcIR and this = useInstruction.getOperand(useTag) } } diff --git a/cpp/ql/src/semmle/code/cpp/ssa/internal/ssa/Instruction.qll b/cpp/ql/src/semmle/code/cpp/ssa/internal/ssa/Instruction.qll index d00999f894aa..241f8cadd469 100644 --- a/cpp/ql/src/semmle/code/cpp/ssa/internal/ssa/Instruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ssa/internal/ssa/Instruction.qll @@ -66,6 +66,13 @@ module InstructionSanity { count(instr.getOperand(tag)) > 1 and not tag instanceof UnmodeledUseOperand } + + query predicate operandAcrossFunctions( + Instruction op, Instruction operand, OperandTag tag + ) { + operand = op.getOperand(tag) and + operand.getFunctionIR() != op.getFunctionIR() + } } /** @@ -302,7 +309,6 @@ class Instruction extends Construction::TInstruction { * an operand with tag `useTag`. */ final predicate hasUse(Instruction useInstruction, OperandTag useTag) { - useInstruction.getFunctionIR() = funcIR and this = useInstruction.getOperand(useTag) } } From 961a7dcf1524ed8c3dd7475b05440660057b4dff Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Thu, 9 Aug 2018 15:33:40 +0200 Subject: [PATCH 2/3] C++ IR: Remove Instruction.hasUse predicate Now that it's been simplified to be the same as `getOperand`, it doesn't seem to have a purpose. --- .../src/semmle/code/cpp/ir/internal/Instruction.qll | 12 ++---------- .../cpp/ssa/internal/aliased_ssa/AliasAnalysis.qll | 2 +- .../cpp/ssa/internal/aliased_ssa/Instruction.qll | 12 ++---------- .../code/cpp/ssa/internal/ssa/AliasAnalysis.qll | 2 +- .../semmle/code/cpp/ssa/internal/ssa/Instruction.qll | 12 ++---------- 5 files changed, 8 insertions(+), 32 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/ir/internal/Instruction.qll b/cpp/ql/src/semmle/code/cpp/ir/internal/Instruction.qll index 241f8cadd469..661136c31124 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/internal/Instruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/internal/Instruction.qll @@ -269,8 +269,8 @@ class Instruction extends Construction::TInstruction { // Register results are always in SSA form. not hasMemoryResult() or // An unmodeled result will have a use on the `UnmodeledUse` instruction. - not exists(UnmodeledUseOperand useTag | - hasUse(_, useTag) + not exists(Instruction useInstr, UnmodeledUseOperand useTag | + this = useInstr.getOperand(useTag) ) } @@ -303,14 +303,6 @@ class Instruction extends Construction::TInstruction { final Instruction getAPredecessor() { result = getPredecessor(_) } - - /** - * Holds if the result of this instruction is consumed by `useInstruction` as - * an operand with tag `useTag`. - */ - final predicate hasUse(Instruction useInstruction, OperandTag useTag) { - this = useInstruction.getOperand(useTag) - } } class VariableInstruction extends Instruction { diff --git a/cpp/ql/src/semmle/code/cpp/ssa/internal/aliased_ssa/AliasAnalysis.qll b/cpp/ql/src/semmle/code/cpp/ssa/internal/aliased_ssa/AliasAnalysis.qll index c5fd6e1d5ae6..cc42ce1c96ea 100644 --- a/cpp/ql/src/semmle/code/cpp/ssa/internal/aliased_ssa/AliasAnalysis.qll +++ b/cpp/ql/src/semmle/code/cpp/ssa/internal/aliased_ssa/AliasAnalysis.qll @@ -160,7 +160,7 @@ predicate operandEscapes(Instruction instr, OperandTag tag) { predicate resultEscapes(Instruction instr) { // The result escapes if it has at least one use that escapes. exists(Instruction useInstr, OperandTag useOperandTag | - instr.hasUse(useInstr, useOperandTag) and + useInstr.getOperand(useOperandTag) = instr and operandEscapes(useInstr, useOperandTag) ) } diff --git a/cpp/ql/src/semmle/code/cpp/ssa/internal/aliased_ssa/Instruction.qll b/cpp/ql/src/semmle/code/cpp/ssa/internal/aliased_ssa/Instruction.qll index 241f8cadd469..661136c31124 100644 --- a/cpp/ql/src/semmle/code/cpp/ssa/internal/aliased_ssa/Instruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ssa/internal/aliased_ssa/Instruction.qll @@ -269,8 +269,8 @@ class Instruction extends Construction::TInstruction { // Register results are always in SSA form. not hasMemoryResult() or // An unmodeled result will have a use on the `UnmodeledUse` instruction. - not exists(UnmodeledUseOperand useTag | - hasUse(_, useTag) + not exists(Instruction useInstr, UnmodeledUseOperand useTag | + this = useInstr.getOperand(useTag) ) } @@ -303,14 +303,6 @@ class Instruction extends Construction::TInstruction { final Instruction getAPredecessor() { result = getPredecessor(_) } - - /** - * Holds if the result of this instruction is consumed by `useInstruction` as - * an operand with tag `useTag`. - */ - final predicate hasUse(Instruction useInstruction, OperandTag useTag) { - this = useInstruction.getOperand(useTag) - } } class VariableInstruction extends Instruction { diff --git a/cpp/ql/src/semmle/code/cpp/ssa/internal/ssa/AliasAnalysis.qll b/cpp/ql/src/semmle/code/cpp/ssa/internal/ssa/AliasAnalysis.qll index c5fd6e1d5ae6..cc42ce1c96ea 100644 --- a/cpp/ql/src/semmle/code/cpp/ssa/internal/ssa/AliasAnalysis.qll +++ b/cpp/ql/src/semmle/code/cpp/ssa/internal/ssa/AliasAnalysis.qll @@ -160,7 +160,7 @@ predicate operandEscapes(Instruction instr, OperandTag tag) { predicate resultEscapes(Instruction instr) { // The result escapes if it has at least one use that escapes. exists(Instruction useInstr, OperandTag useOperandTag | - instr.hasUse(useInstr, useOperandTag) and + useInstr.getOperand(useOperandTag) = instr and operandEscapes(useInstr, useOperandTag) ) } diff --git a/cpp/ql/src/semmle/code/cpp/ssa/internal/ssa/Instruction.qll b/cpp/ql/src/semmle/code/cpp/ssa/internal/ssa/Instruction.qll index 241f8cadd469..661136c31124 100644 --- a/cpp/ql/src/semmle/code/cpp/ssa/internal/ssa/Instruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ssa/internal/ssa/Instruction.qll @@ -269,8 +269,8 @@ class Instruction extends Construction::TInstruction { // Register results are always in SSA form. not hasMemoryResult() or // An unmodeled result will have a use on the `UnmodeledUse` instruction. - not exists(UnmodeledUseOperand useTag | - hasUse(_, useTag) + not exists(Instruction useInstr, UnmodeledUseOperand useTag | + this = useInstr.getOperand(useTag) ) } @@ -303,14 +303,6 @@ class Instruction extends Construction::TInstruction { final Instruction getAPredecessor() { result = getPredecessor(_) } - - /** - * Holds if the result of this instruction is consumed by `useInstruction` as - * an operand with tag `useTag`. - */ - final predicate hasUse(Instruction useInstruction, OperandTag useTag) { - this = useInstruction.getOperand(useTag) - } } class VariableInstruction extends Instruction { From c92111d5521a4542a44186d6a3b574bccdfcf10b Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Fri, 10 Aug 2018 09:04:52 +0200 Subject: [PATCH 3/3] C++: Accept test changes: IR sanity query added --- cpp/ql/test/library-tests/ir/ir/AliasedSSAIRSanity.expected | 1 + cpp/ql/test/library-tests/ir/ir/IRSanity.expected | 1 + cpp/ql/test/library-tests/ir/ir/SSAIRSanity.expected | 1 + 3 files changed, 3 insertions(+) diff --git a/cpp/ql/test/library-tests/ir/ir/AliasedSSAIRSanity.expected b/cpp/ql/test/library-tests/ir/ir/AliasedSSAIRSanity.expected index 3d93d822b3cf..f05c1067fb9f 100644 --- a/cpp/ql/test/library-tests/ir/ir/AliasedSSAIRSanity.expected +++ b/cpp/ql/test/library-tests/ir/ir/AliasedSSAIRSanity.expected @@ -1,3 +1,4 @@ missingOperand unexpectedOperand duplicateOperand +operandAcrossFunctions diff --git a/cpp/ql/test/library-tests/ir/ir/IRSanity.expected b/cpp/ql/test/library-tests/ir/ir/IRSanity.expected index 3d93d822b3cf..f05c1067fb9f 100644 --- a/cpp/ql/test/library-tests/ir/ir/IRSanity.expected +++ b/cpp/ql/test/library-tests/ir/ir/IRSanity.expected @@ -1,3 +1,4 @@ missingOperand unexpectedOperand duplicateOperand +operandAcrossFunctions diff --git a/cpp/ql/test/library-tests/ir/ir/SSAIRSanity.expected b/cpp/ql/test/library-tests/ir/ir/SSAIRSanity.expected index 3d93d822b3cf..f05c1067fb9f 100644 --- a/cpp/ql/test/library-tests/ir/ir/SSAIRSanity.expected +++ b/cpp/ql/test/library-tests/ir/ir/SSAIRSanity.expected @@ -1,3 +1,4 @@ missingOperand unexpectedOperand duplicateOperand +operandAcrossFunctions