[PATCH] D12251: Analyzer: Calculate field offset correctly

[Devin Coughlin via cfe-commits]

dcoughlin requested changes to this revision.
dcoughlin added a comment.
This revision now requires changes to proceed.

Thanks for the patch Ismail! Some comments inline.


================
Comment at: lib/StaticAnalyzer/Core/Store.cpp:408
@@ +407,3 @@
+    if (!Base.isZeroConstant()) {
+      if (const FieldDecl *FD = dyn_cast<const FieldDecl>(D)) {
+        ASTContext &Ctx = D->getASTContext();
----------------
You can use `auto *FD = dyn_cast<const FieldDecl>(D)` here to avoid repeating the type.

================
Comment at: lib/StaticAnalyzer/Core/Store.cpp:410
@@ +409,3 @@
+        ASTContext &Ctx = D->getASTContext();
+        CharUnits CU = Ctx.toCharUnitsFromBits(Ctx.getFieldOffset(FD));
+        loc::ConcreteInt BasePtr = BaseL.castAs<loc::ConcreteInt>();
----------------
`getFieldOffset()` will assert when the field is an Objective-C instance variable because it expects the field's parent to be a record. For example: `int *x = &((SomeClass *)0x10)->someIVar;`

================
Comment at: test/Analysis/array-struct-region.cpp:128
@@ -109,1 +127,3 @@
+#endif
+}
 
----------------
This test doesn't cover the new logic you added in Store.cpp because it is never the case in this test that the base is a non-zero ConcreteInt Loc. In addition to your test with `PFONull`, you should add tests with something like `struct FieldOffset *PFOConstant = (struct FieldOffset *) 0x22;`. I think it is also important to add tests for Objective-C instance variables after addressing the assertion failure in that case.


http://reviews.llvm.org/D12251