[PATCH] D66593: [analyzer] CastValueChecker: Fix some assertions

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D66593#1642253 , @Charusso wrote:

> Wow, it is great you could address every of the edge cases. Thanks you so 
> much! I believe only one problem left: we need to `return false` instead of 
> plain `return` in order to let the Analyzer model the function call. I could 
> fix it easily, thanks!


Well, not really. I have tested out and there is no difference. Thanks for the 
fixes!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66593/new/

https://reviews.llvm.org/D66593



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

[PATCH] D66593: [analyzer] CastValueChecker: Fix some assertions

[Csaba Dabis via Phabricator via cfe-commits]

Charusso abandoned this revision.
Charusso marked 4 inline comments as done.
Charusso added a comment.

Wow, it is great you could address every of the edge cases. Thanks you so much! 
I believe only one problem left: we need to `return false` instead of plain 
`return` in order to let the Analyzer model the function call. I could fix it 
easily, thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66593/new/

https://reviews.llvm.org/D66593



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

[PATCH] D66593: [analyzer] CastValueChecker: Fix some assertions

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Pushed my own emergency hotfixes as rC369726 
, rC369727 
, rC369728 
, rC369729 
; reduced tests included.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66593/new/

https://reviews.llvm.org/D66593



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

[PATCH] D66593: [analyzer] CastValueChecker: Fix some assertions

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:743-750
+  QualType RegionType = DynType.getType();
+  if (RegionType->isPointerType())
+RegionType = RegionType->getPointeeType();
+  else
+RegionType = RegionType.getNonReferenceType();
+
+  assert(!RegionType.isNull() &&

NoQ wrote:
> I don't think this does anything:
> ```lang=c++
>505 QualType Type::getPointeeType() const {
>506   if (const auto *PT = getAs())
>507 return PT->getPointeeType();
>508   if (const auto *OPT = getAs())
>509 return OPT->getPointeeType();
>510   if (const auto *BPT = getAs())
>511 return BPT->getPointeeType();
>512   if (const auto *RT = getAs())
>513 return RT->getPointeeType();
>514   if (const auto *MPT = getAs())
>515 return MPT->getPointeeType();
>516   if (const auto *DT = getAs())
>517 return DT->getPointeeType();
>518   return {};
>519 }
> ```
> This getter usually works very reliably for both pointers and references.
I have measured each assertion failure one-by-one, so all of the hotfixes are 
necessary in order to reduce the crash-counter to zero. I have not got time to 
go in-depth, and have a great talk about them, sorry.



Comment at: clang/lib/StaticAnalyzer/Core/DynamicType.cpp:118-122
+  for (const auto  : Map) {
+const MemRegion *MR = Elem.first;
+if (MR && !SR.isLiveRegion(MR))
+  State = State->remove(MR);
+  }

NoQ wrote:
> We shouldn't put null regions into our maps.
Yes, but that is problematic. I have updated the diff.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66593/new/

https://reviews.llvm.org/D66593



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

[PATCH] D66593: [analyzer] CastValueChecker: Fix some assertions

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 216737.
Charusso marked 2 inline comments as done.
Charusso added a comment.

- The null `MemRegion` is a `0 (Loc)`, try to avoid to store it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66593/new/

https://reviews.llvm.org/D66593

Files:
  clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/test/Analysis/cast-value-notes.cpp

Index: clang/test/Analysis/cast-value-notes.cpp
===
--- clang/test/Analysis/cast-value-notes.cpp
+++ clang/test/Analysis/cast-value-notes.cpp
@@ -85,13 +85,12 @@
 
 void evalNonNullParamNullReturn(const Shape *S) {
   const auto *C = dyn_cast_or_null(S);
-  // expected-note@-1 {{Assuming 'S' is not a 'Circle'}}
+  // expected-note@-1 {{Assuming null pointer is passed into cast}}
 
   if (const auto *T = dyn_cast_or_null(S)) {
-// expected-note@-1 {{Assuming 'S' is a 'Triangle'}}
-// expected-note@-2 {{'T' initialized here}}
-// expected-note@-3 {{'T' is non-null}}
-// expected-note@-4 {{Taking true branch}}
+// expected-note@-1 {{'T' initialized here}}
+// expected-note@-2 {{'T' is non-null}}
+// expected-note@-3 {{Taking true branch}}
 
 (void)(1 / !T);
 // expected-note@-1 {{'T' is non-null}}
Index: clang/lib/StaticAnalyzer/Core/MemRegion.cpp
===
--- clang/lib/StaticAnalyzer/Core/MemRegion.cpp
+++ clang/lib/StaticAnalyzer/Core/MemRegion.cpp
@@ -1048,7 +1048,7 @@
   return getSubRegion(E, getStackLocalsRegion(SFC));
 }
 
-/// Checks whether \p BaseClass is a valid virtual or direct non-virtual base
+/*/// Checks whether \p BaseClass is a valid virtual or direct non-virtual base
 /// class of the type of \p Super.
 static bool isValidBaseClass(const CXXRecordDecl *BaseClass,
  const TypedValueRegion *Super,
@@ -1068,15 +1068,16 @@
   }
 
   return false;
-}
+}*/
 
 const CXXBaseObjectRegion *
 MemRegionManager::getCXXBaseObjectRegion(const CXXRecordDecl *RD,
  const SubRegion *Super,
  bool IsVirtual) {
   if (isa(Super)) {
-assert(isValidBaseClass(RD, dyn_cast(Super), IsVirtual));
-(void)
+// FIXME: Make this assertion great again.
+/*assert(isValidBaseClass(RD, dyn_cast(Super), IsVirtual));
+(void)*/
 
 if (IsVirtual) {
   // Virtual base regions should not be layered, since the layout rules
Index: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -325,13 +325,15 @@
   return State;
 }
 Result = InitWithAdjustments;
-  } else {
+  }
+  // FIXME: Make this assertion great again.
+  /* else {
 // We need to create a region no matter what. For sanity, make sure we don't
 // try to stuff a Loc into a non-pointer temporary region.
 assert(!InitValWithAdjustments.getAs() ||
Loc::isLocType(Result->getType()) ||
Result->getType()->isMemberPointerType());
-  }
+  }*/
 
   ProgramStateManager  = State->getStateManager();
   MemRegionManager  = StateMgr.getRegionManager();
Index: clang/lib/StaticAnalyzer/Core/CallEvent.cpp
===
--- clang/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ clang/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -740,8 +740,14 @@
 return {};
 
   // Is the type a C++ class? (This is mostly a defensive check.)
-  QualType RegionType = DynType.getType()->getPointeeType();
-  assert(!RegionType.isNull() && "DynamicTypeInfo should always be a pointer.");
+  QualType RegionType = DynType.getType();
+  if (RegionType->isPointerType())
+RegionType = RegionType->getPointeeType();
+  else
+RegionType = RegionType.getNonReferenceType();
+
+  assert(!RegionType.isNull() &&
+ "DynamicTypeInfo should always be a pointer or a reference.");
 
   const CXXRecordDecl *RD = RegionType->getAsCXXRecordDecl();
   if (!RD || !RD->hasDefinition())
Index: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
@@ -98,8 +98,7 @@
 
   if (Ty->isPointerType())
 Ty = Ty->getPointeeType();
-
-  if (Ty->isReferenceType())
+  else if (Ty->isReferenceType())
 Ty = Ty.getNonReferenceType();
 
   return Ty.getUnqualifiedType();
@@ -411,6 +410,9 @@
   if (!DV)
 return false;
 
+  if (!DV->getAsRegion())
+return false;
+
   Check(this, Call, *DV, C);
   return true;
 }
___
cfe-commits mailing 

[PATCH] D66593: [analyzer] CastValueChecker: Fix some assertions

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

These assertions are fundamental, so we can't remove them; i believe we messed 
up modeling at some point. I'll pick this up to address some nasty regressions 
quickly; i managed to reproduce the crashes and i already have 4 creduces 
running.




Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:743-750
+  QualType RegionType = DynType.getType();
+  if (RegionType->isPointerType())
+RegionType = RegionType->getPointeeType();
+  else
+RegionType = RegionType.getNonReferenceType();
+
+  assert(!RegionType.isNull() &&

I don't think this does anything:
```lang=c++
   505 QualType Type::getPointeeType() const {
   506   if (const auto *PT = getAs())
   507 return PT->getPointeeType();
   508   if (const auto *OPT = getAs())
   509 return OPT->getPointeeType();
   510   if (const auto *BPT = getAs())
   511 return BPT->getPointeeType();
   512   if (const auto *RT = getAs())
   513 return RT->getPointeeType();
   514   if (const auto *MPT = getAs())
   515 return MPT->getPointeeType();
   516   if (const auto *DT = getAs())
   517 return DT->getPointeeType();
   518   return {};
   519 }
```
This getter usually works very reliably for both pointers and references.



Comment at: clang/lib/StaticAnalyzer/Core/DynamicType.cpp:118-122
+  for (const auto  : Map) {
+const MemRegion *MR = Elem.first;
+if (MR && !SR.isLiveRegion(MR))
+  State = State->remove(MR);
+  }

We shouldn't put null regions into our maps.



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:329-336
+  // FIXME: Make this assertion great again.
+  /* else {
 // We need to create a region no matter what. For sanity, make sure we 
don't
 // try to stuff a Loc into a non-pointer temporary region.
 assert(!InitValWithAdjustments.getAs() ||
Loc::isLocType(Result->getType()) ||
Result->getType()->isMemberPointerType());

This usually fails when we mess up lvalue/rvalue vs. loc/nonloc invariants in 
our modeling.



Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1079-1080
+// FIXME: Make this assertion great again.
+/*assert(isValidBaseClass(RD, dyn_cast(Super), 
IsVirtual));
+(void)*/
 

Ugh, i suspect that we can't pass through the original pointer in our cast 
modeling; we need to actually model pointer casts, which is annoying but 
necessary, given that the cast doesn't necessarily yield the same pointer value 
even in run-time (see multiple inheritance).


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66593/new/

https://reviews.llvm.org/D66593



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

[PATCH] D66593: [analyzer] CastValueChecker: Fix some assertions

[Csaba Dabis via Phabricator via cfe-commits]

Charusso marked an inline comment as done.
Charusso added a comment.

I am not sure how would I fix them, so I just commented them out.




Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:102
+  else if (Ty->isReferenceType())
 Ty = Ty.getNonReferenceType();
 

May it is more explicit.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66593/new/

https://reviews.llvm.org/D66593



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

[PATCH] D66593: [analyzer] CastValueChecker: Fix some assertions

[Csaba Dabis via Phabricator via cfe-commits]

Charusso created this revision.
Charusso added a reviewer: NoQ.
Charusso added a project: clang.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, Szelethus, 
mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.

-


Repository:
  rC Clang

https://reviews.llvm.org/D66593

Files:
  clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/DynamicType.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp

Index: clang/lib/StaticAnalyzer/Core/MemRegion.cpp
===
--- clang/lib/StaticAnalyzer/Core/MemRegion.cpp
+++ clang/lib/StaticAnalyzer/Core/MemRegion.cpp
@@ -1048,7 +1048,7 @@
   return getSubRegion(E, getStackLocalsRegion(SFC));
 }
 
-/// Checks whether \p BaseClass is a valid virtual or direct non-virtual base
+/*/// Checks whether \p BaseClass is a valid virtual or direct non-virtual base
 /// class of the type of \p Super.
 static bool isValidBaseClass(const CXXRecordDecl *BaseClass,
  const TypedValueRegion *Super,
@@ -1068,15 +1068,16 @@
   }
 
   return false;
-}
+}*/
 
 const CXXBaseObjectRegion *
 MemRegionManager::getCXXBaseObjectRegion(const CXXRecordDecl *RD,
  const SubRegion *Super,
  bool IsVirtual) {
   if (isa(Super)) {
-assert(isValidBaseClass(RD, dyn_cast(Super), IsVirtual));
-(void)
+// FIXME: Make this assertion great again.
+/*assert(isValidBaseClass(RD, dyn_cast(Super), IsVirtual));
+(void)*/
 
 if (IsVirtual) {
   // Virtual base regions should not be layered, since the layout rules
Index: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -325,13 +325,15 @@
   return State;
 }
 Result = InitWithAdjustments;
-  } else {
+  }
+  // FIXME: Make this assertion great again.
+  /* else {
 // We need to create a region no matter what. For sanity, make sure we don't
 // try to stuff a Loc into a non-pointer temporary region.
 assert(!InitValWithAdjustments.getAs() ||
Loc::isLocType(Result->getType()) ||
Result->getType()->isMemberPointerType());
-  }
+  }*/
 
   ProgramStateManager  = State->getStateManager();
   MemRegionManager  = StateMgr.getRegionManager();
Index: clang/lib/StaticAnalyzer/Core/DynamicType.cpp
===
--- clang/lib/StaticAnalyzer/Core/DynamicType.cpp
+++ clang/lib/StaticAnalyzer/Core/DynamicType.cpp
@@ -115,9 +115,11 @@
 template 
 ProgramStateRef removeDead(ProgramStateRef State, const MapTy ,
SymbolReaper ) {
-  for (const auto  : Map)
-if (!SR.isLiveRegion(Elem.first))
-  State = State->remove(Elem.first);
+  for (const auto  : Map) {
+const MemRegion *MR = Elem.first;
+if (MR && !SR.isLiveRegion(MR))
+  State = State->remove(MR);
+  }
 
   return State;
 }
Index: clang/lib/StaticAnalyzer/Core/CallEvent.cpp
===
--- clang/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ clang/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -740,8 +740,14 @@
 return {};
 
   // Is the type a C++ class? (This is mostly a defensive check.)
-  QualType RegionType = DynType.getType()->getPointeeType();
-  assert(!RegionType.isNull() && "DynamicTypeInfo should always be a pointer.");
+  QualType RegionType = DynType.getType();
+  if (RegionType->isPointerType())
+RegionType = RegionType->getPointeeType();
+  else
+RegionType = RegionType.getNonReferenceType();
+
+  assert(!RegionType.isNull() &&
+ "DynamicTypeInfo should always be a pointer or a reference.");
 
   const CXXRecordDecl *RD = RegionType->getAsCXXRecordDecl();
   if (!RD || !RD->hasDefinition())
Index: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
@@ -98,8 +98,7 @@
 
   if (Ty->isPointerType())
 Ty = Ty->getPointeeType();
-
-  if (Ty->isReferenceType())
+  else if (Ty->isReferenceType())
 Ty = Ty.getNonReferenceType();
 
   return Ty.getUnqualifiedType();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits