[PATCH] D37478: [analyzer] Implement pointer arithmetic on constants

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I've seen this recently, and while i agree that the fix is correct, i'm not entirely sure that the test cases are correct. As weird as this may sound, null dereference is not an attempt to read from or write to memory address 0. Instead, it is about using a null pointer as if it was pointing to an actual object in memory, even if accessing it by a non-zero offset. For example, in

  struct S {
    int x, y;
  };
  
  void foo() {
    struct S *s = NULL;
    s->y = 1;
  }

we're in fact writing into `*(0x4)`, not `*(0x0)`, however it's intuitive that this code has a //null// pointer dereference, because we use a null pointer `s` as if it points to an actual object of type `struct S`. In this sense, i'd actually want the analyzer to warn in `test4`: it seems that the author of this code was expecting to find something useful by offset 1 to the pointer, so he must have made a mistake. Also i'm not entirely sure if i want the analyzer to warn in `test1`, `test2`, `test3` (also this code pattern doesn't look widespread/idiomatic).

So the analyzer does this really really weird thing by treating many operations with concrete pointers as no-ops, keeping the pointer null when it was null before, and keeping it non-null when it was non-null before - not just in the place that you've fixed, but also when computing field or element or base-class offsets for null pointers. These are marked as FIXME all over the place, but taking up the fix would require to provide another heuristic to distinguish null dereferences from other fixed-address dereferences (i.e. the experimental `FixedAddressChecker`).

I guess there should be more comments on this issue near the FIXMEs you fixed, and more tests covering the intended behavior; adding them is definitely a good thing to do. I do not have any immediate ideas on how to fix the issue as a whole.



================
Comment at: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:938
       llvm::APSInt Multiplicand(rightI.getBitWidth(), /* isUnsigned */ true);
+      QualType PteeTy = resultTy.getTypePtr()->castAs<PointerType>()->getPointeeType();
+      Multiplicand = getContext().getTypeSizeInChars(PteeTy).getQuantity();
----------------
xazax.hun wrote:
> The rest of the code does not abbreviate the Type. I would prefer to name this `pointeeType`.
Also `resultTy->getPointeeType()`. Note the fancy `operator->()` in `QualType`.


https://reviews.llvm.org/D37478