subject:"\[PATCH\] D12251\: Analyzer\: Calculate field offset correctly"

Re: [PATCH] D12251: Analyzer: Calculate field offset correctly

2015-09-30 Thread 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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12251: Analyzer: Calculate field offset correctly

2015-09-30 Thread Ismail Pazarbasi via cfe-commits

ismailp marked 3 inline comments as done.


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

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.


http://reviews.llvm.org/D12251



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12251: Analyzer: Calculate field offset correctly

2015-09-30 Thread Ismail Pazarbasi via cfe-commits

ismailp updated this revision to Diff 36123.
ismailp added a comment.

- 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


http://reviews.llvm.org/D12251

Files:
  lib/StaticAnalyzer/Core/Store.cpp
  test/Analysis/array-struct-region.cpp
  test/Analysis/ivars.m

Index: test/Analysis/ivars.m
===
--- test/Analysis/ivars.m
+++ test/Analysis/ivars.m
@@ -138,3 +138,8 @@
   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;
+}
Index: test/Analysis/array-struct-region.cpp
===
--- test/Analysis/array-struct-region.cpp
+++ test/Analysis/array-struct-region.cpp
@@ -106,6 +106,28 @@
 #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
Index: lib/StaticAnalyzer/Core/Store.cpp
===
--- lib/StaticAnalyzer/Core/Store.cpp
+++ lib/StaticAnalyzer/Core/Store.cpp
@@ -15,6 +15,7 @@
 #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"
 
@@ -401,12 +402,23 @@
 // 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(D)) {
+if (FD->getKind() != Decl::ObjCIvar) {
+  ASTContext &Ctx = D->getASTContext();
+  CharUnits CU = Ctx.toCharUnitsFromBits(Ctx.getFieldOffset(FD));
+  loc::ConcreteInt BasePtr = BaseL.castAs();
+  llvm::APSInt Offset(Ctx.getTypeSize(Ctx.VoidPtrTy));
+  Offset = CU.getQuantity();
+  Offset += BasePtr.getValue();
+  return svalBuilder.makeIntLocVal(Offset);
+}
+  }
+}
 return Base;
+  }
 
   default:
 llvm_unreachable("Unhandled Base.");


Index: test/Analysis/ivars.m
===
--- test/Analysis/ivars.m
+++ test/Analysis/ivars.m
@@ -138,3 +138,8 @@
   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;
+}
Index: test/Analysis/array-struct-region.cpp
===
--- test/Analysis/array-struct-region.cpp
+++ test/Analysis/array-struct-region.cpp
@@ -106,6 +106,28 @@
 #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-

Re: [PATCH] D12251: Analyzer: Calculate field offset correctly

2015-09-25 Thread 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(D)) {
+ASTContext &Ctx = D->getASTContext();

You can use `auto *FD = dyn_cast(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();

`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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12251: Analyzer: Calculate field offset correctly

2015-09-22 Thread Ismail Pazarbasi via cfe-commits

ismailp added a comment.

Ping!


http://reviews.llvm.org/D12251



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12251: Analyzer: Calculate field offset correctly

2015-09-11 Thread Ismail Pazarbasi via cfe-commits

ismailp added a comment.

Ping!


http://reviews.llvm.org/D12251



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D12251: Analyzer: Calculate field offset correctly

2015-08-21 Thread Ismail Pazarbasi via cfe-commits

ismailp created this revision.
ismailp added reviewers: krememek, zaks.anna.
ismailp added a subscriber: cfe-commits.

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

http://reviews.llvm.org/D12251

Files:
  lib/StaticAnalyzer/Core/Store.cpp
  test/Analysis/array-struct-region.cpp

Index: test/Analysis/array-struct-region.cpp
===
--- test/Analysis/array-struct-region.cpp
+++ test/Analysis/array-struct-region.cpp
@@ -106,6 +106,26 @@
 #endif
 }
 
+struct FieldOffset {
+  int firstField;
+  char secondField[63];
+  void *thirdField;
+};
+
+void testFieldOffsets() {
+  struct FieldOffset FO;
+  struct FieldOffset *PFONull = 0;
+#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}}
+#if __cplusplus
+  clang_analyzer_eval((void *)&PFONew->secondField != (void *)&PFONew); // 
expected-warning{{TRUE}}
+#endif
+}
 
 //
 // C++-only tests
Index: lib/StaticAnalyzer/Core/Store.cpp
===
--- lib/StaticAnalyzer/Core/Store.cpp
+++ lib/StaticAnalyzer/Core/Store.cpp
@@ -15,6 +15,7 @@
 #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"
 
@@ -401,12 +402,21 @@
 // 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 FieldDecl *FD = dyn_cast(D)) {
+ASTContext &Ctx = D->getASTContext();
+CharUnits CU = Ctx.toCharUnitsFromBits(Ctx.getFieldOffset(FD));
+loc::ConcreteInt BasePtr = BaseL.castAs();
+llvm::APSInt Offset(Ctx.getTypeSize(Ctx.VoidPtrTy));
+Offset = CU.getQuantity();
+Offset += BasePtr.getValue();
+return svalBuilder.makeIntLocVal(Offset);
+  }
+}
 return Base;
+  }
 
   default:
 llvm_unreachable("Unhandled Base.");


Index: test/Analysis/array-struct-region.cpp
===
--- test/Analysis/array-struct-region.cpp
+++ test/Analysis/array-struct-region.cpp
@@ -106,6 +106,26 @@
 #endif
 }
 
+struct FieldOffset {
+  int firstField;
+  char secondField[63];
+  void *thirdField;
+};
+
+void testFieldOffsets() {
+  struct FieldOffset FO;
+  struct FieldOffset *PFONull = 0;
+#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}}
+#if __cplusplus
+  clang_analyzer_eval((void *)&PFONew->secondField != (void *)&PFONew); // expected-warning{{TRUE}}
+#endif
+}
 
 //
 // C++-only tests
Index: lib/StaticAnalyzer/Core/Store.cpp
===
--- lib/StaticAnalyzer/Core/Store.cpp
+++ lib/StaticAnalyzer/Core/Store.cpp
@@ -15,6 +15,7 @@
 #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"
 
@@ -401,12 +402,21 @@
 // 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 a

Re: [PATCH] D12251: Analyzer: Calculate field offset correctly

Re: [PATCH] D12251: Analyzer: Calculate field offset correctly

Re: [PATCH] D12251: Analyzer: Calculate field offset correctly

Re: [PATCH] D12251: Analyzer: Calculate field offset correctly

Re: [PATCH] D12251: Analyzer: Calculate field offset correctly

Re: [PATCH] D12251: Analyzer: Calculate field offset correctly

[PATCH] D12251: Analyzer: Calculate field offset correctly

7 matches

Site Navigation

Mail list logo

Footer information