[PATCH] D69540: [analyzer] DynamicSize: Remove 'getExtent()' from regions
This revision was automatically updated to reflect the committed changes.
Closed by commit rG601687bf731a: [analyzer] DynamicSize: Remove
getExtent() from regions (authored by Charusso).
Changed prior to commit:
https://reviews.llvm.org/D69540?vs=227013=241447#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69540/new/
https://reviews.llvm.org/D69540
Files:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
clang/lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp
clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
clang/lib/StaticAnalyzer/Core/CMakeLists.txt
clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
clang/lib/StaticAnalyzer/Core/MemRegion.cpp
clang/lib/StaticAnalyzer/Core/RegionStore.cpp
clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
Index: clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
+++ clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
@@ -329,7 +329,7 @@
}
QualType SymbolExtent::getType() const {
- ASTContext = R->getMemRegionManager()->getContext();
+ ASTContext = R->getMemRegionManager().getContext();
return Ctx.getSizeType();
}
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -23,6 +23,7 @@
#include "clang/Basic/TargetInfo.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
@@ -876,7 +877,7 @@
// Find the length (in bits) of the region being invalidated.
uint64_t Length = UINT64_MAX;
- SVal Extent = Top->getExtent(SVB);
+ SVal Extent = Top->getMemRegionManager().getStaticSize(Top, SVB);
if (Optional ExtentCI =
Extent.getAs()) {
const llvm::APSInt = ExtentCI->getValue();
@@ -1394,7 +1395,7 @@
RegionStoreManager::getSizeInElements(ProgramStateRef state,
const MemRegion *R,
QualType EleTy) {
- SVal Size = cast(R)->getExtent(svalBuilder);
+ DefinedOrUnknownSVal Size = getDynamicSize(state, R, svalBuilder);
const llvm::APSInt *SizeInt = svalBuilder.getKnownValue(state, Size);
if (!SizeInt)
return UnknownVal();
Index: clang/lib/StaticAnalyzer/Core/MemRegion.cpp
===
--- clang/lib/StaticAnalyzer/Core/MemRegion.cpp
+++ clang/lib/StaticAnalyzer/Core/MemRegion.cpp
@@ -142,7 +142,7 @@
return false;
}
-MemRegionManager* SubRegion::getMemRegionManager() const {
+MemRegionManager ::getMemRegionManager() const {
const SubRegion* r = this;
do {
const MemRegion *superRegion = r->getSuperRegion();
@@ -159,56 +159,6 @@
return SSR ? SSR->getStackFrame() : nullptr;
}
-//===--===//
-// Region extents.
-//===--===//
-
-DefinedOrUnknownSVal TypedValueRegion::getExtent(SValBuilder ) const {
- ASTContext = svalBuilder.getContext();
- QualType T = getDesugaredValueType(Ctx);
-
- if (isa(T))
-return nonloc::SymbolVal(svalBuilder.getSymbolManager().getExtentSymbol(this));
- if (T->isIncompleteType())
-return UnknownVal();
-
- CharUnits size = Ctx.getTypeSizeInChars(T);
- QualType sizeTy = svalBuilder.getArrayIndexType();
- return svalBuilder.makeIntVal(size.getQuantity(), sizeTy);
-}
-
-DefinedOrUnknownSVal FieldRegion::getExtent(SValBuilder ) const {
- // Force callers to deal with bitfields explicitly.
- if (getDecl()->isBitField())
-return UnknownVal();
-
- DefinedOrUnknownSVal Extent = DeclRegion::getExtent(svalBuilder);
-
- // A zero-length array at the end of a struct often stands for dynamically-
- // allocated extra memory.
- if (Extent.isZeroConstant()) {
-QualType T = getDesugaredValueType(svalBuilder.getContext());
-
-if (isa(T))
- return UnknownVal();
- }
-
- return
[PATCH] D69540: [analyzer] DynamicSize: Remove 'getExtent()' from regions
Charusso added a comment. Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69540/new/ https://reviews.llvm.org/D69540 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69540: [analyzer] DynamicSize: Remove 'getExtent()' from regions
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Fantastic, let's land this! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69540/new/ https://reviews.llvm.org/D69540 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69540: [analyzer] DynamicSize: Remove 'getExtent()' from regions
Charusso added inline comments.
Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:158
// symbols to use, only content metadata.
- return nonloc::SymbolVal(SymMgr.getExtentSymbol(FTR));
+ return FTR->getMemRegionManager().getStaticSize(FTR);
NoQ wrote:
> Charusso wrote:
> > NoQ wrote:
> > > Charusso wrote:
> > > > That is the breaking test's code, which is super wonky. I cannot
> > > > understand what is the rational behind this concept.
> > > Your new code would return a concrete integer here:
> > > ```lang=c++
> > > case MemRegion::FunctionCodeRegionKind: {
> > > QualType Ty = cast(SR)->getDesugaredLocationType(Ctx);
> > > return getTypeSize(Ty, Ctx, SVB);
> > > }
> > > ```
> > > Previously it was a symbol.
> > >
> > > That said, the original code looks like a super gross hack: they used an
> > > extent symbol not because they actually needed an extent, but because
> > > they didn't have a better symbol to use :/ I guess you should just keep
> > > the extent symbol for now :/
> > I see, that was really missing, whoops. Thanks!
> Let's keep the hack here, not move it to the region manager. I.e., please
> create `SymbolExtent` here directly and then separately keep creating it in
> `getStaticSize()`. I.e., don't reuse the code when it isn't supposed to
> behave identically, even if right now it accidentally does behave identically.
Ah, yes, it makes sense. In my mind they are connecting even in fixing them.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69540/new/
https://reviews.llvm.org/D69540
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69540: [analyzer] DynamicSize: Remove 'getExtent()' from regions
Charusso updated this revision to Diff 227013.
Charusso marked 2 inline comments as done.
Charusso added a comment.
- Unpack the issue-solving about code regions.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69540/new/
https://reviews.llvm.org/D69540
Files:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
clang/lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp
clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
clang/lib/StaticAnalyzer/Core/CMakeLists.txt
clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
clang/lib/StaticAnalyzer/Core/MemRegion.cpp
clang/lib/StaticAnalyzer/Core/RegionStore.cpp
clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
Index: clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
+++ clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
@@ -329,7 +329,7 @@
}
QualType SymbolExtent::getType() const {
- ASTContext = R->getMemRegionManager()->getContext();
+ ASTContext = R->getMemRegionManager().getContext();
return Ctx.getSizeType();
}
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -23,6 +23,7 @@
#include "clang/Basic/TargetInfo.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
@@ -876,7 +877,7 @@
// Find the length (in bits) of the region being invalidated.
uint64_t Length = UINT64_MAX;
- SVal Extent = Top->getExtent(SVB);
+ SVal Extent = Top->getMemRegionManager().getStaticSize(Top, SVB);
if (Optional ExtentCI =
Extent.getAs()) {
const llvm::APSInt = ExtentCI->getValue();
@@ -1394,7 +1395,7 @@
RegionStoreManager::getSizeInElements(ProgramStateRef state,
const MemRegion *R,
QualType EleTy) {
- SVal Size = cast(R)->getExtent(svalBuilder);
+ DefinedOrUnknownSVal Size = getDynamicSize(state, R, svalBuilder);
const llvm::APSInt *SizeInt = svalBuilder.getKnownValue(state, Size);
if (!SizeInt)
return UnknownVal();
Index: clang/lib/StaticAnalyzer/Core/MemRegion.cpp
===
--- clang/lib/StaticAnalyzer/Core/MemRegion.cpp
+++ clang/lib/StaticAnalyzer/Core/MemRegion.cpp
@@ -142,7 +142,7 @@
return false;
}
-MemRegionManager* SubRegion::getMemRegionManager() const {
+MemRegionManager ::getMemRegionManager() const {
const SubRegion* r = this;
do {
const MemRegion *superRegion = r->getSuperRegion();
@@ -159,56 +159,6 @@
return SSR ? SSR->getStackFrame() : nullptr;
}
-//===--===//
-// Region extents.
-//===--===//
-
-DefinedOrUnknownSVal TypedValueRegion::getExtent(SValBuilder ) const {
- ASTContext = svalBuilder.getContext();
- QualType T = getDesugaredValueType(Ctx);
-
- if (isa(T))
-return nonloc::SymbolVal(svalBuilder.getSymbolManager().getExtentSymbol(this));
- if (T->isIncompleteType())
-return UnknownVal();
-
- CharUnits size = Ctx.getTypeSizeInChars(T);
- QualType sizeTy = svalBuilder.getArrayIndexType();
- return svalBuilder.makeIntVal(size.getQuantity(), sizeTy);
-}
-
-DefinedOrUnknownSVal FieldRegion::getExtent(SValBuilder ) const {
- // Force callers to deal with bitfields explicitly.
- if (getDecl()->isBitField())
-return UnknownVal();
-
- DefinedOrUnknownSVal Extent = DeclRegion::getExtent(svalBuilder);
-
- // A zero-length array at the end of a struct often stands for dynamically-
- // allocated extra memory.
- if (Extent.isZeroConstant()) {
-QualType T = getDesugaredValueType(svalBuilder.getContext());
-
-if (isa(T))
- return UnknownVal();
- }
-
- return Extent;
-}
-
-DefinedOrUnknownSVal AllocaRegion::getExtent(SValBuilder ) const {
- return nonloc::SymbolVal(svalBuilder.getSymbolManager().getExtentSymbol(this));
-}
-
-DefinedOrUnknownSVal
[PATCH] D69540: [analyzer] DynamicSize: Remove 'getExtent()' from regions
NoQ added inline comments.
Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:158
// symbols to use, only content metadata.
- return nonloc::SymbolVal(SymMgr.getExtentSymbol(FTR));
+ return FTR->getMemRegionManager().getStaticSize(FTR);
Charusso wrote:
> NoQ wrote:
> > Charusso wrote:
> > > That is the breaking test's code, which is super wonky. I cannot
> > > understand what is the rational behind this concept.
> > Your new code would return a concrete integer here:
> > ```lang=c++
> > case MemRegion::FunctionCodeRegionKind: {
> > QualType Ty = cast(SR)->getDesugaredLocationType(Ctx);
> > return getTypeSize(Ty, Ctx, SVB);
> > }
> > ```
> > Previously it was a symbol.
> >
> > That said, the original code looks like a super gross hack: they used an
> > extent symbol not because they actually needed an extent, but because they
> > didn't have a better symbol to use :/ I guess you should just keep the
> > extent symbol for now :/
> I see, that was really missing, whoops. Thanks!
Let's keep the hack here, not move it to the region manager. I.e., please
create `SymbolExtent` here directly and then separately keep creating it in
`getStaticSize()`. I.e., don't reuse the code when it isn't supposed to behave
identically, even if right now it accidentally does behave identically.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69540/new/
https://reviews.llvm.org/D69540
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69540: [analyzer] DynamicSize: Remove 'getExtent()' from regions
Charusso added a comment.
Thanks for the review! I am not sure why but after your review I always see the
most appropriate design immediately.
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1253-1255
+ MemRegionManager(ASTContext , llvm::BumpPtrAllocator , SValBuilder ,
+ SymbolManager )
+ : Ctx(c), A(a), SVB(SVB), SymMgr(SymMgr) {}
NoQ wrote:
> This looks like a layering violation to me. It's not super important, but i'd
> rather not have `MemRegion` depend on `SValBuilder`.
>
> Can we have `getStaticSize()` be a method on `SValBuilder` instead? Or simply
> a standalone static function in `DynamicSize.cpp`? 'Cause ideally it
> shouldn't be called directly.
Hm, in my game-dev world every manager knows about every manager, so I felt
that it needs to work. I like the idea behind the directness and hiding the
implementation, but I believe the `MemRegionManager` should manage its stuff.
Also we are lucky, because the `SValBuilder` is available everywhere with a
tiny stress on the API.
Comment at: clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp:95-99
+DefinedOrUnknownSVal DynSize = getDynamicSize(state, R);
+
+DefinedOrUnknownSVal DynSizeMatchesSizeArg =
+svalBuilder.evalEQ(state, DynSize,
Size.castAs());
+state = state->assume(DynSizeMatchesSizeArg, true);
NoQ wrote:
> As the next obvious step for the next patch, i suggest replacing `evalEQ()`
> with some sort of `setDynamicSize()` here.
Okay, good idea, thanks! I want to eliminate `getSizeInElements` as well.
Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:158
// symbols to use, only content metadata.
- return nonloc::SymbolVal(SymMgr.getExtentSymbol(FTR));
+ return FTR->getMemRegionManager().getStaticSize(FTR);
NoQ wrote:
> Charusso wrote:
> > That is the breaking test's code, which is super wonky. I cannot understand
> > what is the rational behind this concept.
> Your new code would return a concrete integer here:
> ```lang=c++
> case MemRegion::FunctionCodeRegionKind: {
> QualType Ty = cast(SR)->getDesugaredLocationType(Ctx);
> return getTypeSize(Ty, Ctx, SVB);
> }
> ```
> Previously it was a symbol.
>
> That said, the original code looks like a super gross hack: they used an
> extent symbol not because they actually needed an extent, but because they
> didn't have a better symbol to use :/ I guess you should just keep the extent
> symbol for now :/
I see, that was really missing, whoops. Thanks!
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69540/new/
https://reviews.llvm.org/D69540
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69540: [analyzer] DynamicSize: Remove 'getExtent()' from regions
Charusso updated this revision to Diff 227006.
Charusso marked 12 inline comments as done.
Charusso added a comment.
- Fix.
- Added a `FIXME` about the missing symbol of `BlockDataRegion`,
`BlockCodeRegion` and `FunctionCodeRegion`.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69540/new/
https://reviews.llvm.org/D69540
Files:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
clang/lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp
clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
clang/lib/StaticAnalyzer/Core/CMakeLists.txt
clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
clang/lib/StaticAnalyzer/Core/MemRegion.cpp
clang/lib/StaticAnalyzer/Core/RegionStore.cpp
clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
Index: clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
+++ clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
@@ -329,7 +329,7 @@
}
QualType SymbolExtent::getType() const {
- ASTContext = R->getMemRegionManager()->getContext();
+ ASTContext = R->getMemRegionManager().getContext();
return Ctx.getSizeType();
}
Index: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -155,7 +155,7 @@
// FIXME: Currently we are using an extent symbol here,
// because there are no generic region address metadata
// symbols to use, only content metadata.
- return nonloc::SymbolVal(SymMgr.getExtentSymbol(FTR));
+ return MemMgr.getStaticSize(FTR, *this);
if (const SymbolicRegion *SymR = R->getSymbolicBase())
return makeNonLoc(SymR->getSymbol(), BO_NE,
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -23,6 +23,7 @@
#include "clang/Basic/TargetInfo.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
@@ -876,7 +877,7 @@
// Find the length (in bits) of the region being invalidated.
uint64_t Length = UINT64_MAX;
- SVal Extent = Top->getExtent(SVB);
+ SVal Extent = Top->getMemRegionManager().getStaticSize(Top, SVB);
if (Optional ExtentCI =
Extent.getAs()) {
const llvm::APSInt = ExtentCI->getValue();
@@ -1394,7 +1395,7 @@
RegionStoreManager::getSizeInElements(ProgramStateRef state,
const MemRegion *R,
QualType EleTy) {
- SVal Size = cast(R)->getExtent(svalBuilder);
+ DefinedOrUnknownSVal Size = getDynamicSize(state, R, svalBuilder);
const llvm::APSInt *SizeInt = svalBuilder.getKnownValue(state, Size);
if (!SizeInt)
return UnknownVal();
Index: clang/lib/StaticAnalyzer/Core/MemRegion.cpp
===
--- clang/lib/StaticAnalyzer/Core/MemRegion.cpp
+++ clang/lib/StaticAnalyzer/Core/MemRegion.cpp
@@ -142,7 +142,7 @@
return false;
}
-MemRegionManager* SubRegion::getMemRegionManager() const {
+MemRegionManager ::getMemRegionManager() const {
const SubRegion* r = this;
do {
const MemRegion *superRegion = r->getSuperRegion();
@@ -159,56 +159,6 @@
return SSR ? SSR->getStackFrame() : nullptr;
}
-//===--===//
-// Region extents.
-//===--===//
-
-DefinedOrUnknownSVal TypedValueRegion::getExtent(SValBuilder ) const {
- ASTContext = svalBuilder.getContext();
- QualType T = getDesugaredValueType(Ctx);
-
- if (isa(T))
-return nonloc::SymbolVal(svalBuilder.getSymbolManager().getExtentSymbol(this));
- if (T->isIncompleteType())
-return UnknownVal();
-
- CharUnits size = Ctx.getTypeSizeInChars(T);
- QualType sizeTy =
[PATCH] D69540: [analyzer] DynamicSize: Remove 'getExtent()' from regions
NoQ added inline comments.
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1253-1255
+ MemRegionManager(ASTContext , llvm::BumpPtrAllocator , SValBuilder ,
+ SymbolManager )
+ : Ctx(c), A(a), SVB(SVB), SymMgr(SymMgr) {}
This looks like a layering violation to me. It's not super important, but i'd
rather not have `MemRegion` depend on `SValBuilder`.
Can we have `getStaticSize()` be a method on `SValBuilder` instead? Or simply a
standalone static function in `DynamicSize.cpp`? 'Cause ideally it shouldn't be
called directly.
Comment at: clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp:95-99
+DefinedOrUnknownSVal DynSize = getDynamicSize(state, R);
+
+DefinedOrUnknownSVal DynSizeMatchesSizeArg =
+svalBuilder.evalEQ(state, DynSize,
Size.castAs());
+state = state->assume(DynSizeMatchesSizeArg, true);
As the next obvious step for the next patch, i suggest replacing `evalEQ()`
with some sort of `setDynamicSize()` here.
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1074
std::tie(StateWholeReg, StateNotWholeReg) =
-State->assume(svalBuilder.evalEQ(State, Extent, *SizeNL));
+State->assume(svalBuilder.evalEQ(State, SizeDV, *SizeNL));
And here.
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1413
svalBuilder.getArrayIndexType());
-DefinedOrUnknownSVal extentMatchesSize = svalBuilder.evalEQ(
-State, Extent, SizeInBytes.castAs());
-State = State->assume(extentMatchesSize, true);
+DefinedOrUnknownSVal DynSizeMatchesSize = svalBuilder.evalEQ(
+State, DynSize, SizeInBytes.castAs());
And here.
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1547
+DefinedOrUnknownSVal DynSizeMatchesSize =
+svalBuilder.evalEQ(State, DynSize, *DefinedSize);
And here.
Comment at: clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp:176
DefinedOrUnknownSVal sizeIsKnown =
-svalBuilder.evalEQ(state, Extent, ArraySize);
+ svalBuilder.evalEQ(state, DynSize, ArraySize);
state = state->assume(sizeIsKnown, true);
And here.
Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:29-30
+const MemRegion *MR) {
+ if (const DefinedOrUnknownSVal *Size = State->get(MR))
+return *Size;
+
For now the map is always empty, right? Maybe we should remove the map until we
actually add a setter.
Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:692-693
+ case MemRegion::FunctionCodeRegionKind: {
+QualType Ty = cast(SR)->getDesugaredLocationType(Ctx);
+return getTypeSize(Ty, Ctx, SVB);
+ }
This code doesn't make much sense; it'll return a pointer size, which has
nothing to do with the size of the region.
Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:158
// symbols to use, only content metadata.
- return nonloc::SymbolVal(SymMgr.getExtentSymbol(FTR));
+ return FTR->getMemRegionManager().getStaticSize(FTR);
Charusso wrote:
> That is the breaking test's code, which is super wonky. I cannot understand
> what is the rational behind this concept.
Your new code would return a concrete integer here:
```lang=c++
case MemRegion::FunctionCodeRegionKind: {
QualType Ty = cast(SR)->getDesugaredLocationType(Ctx);
return getTypeSize(Ty, Ctx, SVB);
}
```
Previously it was a symbol.
That said, the original code looks like a super gross hack: they used an extent
symbol not because they actually needed an extent, but because they didn't have
a better symbol to use :/ I guess you should just keep the extent symbol for
now :/
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69540/new/
https://reviews.llvm.org/D69540
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69540: [analyzer] DynamicSize: Remove 'getExtent()' from regions
Charusso marked 3 inline comments as done.
Charusso added inline comments.
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:880
uint64_t Length = UINT64_MAX;
- SVal Extent = Top->getExtent(SVB);
+ SVal Extent = Top->getMemRegionManager().getStaticSize(Top);
if (Optional ExtentCI =
That is why the static size information needs to be obtainable by a manager,
which seems like a design problem from the very beginning.
Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:158
// symbols to use, only content metadata.
- return nonloc::SymbolVal(SymMgr.getExtentSymbol(FTR));
+ return FTR->getMemRegionManager().getStaticSize(FTR);
That is the breaking test's code, which is super wonky. I cannot understand
what is the rational behind this concept.
Comment at: clang/test/Analysis/weak-functions.c:10
clang_analyzer_eval(myFunc == NULL); // expected-warning{{FALSE}}
- clang_analyzer_eval(myWeakFunc == NULL); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(myWeakFunc == NULL); // expected-warning{{FALSE}}
if (myWeakFunc == NULL) {
I have literally copy-pasted the implementation of `getExtent()`, so I could
not figured out why this test has been changed.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69540/new/
https://reviews.llvm.org/D69540
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69540: [analyzer] DynamicSize: Remove 'getExtent()' from regions
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, mgorny.
This patch introduces a placeholder for representing the dynamic size of
regions. It also moves the `getExtent()` method of `SubRegions` to the
`MemRegionManager` as `getStaticSize()`.
Repository:
rC Clang
https://reviews.llvm.org/D69540
Files:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
clang/lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp
clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
clang/lib/StaticAnalyzer/Core/CMakeLists.txt
clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
clang/lib/StaticAnalyzer/Core/MemRegion.cpp
clang/lib/StaticAnalyzer/Core/RegionStore.cpp
clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
clang/test/Analysis/weak-functions.c
Index: clang/test/Analysis/weak-functions.c
===
--- clang/test/Analysis/weak-functions.c
+++ clang/test/Analysis/weak-functions.c
@@ -7,9 +7,9 @@
void testWeakFuncIsNull()
{
clang_analyzer_eval(myFunc == NULL); // expected-warning{{FALSE}}
- clang_analyzer_eval(myWeakFunc == NULL); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(myWeakFunc == NULL); // expected-warning{{FALSE}}
if (myWeakFunc == NULL) {
-clang_analyzer_eval(myWeakFunc == NULL); // expected-warning{{TRUE}}
+clang_analyzer_eval(myWeakFunc == NULL); // no-warning
} else {
clang_analyzer_eval(myWeakFunc == NULL); // expected-warning{{FALSE}}
}
@@ -17,9 +17,9 @@
void testWeakFuncIsNot()
{
- clang_analyzer_eval(myWeakFunc == NULL); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(myWeakFunc == NULL); // expected-warning{{FALSE}}
if (!myWeakFunc) {
-clang_analyzer_eval(myWeakFunc == NULL); // expected-warning{{TRUE}}
+clang_analyzer_eval(myWeakFunc == NULL); // no-warning
} else {
clang_analyzer_eval(myWeakFunc == NULL); // expected-warning{{FALSE}}
}
@@ -27,11 +27,11 @@
void testWeakFuncIsTrue()
{
-clang_analyzer_eval(myWeakFunc == NULL); // expected-warning{{UNKNOWN}}
+clang_analyzer_eval(myWeakFunc == NULL); // expected-warning{{FALSE}}
if (myWeakFunc) {
clang_analyzer_eval(myWeakFunc == NULL); // expected-warning{{FALSE}}
} else {
-clang_analyzer_eval(myWeakFunc == NULL); // expected-warning{{TRUE}}
+clang_analyzer_eval(myWeakFunc == NULL); // no-warning
}
}
Index: clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
+++ clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
@@ -329,7 +329,7 @@
}
QualType SymbolExtent::getType() const {
- ASTContext = R->getMemRegionManager()->getContext();
+ ASTContext = R->getMemRegionManager().getContext();
return Ctx.getSizeType();
}
Index: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -155,7 +155,7 @@
// FIXME: Currently we are using an extent symbol here,
// because there are no generic region address metadata
// symbols to use, only content metadata.
- return nonloc::SymbolVal(SymMgr.getExtentSymbol(FTR));
+ return FTR->getMemRegionManager().getStaticSize(FTR);
if (const SymbolicRegion *SymR = R->getSymbolicBase())
return makeNonLoc(SymR->getSymbol(), BO_NE,
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -23,6 +23,7 @@
#include "clang/Basic/TargetInfo.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
#include
