Merge pull request #1382 from jbj/redundant-null-check-gvn

Approved by dave-bartolomeo

cpp/ql/src/Likely Bugs/RedundantNullCheckSimple.ql

@@ -13,10 +13,11 @@
 
 /*
  * Note: this query is not assigned a precision yet because we don't want it on
- * LGTM until its performance is well understood. It's also lacking qhelp.
+ * LGTM until its performance is well understood.
  */
 
 import semmle.code.cpp.ir.IR
+import semmle.code.cpp.ir.ValueNumbering
 
 class NullInstruction extends ConstantValueInstruction {
   NullInstruction() {
@@ -25,17 +26,6 @@ class NullInstruction extends ConstantValueInstruction {
   }
 }
 
-/**
- * An instruction that will never have slicing on its result.
- */
-class SingleValuedInstruction extends Instruction {
-  SingleValuedInstruction() {
-    this.getResultMemoryAccess() instanceof IndirectMemoryAccess
-    or
-    not this.hasMemoryResult()
-  }
-}
-
 predicate explicitNullTestOfInstruction(Instruction checked, Instruction bool) {
   bool = any(CompareInstruction cmp |
       exists(NullInstruction null |
@@ -56,22 +46,24 @@ predicate explicitNullTestOfInstruction(Instruction checked, Instruction bool) {
     )
 }
 
-predicate candidateResult(LoadInstruction checked, SingleValuedInstruction sourceValue)
+pragma[noinline]
+predicate candidateResult(LoadInstruction checked, ValueNumber value, IRBlock dominator)
 {
   explicitNullTestOfInstruction(checked, _) and
   not checked.getAST().isInMacroExpansion() and
-  sourceValue = checked.getSourceValue()
+  value.getAnInstruction() = checked and
+  dominator.dominates(checked.getBlock())
 }
 
-from LoadInstruction checked, LoadInstruction deref, SingleValuedInstruction sourceValue
+from LoadInstruction checked, LoadInstruction deref, ValueNumber sourceValue, IRBlock dominator
 where
-  candidateResult(checked, sourceValue) and
-  sourceValue = deref.getSourceAddress().(LoadInstruction).getSourceValue() and
+  candidateResult(checked, sourceValue, dominator) and
+  sourceValue.getAnInstruction() = deref.getSourceAddress() and
   // This also holds if the blocks are equal, meaning that the check could come
   // before the deref. That's still not okay because when they're in the same
   // basic block then the deref is unavoidable even if the check concluded that
   // the pointer was null. To follow this idea to its full generality, we
   // should also give an alert when `check` post-dominates `deref`.
-  deref.getBlock().dominates(checked.getBlock())
+  deref.getBlock() = dominator
 select checked, "This null check is redundant because the value is $@ in any case", deref,
   "dereferenced here"

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/gvn/ValueNumbering.qll

@@ -105,19 +105,10 @@ class ValueNumber extends TValueNumber {
  * definition because it accesses the exact same memory.
  * The use of `p.x` on line 3 is linked to the definition of `p` on line 1 as well, but is not
  * congruent to that definition because `p.x` accesses only a subset of the memory defined by `p`.
- *
- * This concept should probably be exposed in the public IR API.
  */
 private class CongruentCopyInstruction extends CopyInstruction {
   CongruentCopyInstruction() {
-    exists(Instruction def |
-      def = this.getSourceValue() and
-      (
-        def.getResultMemoryAccess() instanceof IndirectMemoryAccess or
-        def.getResultMemoryAccess() instanceof PhiMemoryAccess or
-        not def.hasMemoryResult()
-      )
-    )
+    this.getSourceValueOperand().getDefinitionOverlap() instanceof MustExactlyOverlap
   }
 }
 

cpp/ql/src/semmle/code/cpp/ir/implementation/raw/gvn/ValueNumbering.qll

@@ -105,19 +105,10 @@ class ValueNumber extends TValueNumber {
  * definition because it accesses the exact same memory.
  * The use of `p.x` on line 3 is linked to the definition of `p` on line 1 as well, but is not
  * congruent to that definition because `p.x` accesses only a subset of the memory defined by `p`.
- *
- * This concept should probably be exposed in the public IR API.
  */
 private class CongruentCopyInstruction extends CopyInstruction {
   CongruentCopyInstruction() {
-    exists(Instruction def |
-      def = this.getSourceValue() and
-      (
-        def.getResultMemoryAccess() instanceof IndirectMemoryAccess or
-        def.getResultMemoryAccess() instanceof PhiMemoryAccess or
-        not def.hasMemoryResult()
-      )
-    )
+    this.getSourceValueOperand().getDefinitionOverlap() instanceof MustExactlyOverlap
   }
 }
 

cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/gvn/ValueNumbering.qll

@@ -105,19 +105,10 @@ class ValueNumber extends TValueNumber {
  * definition because it accesses the exact same memory.
  * The use of `p.x` on line 3 is linked to the definition of `p` on line 1 as well, but is not
  * congruent to that definition because `p.x` accesses only a subset of the memory defined by `p`.
- *
- * This concept should probably be exposed in the public IR API.
  */
 private class CongruentCopyInstruction extends CopyInstruction {
   CongruentCopyInstruction() {
-    exists(Instruction def |
-      def = this.getSourceValue() and
-      (
-        def.getResultMemoryAccess() instanceof IndirectMemoryAccess or
-        def.getResultMemoryAccess() instanceof PhiMemoryAccess or
-        not def.hasMemoryResult()
-      )
-    )
+    this.getSourceValueOperand().getDefinitionOverlap() instanceof MustExactlyOverlap
   }
 }