Address review.

Most important fix is that VNLength is now restricted to the subset
of value numbers that are Bounds in the RangeAnalysis.

cpp/ql/src/experimental/library/ArrayLengthAnalysis.qll

@@ -25,14 +25,30 @@ private import semmle.code.cpp.rangeanalysis.RangeUtils
 private newtype TLength =
   TZeroLength() or
   TVNLength(ValueNumber vn) {
+    not vn.getAnInstruction() instanceof ConstantInstruction and
     exists(Instruction i |
       vn.getAnInstruction() = i and
       (
         i.getResultIRType() instanceof IRSignedIntegerType or
         i.getResultIRType() instanceof IRUnsignedIntegerType
       )
-    ) and
-    not vn.getAnInstruction() instanceof ConstantInstruction
+    |
+      i instanceof PhiInstruction
+      or
+      i instanceof InitializeParameterInstruction
+      or
+      i instanceof CallInstruction
+      or
+      i instanceof VariableAddressInstruction
+      or
+      i instanceof FieldAddressInstruction
+      or
+      i.(LoadInstruction).getSourceAddress() instanceof VariableAddressInstruction
+      or
+      i.(LoadInstruction).getSourceAddress() instanceof FieldAddressInstruction
+      or
+      i.getAUse() instanceof ArgumentOperand
+    )
   }
 
 /**
@@ -40,8 +56,8 @@ private newtype TLength =
  * This class keeps track of the ValueNumber or Zero.
  * The delta is tracked in the predicate `knownArrayLength`.
  */
-abstract class Length extends TLength {
-  abstract string toString();
+class Length extends TLength {
+  string toString() { none() } // overridden in subclasses
 }
 
 /**
@@ -62,7 +78,7 @@ class VNLength extends Length, TVNLength {
 
   VNLength() { this = TVNLength(vn) }
 
-  /** Gets the SSA variable that equals value number bound. */
+  /** Gets an instruction with this value number bound. */
   Instruction getInstruction() { this = TVNLength(valueNumber(result)) }
 
   ValueNumber getValueNumber() { result = vn }
@@ -81,8 +97,8 @@ private newtype TOffset =
  * This class describes the offset of a pointer in a chunk of memory.
  * It is either an `Operand` or zero, an additional integer delta is added later.
  */
-abstract class Offset extends TOffset {
-  abstract string toString();
+class Offset extends TOffset {
+  string toString() { none() } // overridden in subclasses
 }
 
 /**
@@ -203,12 +219,10 @@ private predicate allocation(Instruction array, Length length, int delta) {
       (
         exists(Expr lengthExpr |
           deconstructMallocSizeExpr(alloc.getSizeExpr(), lengthExpr, delta) and
-          // TODO converted or unconverted here?
           length.(VNLength).getInstruction().getConvertedResultExpression() = lengthExpr
         )
         or
         not exists(int d | deconstructMallocSizeExpr(alloc.getSizeExpr(), _, d)) and
-        // TODO converted or unconverted here?
         length.(VNLength).getInstruction().getConvertedResultExpression() = alloc.getSizeExpr() and
         delta = 0
       )
@@ -245,14 +259,13 @@ predicate arrayAllocationOrDeclaration(Instruction array, Length length, int len
  * Holds if the instruction `array` represents a pointer to a chunk of memory that holds
  * `length + lengthDelta` elements, using only local analysis.
  * `array` points at `offset + offsetDelta` in the chunk of memory.
- * The pointer is in-bounds if `offset < length + lengthDelta` and
+ * The pointer is in-bounds if `offset + offsetDelta < length + lengthDelta` and
  * `offset + offsetDelta >= 0` holds.
- * The pointer is out-of-bounds if `offset >= length + lengthDelta`
+ * The pointer is out-of-bounds if `offset + offsetDelta >= length + lengthDelta`
  * or `offset + offsetDelta < 0` holds.
  * All pointers in this predicate are guaranteed to be non-null,
  * but are not guaranteed to be live.
  */
-cached
 predicate knownArrayLength(
   Instruction array, Length length, int lengthDelta, Offset offset, int offsetDelta
 ) {

cpp/ql/test/experimental/library-tests/arraylengthanalysis/ArrayLengthAnalysisTest.expected

@@ -2,7 +2,6 @@
 | test.cpp:19:8:19:8 | Load: a | VNLength(Chi: ptr) | 0 | ZeroOffset | 0 |
 | test.cpp:21:8:21:8 | Load: a | VNLength(Chi: ptr) | -1 | ZeroOffset | 0 |
 | test.cpp:23:8:23:8 | Load: a | VNLength(Chi: ptr) | 1 | ZeroOffset | 0 |
-| test.cpp:25:8:25:8 | Load: a | VNLength(Mul: ... * ...) | 0 | ZeroOffset | 0 |
 | test.cpp:27:8:27:8 | Load: c | VNLength(Chi: ptr) | 0 | ZeroOffset | 0 |
 | test.cpp:28:8:28:24 | Convert: (unsigned char *)... | VNLength(Chi: ptr) | 0 | ZeroOffset | 0 |
 | test.cpp:30:8:30:8 | Load: v | VNLength(Chi: ptr) | 0 | ZeroOffset | 0 |
@@ -22,4 +21,3 @@
 | test.cpp:80:8:80:8 | Load: a | VNLength(InitializeParameter: count) | 1 | OpOffset(Load: count) | 1 |
 | test.cpp:85:8:85:8 | Load: a | VNLength(InitializeParameter: count) | 1 | OpOffset(Add: ... + ...) | 0 |
 | test.cpp:87:8:87:8 | Load: a | VNLength(InitializeParameter: count) | 1 | OpOffset(Add: ... + ...) | 1 |
-| test.cpp:89:8:89:8 | Load: a | VNLength(Sub: ... - ...) | 0 | ZeroOffset | 0 |

cpp/ql/test/experimental/library-tests/arraylengthanalysis/test.cpp

@@ -22,7 +22,7 @@ void test1(unsigned int count) {
   a = (int *) malloc(sizeof(int) * (count + 1));
   sink(a); // (count, 1, Zero, 0)
   a = (int *) malloc(sizeof(int) * (2 * count));
-  sink(a); // (2*count, 0, Zero, 0)
+  sink(a); // none, as the size expression is too complicated
   char* c = (char *)malloc(count);
   sink(c); // /count, 0, Zero, 0)
   sink((unsigned char*)c); // (count, 0, Zero, 0)
@@ -86,5 +86,5 @@ void test2(unsigned int count, bool b) {
   a += 1;
   sink(a); // TODO, should be (count, 1, count, 2), but is (count, 1, count + 1, 1)
   a = (int*) malloc(sizeof(int) * (1024 - count));
-  sink(a); // (1024-count, 0, Zero, 0)
+  sink(a); // none, as the size expression is too complicated
 }