[PATCH] D12251: Analyzer: Calculate field offset correctly

Devin Coughlin via cfe-commits cfe-commits at lists.llvm.org
Fri Sep 25 18:35:43 PDT 2015

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 @@
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.


More information about the cfe-commits mailing list