[PATCH] D12251: Analyzer: Calculate field offset correctly

[Devin Coughlin via cfe-commits]

dcoughlin added inline comments.

================
Comment at: test/Analysis/array-struct-region.cpp:128
@@ +127,3 @@
+#if __cplusplus
+  clang_analyzer_eval((void *)&PFONew->secondField != (void *)&PFONew); // expected-warning{{TRUE}}
+#endif
----------------
ismailp wrote:
> I might be missing something, and would be very happy if you could explain why it is necessary to add `PFOConstant`.
> 
> At line 122, for example, where `&FO` is always non-null. Likewise, I'd expect line 128 to be non-null, because `new` in this translation unit either throws -- in which case, we shouldn't be executing this line -- or succeeds -- in which case, `PFONew` is non-null.
Sorry I wasn't clear. The branch of the case that you modified here only executes when the base is an int with a known value, such as '0xa' (see the code comment with the FIXME you removed.) This means that your test with `PFONew` doesn't exercise your new code at all. The `PFONull` case does exercise this case branch (because NULL is the 0 concreteInt) but it does not exercise your newly added logic for calculating field offsets (because for `NULL`, `Base.isZeroConstant()` is true). This means there were no tests exercising that logic (try removing the logic and adding `assert(false)` -- your old tests would still pass).

Separately, as I mentioned before, I think it is important to leave a FIXME here documenting that the `NULL` case still doesn't work properly. That is, even with your changes, the analyzer still incorrectly says that `&(((struct foo *) NULL)->f == 0`. (Fixing this would be quite an undertaking.)  It would probably also be good to add comments to the tests saying the behavior they expect for `PFONull` is incorrect so that if we ever fix this issue we will know it is safe to update the tests:

```
  clang_analyzer_eval((void *)&PFONull->secondField != (void *)&PFONull->thirdField); // expected-warning{{FALSE}}
  clang_analyzer_eval((void *)&PFONull->secondField == (void *)0); // expected-warning{{TRUE}}

```
(If we handled `NULL` properly, the first would be TRUE and second would be FALSE.)

Note that your updated version doesn't calculate field offsets for ObjC ivars with concreteInt bases, so the analyzer also still won't properly say `&((Root *)0x10)->uniqueID != (Root *)0x10)`. Please add it to the FIXME comment, as well.


http://reviews.llvm.org/D12251