Analyzer: Calculate field offset correctly

[ismailp]

Summary

StoreManager::getLValueFieldOrIvar should return loc as
base + field-offset, instead of just base.

[ismailp]

Ping!

[ismailp]

Ping!

[dcoughlin]

Thanks for the patch Ismail! Some comments inline.

lib/StaticAnalyzer/Core/Store.cpp

408

You can use auto *FD = dyn_cast<const FieldDecl>(D) here to avoid repeating the type.

410

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;

test/Analysis/array-struct-region.cpp

128

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.

[ismailp]

• Don't try to calculate field offset for Objective-C instance variables
• Added test for Objective-C instance variables
• Added a non-null pointer in test

[ismailp]

test/Analysis/array-struct-region.cpp

128

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.

[dcoughlin]

test/Analysis/array-struct-region.cpp

128

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.

lib/StaticAnalyzer/Core/Store.cpp

 //  This file defined the types Store and StoreManager.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/StaticAnalyzer/Core/PathSensitive/Store.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/CharUnits.h"
 #include "clang/AST/DeclObjC.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 
 using namespace clang;
 using namespace ento;
 
 StoreManager::StoreManager(ProgramStateManager &stateMgr)
   : svalBuilder(stateMgr.getSValBuilder()), StateMgr(stateMgr),
SVal StoreManager::getLValueFieldOrIvar(const Decl *D, SVal Base) {
   case loc::MemRegionKind:
     BaseR = BaseL.castAs<loc::MemRegionVal>().getRegion();
     break;
 
   case loc::GotoLabelKind:
     // These are anormal cases. Flag an undefined value.
     return UndefinedVal();
 
-  case loc::ConcreteIntKind:
-    // While these seem funny, this can happen through casts.
-    // FIXME: What we should return is the field offset.  For example,
-    //  add the field offset to the integer value.  That way funny things
-    //  like this work properly:  &(((struct foo *) 0xa)->f)
+  case loc::ConcreteIntKind: {
+    // Don't allow field offset calculations, if base is null.
+    if (!Base.isZeroConstant()) {
+      if (const auto *FD = dyn_cast<const FieldDecl>(D)) {
+        if (FD->getKind() != Decl::ObjCIvar) {
+          ASTContext &Ctx = D->getASTContext();
+          CharUnits CU = Ctx.toCharUnitsFromBits(Ctx.getFieldOffset(FD));
+          loc::ConcreteInt BasePtr = BaseL.castAs<loc::ConcreteInt>();
+          llvm::APSInt Offset(Ctx.getTypeSize(Ctx.VoidPtrTy));
+          Offset = CU.getQuantity();
+          Offset += BasePtr.getValue();
+          return svalBuilder.makeIntLocVal(Offset);
+        }
+      }
+    }
     return Base;
+  }
 
   default:
     llvm_unreachable("Unhandled Base.");
   }
 
   // NOTE: We must have this check first because ObjCIvarDecl is a subclass
   // of FieldDecl.
   if (const ObjCIvarDecl *ID = dyn_cast<ObjCIvarDecl>(D))

test/Analysis/array-struct-region.cpp

 
 #if __cplusplus
   clang_analyzer_eval(((getS())).check()); // expected-warning{{TRUE}}
   clang_analyzer_eval(!((getS()))); // expected-warning{{FALSE}}
   clang_analyzer_eval(~((getS()))); // expected-warning{{FALSE}}
 #endif
 }
 
+struct FieldOffset {
+  int firstField;
+  char secondField[63];
+  void *thirdField;
+};
+
+void testFieldOffsets() {
+  struct FieldOffset FO;
+  struct FieldOffset *PFONull = 0;
+  struct FieldOffset *PFOConstant = (struct FieldOffset *) 0x22;
+#if __cplusplus
+  struct FieldOffset *PFONew = new struct FieldOffset;
+#endif
+  clang_analyzer_eval((void *)&FO.secondField != (void*)&FO); // expected-warning{{TRUE}}
+  clang_analyzer_eval((void *)&FO.secondField != (void*)&FO.thirdField); // expected-warning{{TRUE}}
+  clang_analyzer_eval((void *)&PFONull->secondField != (void *)&PFONull->thirdField); // expected-warning{{FALSE}}
+  clang_analyzer_eval((void *)&PFONull->secondField == (void *)0); // expected-warning{{TRUE}}
+  clang_analyzer_eval((void *)&PFOConstant->secondField != (void*)PFOConstant); // expected-warning{{TRUE}}
+#if __cplusplus
+  clang_analyzer_eval((void *)&PFONew->secondField != (void *)&PFONew); // expected-warning{{TRUE}}
+#endif
+}
 
 //--------------------
 // C++-only tests
 //--------------------
 
 #if __cplusplus
 void testReferenceAssignment() {
   const S &s = getS();

test/Analysis/ivars.m

 
 
 int testNull(Root *obj) {
   if (obj) return 0;
 
   int *x = &obj->uniqueID;
   return *x; // expected-warning{{Dereference of null pointer (loaded from variable 'x')}}
 }
+
+void testFieldOffset() {
+  int *v = &((Root *)0x10)->uniqueID;
+  (void)v;
+}