[PATCH] D69599: [analyzer] DynamicSize: Remove 'getSizeInElements()' from store

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.
Herald added subscribers: martong, steakhal.

@Charusso 
I think this patch may fix this bug https://bugs.llvm.org/show_bug.cgi?id=25284
Could you please verify and close it if so?
At least I couldn't reproduce it on the latest build.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69599



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

[PATCH] D69599: [analyzer] DynamicSize: Remove 'getSizeInElements()' from store

[Csaba Dabis via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGaf3d0d16286a: [analyzer] DynamicSize: Remove 
getSizeInElements() from store (authored by Charusso).

Changed prior to commit:
  https://reviews.llvm.org/D69599?vs=227546=241460#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69599

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
  clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp

Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -623,15 +623,6 @@
   SymbolReaper& SymReaper) override;
 
   //===--===//
-  // Region "extents".
-  //===--===//
-
-  // FIXME: This method will soon be eliminated; see the note in Store.h.
-  DefinedOrUnknownSVal getSizeInElements(ProgramStateRef state,
- const MemRegion* R,
- QualType EleTy) override;
-
-  //===--===//
   // Utility methods.
   //===--===//
 
@@ -1388,37 +1379,6 @@
 }
 
 //===--===//
-// Extents for regions.
-//===--===//
-
-DefinedOrUnknownSVal
-RegionStoreManager::getSizeInElements(ProgramStateRef state,
-  const MemRegion *R,
-  QualType EleTy) {
-  DefinedOrUnknownSVal Size = getDynamicSize(state, R, svalBuilder);
-  const llvm::APSInt *SizeInt = svalBuilder.getKnownValue(state, Size);
-  if (!SizeInt)
-return UnknownVal();
-
-  CharUnits RegionSize = CharUnits::fromQuantity(SizeInt->getSExtValue());
-
-  if (Ctx.getAsVariableArrayType(EleTy)) {
-// FIXME: We need to track extra state to properly record the size
-// of VLAs.  Returning UnknownVal here, however, is a stop-gap so that
-// we don't have a divide-by-zero below.
-return UnknownVal();
-  }
-
-  CharUnits EleSize = Ctx.getTypeSizeInChars(EleTy);
-
-  // If a variable is reinterpreted as a type that doesn't fit into a larger
-  // type evenly, round it down.
-  // This is a signed value, since it's used in arithmetic with signed indices.
-  return svalBuilder.makeIntVal(RegionSize / EleSize,
-svalBuilder.getArrayIndexType());
-}
-
-//===--===//
 // Location and region casting.
 //===--===//
 
Index: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
===
--- clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
+++ clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
@@ -11,6 +11,7 @@
 //===--===//
 
 #include "clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h"
+#include "clang/AST/Expr.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
@@ -26,5 +27,22 @@
   return MR->getMemRegionManager().getStaticSize(MR, SVB);
 }
 
+DefinedOrUnknownSVal getDynamicElementCount(ProgramStateRef State,
+const MemRegion *MR,
+SValBuilder ,
+QualType ElementTy) {
+  MemRegionManager  = MR->getMemRegionManager();
+  ASTContext  = MemMgr.getContext();
+
+  DefinedOrUnknownSVal Size = getDynamicSize(State, MR, SVB);
+  SVal ElementSizeV = SVB.makeIntVal(
+  Ctx.getTypeSizeInChars(ElementTy).getQuantity(), SVB.getArrayIndexType());
+
+  SVal DivisionV =
+  SVB.evalBinOp(State, BO_Div, Size, ElementSizeV, SVB.getArrayIndexType());
+
+  return DivisionV.castAs();
+}
+
 } // namespace ento
 } // namespace clang
Index: clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
+++ 

[PATCH] D69599: [analyzer] DynamicSize: Remove 'getSizeInElements()' from store

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Thanks for the review!




Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:40-48
+  if (const llvm::APSInt *SizeInt = SVB.getKnownValue(State, Size)) {
+CharUnits RegionSize = CharUnits::fromQuantity(SizeInt->getSExtValue());
+
+// If a variable is reinterpreted as a type that doesn't fit into a larger
+// type evenly, round it down.
+// This is a signed value, since it's used in arithmetic with signed
+// indices.

NoQ wrote:
> Charusso wrote:
> > NoQ wrote:
> > > Charusso wrote:
> > > > NoQ wrote:
> > > > > And then remove the manual division.
> > > > Hmpf.
> > > > 
> > > > ```
> > > > Failing Tests (7):
> > > > Clang :: Analysis/misc-ps-region-store.m
> > > > Clang :: Analysis/mpichecker.cpp
> > > > Clang :: Analysis/outofbound.c
> > > > Clang :: Analysis/rdar-6541136-region.c
> > > > Clang :: Analysis/return-ptr-range.cpp
> > > > Clang :: Analysis/track-control-dependency-conditions.cpp
> > > > Clang :: Analysis/uninit-vals.c
> > > > ```
> > > > 
> > > > I would pick that solution because it may be a tiny-bit faster, and 
> > > > then later on investigate this issue when we model more about dynamic 
> > > > sizes.
> > > Soo what does it tell us about the correctness of the new 
> > > `evalBinOp`-based solution?
> > So, when I tried to inject an `APSInt` it converted to `0` so division by 
> > zero made that. I felt that the implicit conversion is wonky, but dividing 
> > by 0, ugh.
> Yay, great job figuring this out!
> 
> Also the conversion wasn't implicit; you explicitly specified 
> `llvm::APSInt(...)`. I agree that this constructor is evil, though.
`getQuantity()` retuns a `QuantityType`, but now I see: `typedef int64_t 
QuantityType`, so I was fooled.


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

https://reviews.llvm.org/D69599



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

[PATCH] D69599: [analyzer] DynamicSize: Remove 'getSizeInElements()' from store

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 227546.
Charusso marked 3 inline comments as done.
Charusso added a comment.

- Done.


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

https://reviews.llvm.org/D69599

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
  clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp

Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -622,15 +622,6 @@
   StoreRef removeDeadBindings(Store store, const StackFrameContext *LCtx,
   SymbolReaper& SymReaper) override;
 
-  //===--===//
-  // Region "extents".
-  //===--===//
-
-  // FIXME: This method will soon be eliminated; see the note in Store.h.
-  DefinedOrUnknownSVal getSizeInElements(ProgramStateRef state,
- const MemRegion* R,
- QualType EleTy) override;
-
   //===--===//
   // Utility methods.
   //===--===//
@@ -1387,37 +1378,6 @@
   return StoreRef(B.asStore(), *this);
 }
 
-//===--===//
-// Extents for regions.
-//===--===//
-
-DefinedOrUnknownSVal
-RegionStoreManager::getSizeInElements(ProgramStateRef state,
-  const MemRegion *R,
-  QualType EleTy) {
-  DefinedOrUnknownSVal Size = getDynamicSize(state, R, svalBuilder);
-  const llvm::APSInt *SizeInt = svalBuilder.getKnownValue(state, Size);
-  if (!SizeInt)
-return UnknownVal();
-
-  CharUnits RegionSize = CharUnits::fromQuantity(SizeInt->getSExtValue());
-
-  if (Ctx.getAsVariableArrayType(EleTy)) {
-// FIXME: We need to track extra state to properly record the size
-// of VLAs.  Returning UnknownVal here, however, is a stop-gap so that
-// we don't have a divide-by-zero below.
-return UnknownVal();
-  }
-
-  CharUnits EleSize = Ctx.getTypeSizeInChars(EleTy);
-
-  // If a variable is reinterpreted as a type that doesn't fit into a larger
-  // type evenly, round it down.
-  // This is a signed value, since it's used in arithmetic with signed indices.
-  return svalBuilder.makeIntVal(RegionSize / EleSize,
-svalBuilder.getArrayIndexType());
-}
-
 //===--===//
 // Location and region casting.
 //===--===//
Index: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
===
--- clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
+++ clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
@@ -11,6 +11,7 @@
 //===--===//
 
 #include "clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h"
+#include "clang/AST/Expr.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
@@ -26,5 +27,22 @@
   return MR->getMemRegionManager().getStaticSize(MR, SVB);
 }
 
+DefinedOrUnknownSVal getDynamicElementCount(ProgramStateRef State,
+const MemRegion *MR,
+SValBuilder ,
+QualType ElementTy) {
+  MemRegionManager  = MR->getMemRegionManager();
+  ASTContext  = MemMgr.getContext();
+
+  DefinedOrUnknownSVal Size = getDynamicSize(State, MR, SVB);
+  SVal ElementSizeV = SVB.makeIntVal(
+  Ctx.getTypeSizeInChars(ElementTy).getQuantity(), SVB.getArrayIndexType());
+
+  SVal DivisionV =
+  SVB.evalBinOp(State, BO_Div, Size, ElementSizeV, SVB.getArrayIndexType());
+
+  return DivisionV.castAs();
+}
+
 } // namespace ento
 } // namespace clang
Index: clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
@@ -16,6 +16,7 @@
 #include 

[PATCH] D69599: [analyzer] DynamicSize: Remove 'getSizeInElements()' from store

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:40-48
+  if (const llvm::APSInt *SizeInt = SVB.getKnownValue(State, Size)) {
+CharUnits RegionSize = CharUnits::fromQuantity(SizeInt->getSExtValue());
+
+// If a variable is reinterpreted as a type that doesn't fit into a larger
+// type evenly, round it down.
+// This is a signed value, since it's used in arithmetic with signed
+// indices.

Charusso wrote:
> NoQ wrote:
> > Charusso wrote:
> > > NoQ wrote:
> > > > And then remove the manual division.
> > > Hmpf.
> > > 
> > > ```
> > > Failing Tests (7):
> > > Clang :: Analysis/misc-ps-region-store.m
> > > Clang :: Analysis/mpichecker.cpp
> > > Clang :: Analysis/outofbound.c
> > > Clang :: Analysis/rdar-6541136-region.c
> > > Clang :: Analysis/return-ptr-range.cpp
> > > Clang :: Analysis/track-control-dependency-conditions.cpp
> > > Clang :: Analysis/uninit-vals.c
> > > ```
> > > 
> > > I would pick that solution because it may be a tiny-bit faster, and then 
> > > later on investigate this issue when we model more about dynamic sizes.
> > Soo what does it tell us about the correctness of the new 
> > `evalBinOp`-based solution?
> So, when I tried to inject an `APSInt` it converted to `0` so division by 
> zero made that. I felt that the implicit conversion is wonky, but dividing by 
> 0, ugh.
Yay, great job figuring this out!

Also the conversion wasn't implicit; you explicitly specified 
`llvm::APSInt(...)`. I agree that this constructor is evil, though.



Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:44-47
+  if (auto DV = DivisionV.getAs())
+return *DV;
+
+  return UnknownVal();

I'd rather do a `castAs` here. Allocating a region of garbage size should be an 
immediate warning; supplying a zero-size `ElementTy` should be an immediate 
crash; in all other cases the result of division must be defined.


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

https://reviews.llvm.org/D69599



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

[PATCH] D69599: [analyzer] DynamicSize: Remove 'getSizeInElements()' from store

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Thanks, now it is cool!




Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:40-48
+  if (const llvm::APSInt *SizeInt = SVB.getKnownValue(State, Size)) {
+CharUnits RegionSize = CharUnits::fromQuantity(SizeInt->getSExtValue());
+
+// If a variable is reinterpreted as a type that doesn't fit into a larger
+// type evenly, round it down.
+// This is a signed value, since it's used in arithmetic with signed
+// indices.

NoQ wrote:
> Charusso wrote:
> > NoQ wrote:
> > > And then remove the manual division.
> > Hmpf.
> > 
> > ```
> > Failing Tests (7):
> > Clang :: Analysis/misc-ps-region-store.m
> > Clang :: Analysis/mpichecker.cpp
> > Clang :: Analysis/outofbound.c
> > Clang :: Analysis/rdar-6541136-region.c
> > Clang :: Analysis/return-ptr-range.cpp
> > Clang :: Analysis/track-control-dependency-conditions.cpp
> > Clang :: Analysis/uninit-vals.c
> > ```
> > 
> > I would pick that solution because it may be a tiny-bit faster, and then 
> > later on investigate this issue when we model more about dynamic sizes.
> Soo what does it tell us about the correctness of the new 
> `evalBinOp`-based solution?
So, when I tried to inject an `APSInt` it converted to `0` so division by zero 
made that. I felt that the implicit conversion is wonky, but dividing by 0, ugh.


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

https://reviews.llvm.org/D69599



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

[PATCH] D69599: [analyzer] DynamicSize: Remove 'getSizeInElements()' from store

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 227543.
Charusso marked 4 inline comments as done.
Charusso added a comment.

- Old division swapped by `evalBinOp`.


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

https://reviews.llvm.org/D69599

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
  clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp

Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -622,15 +622,6 @@
   StoreRef removeDeadBindings(Store store, const StackFrameContext *LCtx,
   SymbolReaper& SymReaper) override;
 
-  //===--===//
-  // Region "extents".
-  //===--===//
-
-  // FIXME: This method will soon be eliminated; see the note in Store.h.
-  DefinedOrUnknownSVal getSizeInElements(ProgramStateRef state,
- const MemRegion* R,
- QualType EleTy) override;
-
   //===--===//
   // Utility methods.
   //===--===//
@@ -1387,37 +1378,6 @@
   return StoreRef(B.asStore(), *this);
 }
 
-//===--===//
-// Extents for regions.
-//===--===//
-
-DefinedOrUnknownSVal
-RegionStoreManager::getSizeInElements(ProgramStateRef state,
-  const MemRegion *R,
-  QualType EleTy) {
-  DefinedOrUnknownSVal Size = getDynamicSize(state, R, svalBuilder);
-  const llvm::APSInt *SizeInt = svalBuilder.getKnownValue(state, Size);
-  if (!SizeInt)
-return UnknownVal();
-
-  CharUnits RegionSize = CharUnits::fromQuantity(SizeInt->getSExtValue());
-
-  if (Ctx.getAsVariableArrayType(EleTy)) {
-// FIXME: We need to track extra state to properly record the size
-// of VLAs.  Returning UnknownVal here, however, is a stop-gap so that
-// we don't have a divide-by-zero below.
-return UnknownVal();
-  }
-
-  CharUnits EleSize = Ctx.getTypeSizeInChars(EleTy);
-
-  // If a variable is reinterpreted as a type that doesn't fit into a larger
-  // type evenly, round it down.
-  // This is a signed value, since it's used in arithmetic with signed indices.
-  return svalBuilder.makeIntVal(RegionSize / EleSize,
-svalBuilder.getArrayIndexType());
-}
-
 //===--===//
 // Location and region casting.
 //===--===//
Index: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
===
--- clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
+++ clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
@@ -11,6 +11,7 @@
 //===--===//
 
 #include "clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h"
+#include "clang/AST/Expr.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
@@ -26,5 +27,25 @@
   return MR->getMemRegionManager().getStaticSize(MR, SVB);
 }
 
+DefinedOrUnknownSVal getDynamicElementCount(ProgramStateRef State,
+const MemRegion *MR,
+SValBuilder ,
+QualType ElementTy) {
+  MemRegionManager  = MR->getMemRegionManager();
+  ASTContext  = MemMgr.getContext();
+
+  DefinedOrUnknownSVal Size = getDynamicSize(State, MR, SVB);
+  SVal ElementSizeV = SVB.makeIntVal(
+  Ctx.getTypeSizeInChars(ElementTy).getQuantity(), SVB.getArrayIndexType());
+
+  SVal DivisionV =
+  SVB.evalBinOp(State, BO_Div, Size, ElementSizeV, SVB.getArrayIndexType());
+
+  if (auto DV = DivisionV.getAs())
+return *DV;
+
+  return UnknownVal();
+}
+
 } // namespace ento
 } // namespace clang
Index: clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
+++ 

[PATCH] D69599: [analyzer] DynamicSize: Remove 'getSizeInElements()' from store

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:40-48
+  if (const llvm::APSInt *SizeInt = SVB.getKnownValue(State, Size)) {
+CharUnits RegionSize = CharUnits::fromQuantity(SizeInt->getSExtValue());
+
+// If a variable is reinterpreted as a type that doesn't fit into a larger
+// type evenly, round it down.
+// This is a signed value, since it's used in arithmetic with signed
+// indices.

Charusso wrote:
> NoQ wrote:
> > And then remove the manual division.
> Hmpf.
> 
> ```
> Failing Tests (7):
> Clang :: Analysis/misc-ps-region-store.m
> Clang :: Analysis/mpichecker.cpp
> Clang :: Analysis/outofbound.c
> Clang :: Analysis/rdar-6541136-region.c
> Clang :: Analysis/return-ptr-range.cpp
> Clang :: Analysis/track-control-dependency-conditions.cpp
> Clang :: Analysis/uninit-vals.c
> ```
> 
> I would pick that solution because it may be a tiny-bit faster, and then 
> later on investigate this issue when we model more about dynamic sizes.
Soo what does it tell us about the correctness of the new `evalBinOp`-based 
solution?


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

https://reviews.llvm.org/D69599



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

[PATCH] D69599: [analyzer] DynamicSize: Remove 'getSizeInElements()' from store

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:40-48
+  if (const llvm::APSInt *SizeInt = SVB.getKnownValue(State, Size)) {
+CharUnits RegionSize = CharUnits::fromQuantity(SizeInt->getSExtValue());
+
+// If a variable is reinterpreted as a type that doesn't fit into a larger
+// type evenly, round it down.
+// This is a signed value, since it's used in arithmetic with signed
+// indices.

NoQ wrote:
> And then remove the manual division.
Hmpf.

```
Failing Tests (7):
Clang :: Analysis/misc-ps-region-store.m
Clang :: Analysis/mpichecker.cpp
Clang :: Analysis/outofbound.c
Clang :: Analysis/rdar-6541136-region.c
Clang :: Analysis/return-ptr-range.cpp
Clang :: Analysis/track-control-dependency-conditions.cpp
Clang :: Analysis/uninit-vals.c
```

I would pick that solution because it may be a tiny-bit faster, and then later 
on investigate this issue when we model more about dynamic sizes.


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

https://reviews.llvm.org/D69599



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

[PATCH] D69599: [analyzer] DynamicSize: Remove 'getSizeInElements()' from store

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:40-48
+  if (const llvm::APSInt *SizeInt = SVB.getKnownValue(State, Size)) {
+CharUnits RegionSize = CharUnits::fromQuantity(SizeInt->getSExtValue());
+
+// If a variable is reinterpreted as a type that doesn't fit into a larger
+// type evenly, round it down.
+// This is a signed value, since it's used in arithmetic with signed
+// indices.

And then remove the manual division.


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

https://reviews.llvm.org/D69599



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

[PATCH] D69599: [analyzer] DynamicSize: Remove 'getSizeInElements()' from store

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D69599#1730707 , @NoQ wrote:

> > This is the first step to mitigate that issue.
>
> What's the issue?


Well, after I mentioned an issue I have realized the somewhat path-insensitive 
`getSizeInElements()` does not touch the (void *) return value. Basically the 
expression `int *foo = malloc()` could not compile, and I had felt that the 
assumptions about the overflow are wrong. Now I see that none of the overflow 
tests would compile, so I think we just bypass a cast here-and-there. Therefore 
there is no issue, just I was surprised.




Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:37-39
+  const llvm::APSInt *SizeInt = SVB.getKnownValue(State, Size);
+  if (!SizeInt)
+return UnknownVal();

NoQ wrote:
> Even if the size is not concrete, you can ask `SValBuilder` to perform the 
> division. It's going to be a symbolic expression which we won't be able to 
> work with anyway, but these days we believe that it's still worth it, in hope 
> that our constraint solver eventually gets better.
Good idea, thanks!


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

https://reviews.llvm.org/D69599



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

[PATCH] D69599: [analyzer] DynamicSize: Remove 'getSizeInElements()' from store

[Csaba Dabis via Phabricator via cfe-commits]

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

- Fix.


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

https://reviews.llvm.org/D69599

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
  clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp

Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -622,15 +622,6 @@
   StoreRef removeDeadBindings(Store store, const StackFrameContext *LCtx,
   SymbolReaper& SymReaper) override;
 
-  //===--===//
-  // Region "extents".
-  //===--===//
-
-  // FIXME: This method will soon be eliminated; see the note in Store.h.
-  DefinedOrUnknownSVal getSizeInElements(ProgramStateRef state,
- const MemRegion* R,
- QualType EleTy) override;
-
   //===--===//
   // Utility methods.
   //===--===//
@@ -1387,37 +1378,6 @@
   return StoreRef(B.asStore(), *this);
 }
 
-//===--===//
-// Extents for regions.
-//===--===//
-
-DefinedOrUnknownSVal
-RegionStoreManager::getSizeInElements(ProgramStateRef state,
-  const MemRegion *R,
-  QualType EleTy) {
-  DefinedOrUnknownSVal Size = getDynamicSize(state, R, svalBuilder);
-  const llvm::APSInt *SizeInt = svalBuilder.getKnownValue(state, Size);
-  if (!SizeInt)
-return UnknownVal();
-
-  CharUnits RegionSize = CharUnits::fromQuantity(SizeInt->getSExtValue());
-
-  if (Ctx.getAsVariableArrayType(EleTy)) {
-// FIXME: We need to track extra state to properly record the size
-// of VLAs.  Returning UnknownVal here, however, is a stop-gap so that
-// we don't have a divide-by-zero below.
-return UnknownVal();
-  }
-
-  CharUnits EleSize = Ctx.getTypeSizeInChars(EleTy);
-
-  // If a variable is reinterpreted as a type that doesn't fit into a larger
-  // type evenly, round it down.
-  // This is a signed value, since it's used in arithmetic with signed indices.
-  return svalBuilder.makeIntVal(RegionSize / EleSize,
-svalBuilder.getArrayIndexType());
-}
-
 //===--===//
 // Location and region casting.
 //===--===//
Index: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
===
--- clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
+++ clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
@@ -11,6 +11,7 @@
 //===--===//
 
 #include "clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h"
+#include "clang/AST/Expr.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
@@ -26,5 +27,37 @@
   return MR->getMemRegionManager().getStaticSize(MR, SVB);
 }
 
+DefinedOrUnknownSVal getDynamicElementCount(ProgramStateRef State,
+const MemRegion *MR,
+SValBuilder ,
+QualType ElementTy) {
+  MemRegionManager  = MR->getMemRegionManager();
+  ASTContext  = MemMgr.getContext();
+
+  CharUnits ElementSize = Ctx.getTypeSizeInChars(ElementTy);
+  DefinedOrUnknownSVal Size = getDynamicSize(State, MR, SVB);
+
+  if (const llvm::APSInt *SizeInt = SVB.getKnownValue(State, Size)) {
+CharUnits RegionSize = CharUnits::fromQuantity(SizeInt->getSExtValue());
+
+// If a variable is reinterpreted as a type that doesn't fit into a larger
+// type evenly, round it down.
+// This is a signed value, since it's used in arithmetic with signed
+// indices.
+return SVB.makeIntVal(RegionSize / ElementSize, SVB.getArrayIndexType());
+  }
+
+  // Try to rely on the 'SValBuilder'.
+  SVal ElementSizeV = SVB.makeIntVal(
+  

[PATCH] D69599: [analyzer] DynamicSize: Remove 'getSizeInElements()' from store

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

> This is the first step to mitigate that issue.

What's the issue?




Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:37-39
+  const llvm::APSInt *SizeInt = SVB.getKnownValue(State, Size);
+  if (!SizeInt)
+return UnknownVal();

Even if the size is not concrete, you can ask `SValBuilder` to perform the 
division. It's going to be a symbolic expression which we won't be able to work 
with anyway, but these days we believe that it's still worth it, in hope that 
our constraint solver eventually gets better.


Repository:
  rC Clang

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

https://reviews.llvm.org/D69599



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

[PATCH] D69599: [analyzer] DynamicSize: Remove 'getSizeInElements()' from store

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

The [1] patch which introduced such static element-count data has only one test 
case in `outofbound.c`:

  void f2() {
int *p = malloc(12);
p[3] = 4; // expected-warning{{Access out-of-bound array element (buffer 
overflow)}}
  }

which probably wanted to be `(int *)malloc(12)`, but in both ways the warning 
occurs, which is problematic. This is the first step to mitigate that issue.
[1] 
https://github.com/llvm/llvm-project/commit/228b0d4defb85b2e7acbb642b9f0b5dfc49d3fe7


Repository:
  rC Clang

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

https://reviews.llvm.org/D69599



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

[PATCH] D69599: [analyzer] DynamicSize: Remove 'getSizeInElements()' from store

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

This patch uses the new `DynamicSize.cpp` to serve dynamic information.
Previously it was static and probably imprecise data.


Repository:
  rC Clang

https://reviews.llvm.org/D69599

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
  clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp

Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -622,15 +622,6 @@
   StoreRef removeDeadBindings(Store store, const StackFrameContext *LCtx,
   SymbolReaper& SymReaper) override;
 
-  //===--===//
-  // Region "extents".
-  //===--===//
-
-  // FIXME: This method will soon be eliminated; see the note in Store.h.
-  DefinedOrUnknownSVal getSizeInElements(ProgramStateRef state,
- const MemRegion* R,
- QualType EleTy) override;
-
   //===--===//
   // Utility methods.
   //===--===//
@@ -1387,37 +1378,6 @@
   return StoreRef(B.asStore(), *this);
 }
 
-//===--===//
-// Extents for regions.
-//===--===//
-
-DefinedOrUnknownSVal
-RegionStoreManager::getSizeInElements(ProgramStateRef state,
-  const MemRegion *R,
-  QualType EleTy) {
-  DefinedOrUnknownSVal Size = getDynamicSize(state, R, svalBuilder);
-  const llvm::APSInt *SizeInt = svalBuilder.getKnownValue(state, Size);
-  if (!SizeInt)
-return UnknownVal();
-
-  CharUnits RegionSize = CharUnits::fromQuantity(SizeInt->getSExtValue());
-
-  if (Ctx.getAsVariableArrayType(EleTy)) {
-// FIXME: We need to track extra state to properly record the size
-// of VLAs.  Returning UnknownVal here, however, is a stop-gap so that
-// we don't have a divide-by-zero below.
-return UnknownVal();
-  }
-
-  CharUnits EleSize = Ctx.getTypeSizeInChars(EleTy);
-
-  // If a variable is reinterpreted as a type that doesn't fit into a larger
-  // type evenly, round it down.
-  // This is a signed value, since it's used in arithmetic with signed indices.
-  return svalBuilder.makeIntVal(RegionSize / EleSize,
-svalBuilder.getArrayIndexType());
-}
-
 //===--===//
 // Location and region casting.
 //===--===//
Index: clang/lib/StaticAnalyzer/Core/MemRegion.cpp
===
--- clang/lib/StaticAnalyzer/Core/MemRegion.cpp
+++ clang/lib/StaticAnalyzer/Core/MemRegion.cpp
@@ -668,7 +668,7 @@
 //===--===//
 
 static DefinedOrUnknownSVal getTypeSize(QualType Ty, ASTContext ,
-  SValBuilder ) {
+SValBuilder ) {
   CharUnits Size = Ctx.getTypeSizeInChars(Ty);
   QualType SizeTy = SVB.getArrayIndexType();
   return SVB.makeIntVal(Size.getQuantity(), SizeTy);
Index: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
===
--- clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
+++ clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
@@ -26,5 +26,26 @@
   return MR->getMemRegionManager().getStaticSize(MR, SVB);
 }
 
+DefinedOrUnknownSVal getDynamicElementCount(ProgramStateRef State,
+const MemRegion *MR,
+SValBuilder ,
+QualType ElementTy) {
+  MemRegionManager  = MR->getMemRegionManager();
+  ASTContext  = MemMgr.getContext();
+
+  DefinedOrUnknownSVal Size = getDynamicSize(State, MR, SVB);
+  const llvm::APSInt *SizeInt = SVB.getKnownValue(State, Size);
+  if (!SizeInt)
+