Hi jordan_rose,
Hi Jordan,
This patch is not yet complete and I'm not completly sure about this patch yet,
as in if this is the correct way to model allocator call.
I would like to get a few inputs if i'm in the right direction.
Since we have modelled Allocator in CFG i'm now trying to plugin in the same
into SA Core.
I'm a bit confused on what part of VisitCXXNewExpr will go into
VisitCXXNewAllocatorCall and if that is required?
In this patch i have just called the relevent allocator function in
VisitCXXNewAllocatorCall and proceeded.
In VisitCXXConstructExpr i check if this constructor was call due to a call to
new in which case i use the CXXNewExpr to conjure a symbol and use the
memregion returned to call the constructor.
Later when VisitCXXNewExpr the same region is returned for the CXXNewExpr and i
continue with other initialization.
This seem to work and constructor is now getting inlined and relevent warnings
are now being detected but i'm not sure if this approach is correct.
Could you guide me if we can follow this approach? If not how exactly to model
VisitCXXNewAllocatorCall call to reuse the allocated memregion in
VisitCXXConstructExpr?
Any inputs would be greatly appreciated.
Thanks
Karthik Bhat
http://llvm-reviews.chandlerc.com/D2616
Files:
test/Analysis/new.cpp
test/Analysis/inline.cpp
test/Analysis/temporaries.cpp
test/Analysis/ctor.mm
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
lib/StaticAnalyzer/Core/ExprEngine.cpp
lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
Index: test/Analysis/new.cpp
===================================================================
--- test/Analysis/new.cpp
+++ test/Analysis/new.cpp
@@ -74,6 +74,18 @@
clang_analyzer_eval(*n == 0); // expected-warning{{TRUE}}
}
+class InlineConstructor {
+public:
+ int* k;
+ InlineConstructor() {k=0;}
+ ~InlineConstructor() {*k=1;} // expected-warning{{Dereference of null pointer (loaded from field 'k')}}
+};
+
+void test_construct_call() {
+ InlineConstructor *m1 = new InlineConstructor();
+ delete m1;
+}
+
struct PtrWrapper {
int *x;
Index: test/Analysis/inline.cpp
===================================================================
--- test/Analysis/inline.cpp
+++ test/Analysis/inline.cpp
@@ -307,17 +307,13 @@
int value;
IntWrapper(int input) : value(input) {
- // We don't want this constructor to be inlined unless we can actually
- // use the proper region for operator new.
- // See PR12014 and <rdar://problem/12180598>.
- clang_analyzer_checkInlined(false); // no-warning
+ clang_analyzer_checkInlined(false); // expected-warning{{FALSE}}
}
};
void test() {
IntWrapper *obj = new IntWrapper(42);
- // should be TRUE
- clang_analyzer_eval(obj->value == 42); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(obj->value == 42); // expected-warning{{TRUE}}
delete obj;
}
Index: test/Analysis/temporaries.cpp
===================================================================
--- test/Analysis/temporaries.cpp
+++ test/Analysis/temporaries.cpp
@@ -105,7 +105,7 @@
// FIXME: should be TRUE, but we don't inline the constructors of
// temporaries because we can't model their destructors yet.
- clang_analyzer_eval(((HasCtorDtor){1, 42}).y == 42); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(((HasCtorDtor){1, 42}).y == 42); // expected-warning{{TRUE}}
#endif
}
}
Index: test/Analysis/ctor.mm
===================================================================
--- test/Analysis/ctor.mm
+++ test/Analysis/ctor.mm
@@ -563,19 +563,17 @@
}
void testNew() {
- // FIXME: Pending proper implementation of constructors for 'new'.
raw_pair *pp = new raw_pair();
- clang_analyzer_eval(pp->p1 == 0); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(pp->p2 == 0); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(pp->p1 == 0); // expected-warning{{TRUE}}
+ clang_analyzer_eval(pp->p2 == 0); // expected-warning{{TRUE}}
}
void testArrayNew() {
- // FIXME: Pending proper implementation of constructors for 'new[]'.
raw_pair *p = new raw_pair[2]();
- clang_analyzer_eval(p[0].p1 == 0); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(p[0].p2 == 0); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(p[1].p1 == 0); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(p[1].p2 == 0); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(p[0].p1 == 0); // expected-warning{{TRUE}}
+ clang_analyzer_eval(p[0].p2 == 0); // expected-warning{{TRUE}}
+ clang_analyzer_eval(p[1].p1 == 0); // expected-warning{{TRUE}}
+ clang_analyzer_eval(p[1].p2 == 0); // expected-warning{{TRUE}}
}
struct initializing_pair {
@@ -670,7 +668,6 @@
void testDynamic() {
List *list = new List{1, 2};
- // FIXME: When we handle constructors with 'new', this will be TRUE.
- clang_analyzer_eval(list->usedInitializerList); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(list->usedInitializerList); // expected-warning{{TRUE}}
}
}
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -18,6 +18,7 @@
#include "clang/StaticAnalyzer/Core/CheckerManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/AST/ParentMap.h"
using namespace clang;
using namespace ento;
@@ -127,7 +128,7 @@
const CFGBlock *B = currBldrCtx->getBlock();
if (currStmtIdx + 1 < B->size()) {
CFGElement Next = (*B)[currStmtIdx+1];
-
+ const Stmt *Parent = LCtx->getParentMap().getParent(CE);
// Is this a constructor for a local variable?
if (Optional<CFGStmt> StmtElem = Next.getAs<CFGStmt>()) {
if (const DeclStmt *DS = dyn_cast<DeclStmt>(StmtElem->getStmt())) {
@@ -140,10 +141,41 @@
}
}
}
+ else if (isa<CXXNewExpr>(Parent)) {
+ const CXXNewExpr *CNE = dyn_cast<CXXNewExpr>(Parent);
+ unsigned blockCount = currBldrCtx->blockCount();
+ DefinedOrUnknownSVal symVal = UnknownVal();
+ FunctionDecl *FD = CNE->getOperatorNew();
+ bool IsStandardGlobalOpNewFunction = false;
+ if (FD && !isa<CXXMethodDecl>(FD) && !FD->isVariadic()) {
+ if (FD->getNumParams() == 2) {
+ QualType T = FD->getParamDecl(1)->getType();
+ if (const IdentifierInfo *II = T.getBaseTypeIdentifier())
+ // NoThrow placement new behaves as a standard new.
+ IsStandardGlobalOpNewFunction = II->getName().equals("nothrow_t");
+ }
+ else
+ // Placement forms are considered non-standard.
+ IsStandardGlobalOpNewFunction = (FD->getNumParams() == 1);
+ }
+
+ if (IsStandardGlobalOpNewFunction)
+ symVal = svalBuilder.getConjuredHeapSymbolVal(CNE, LCtx, blockCount);
+ else
+ symVal = svalBuilder.conjureSymbolVal(0, CNE, LCtx, CNE->getType(),
+ blockCount);
+ SVal DestVal = UnknownVal();
+ const MemRegion *Dest = symVal.getAsRegion();
+ if (Dest)
+ DestVal = loc::MemRegionVal(Dest);
+ QualType Ty = CNE->getType();
+ DestVal = makeZeroElementRegion(State, DestVal, Ty);
+ Target = DestVal.getAsRegion();
+ }
}
// Is this a constructor for a member?
- if (Optional<CFGInitializer> InitElem = Next.getAs<CFGInitializer>()) {
+ else if (Optional<CFGInitializer> InitElem = Next.getAs<CFGInitializer>()) {
const CXXCtorInitializer *Init = InitElem->getInitializer();
assert(Init->isAnyMemberInitializer());
@@ -328,15 +360,39 @@
getCheckerManager().runCheckersForPostCall(Dst, DstInvalidated,
*Call, *this);
}
+void ExprEngine::VisitCXXNewAllocatorCall(const CXXNewExpr *CNE,
+ ExplodedNode *Pred,
+ ExplodedNodeSet &Dst) {
+ ProgramStateRef State = Pred->getState();
+ const LocationContext *LCtx = Pred->getLocationContext();
+ CallEventManager &CEMgr = getStateManager().getCallEventManager();
+ CallEventRef<CXXAllocatorCall> Call =
+ CEMgr.getCXXAllocatorCall(CNE, State, LCtx);
+
+ PrettyStackTraceLoc CrashInfo(getContext().getSourceManager(),
+ Call->getSourceRange().getBegin(),
+ "Error evaluating destructor");
+
+ ExplodedNodeSet DstPreCall;
+ getCheckerManager().runCheckersForPreCall(DstPreCall, Pred,
+ *Call, *this);
+
+ ExplodedNodeSet DstInvalidated;
+ StmtNodeBuilder Bldr(DstPreCall, DstInvalidated, *currBldrCtx);
+ for (ExplodedNodeSet::iterator I = DstPreCall.begin(), E = DstPreCall.end();
+ I != E; ++I)
+ defaultEvalCall(Bldr, *I, *Call);
+
+ getCheckerManager().runCheckersForPostCall(Dst, DstInvalidated,
+ *Call, *this);
+}
+
void ExprEngine::VisitCXXNewExpr(const CXXNewExpr *CNE, ExplodedNode *Pred,
ExplodedNodeSet &Dst) {
- // FIXME: Much of this should eventually migrate to CXXAllocatorCall.
- // Also, we need to decide how allocators actually work -- they're not
- // really part of the CXXNewExpr because they happen BEFORE the
- // CXXConstructExpr subexpression. See PR12014 for some discussion.
-
+
unsigned blockCount = currBldrCtx->blockCount();
+ ProgramStateRef State = Pred->getState();
const LocationContext *LCtx = Pred->getLocationContext();
DefinedOrUnknownSVal symVal = UnknownVal();
FunctionDecl *FD = CNE->getOperatorNew();
@@ -363,18 +419,6 @@
symVal = svalBuilder.conjureSymbolVal(0, CNE, LCtx, CNE->getType(),
blockCount);
- ProgramStateRef State = Pred->getState();
- CallEventManager &CEMgr = getStateManager().getCallEventManager();
- CallEventRef<CXXAllocatorCall> Call =
- CEMgr.getCXXAllocatorCall(CNE, State, LCtx);
-
- // Invalidate placement args.
- // FIXME: Once we figure out how we want allocators to work,
- // we should be using the usual pre-/(default-)eval-/post-call checks here.
- State = Call->invalidateRegions(blockCount);
- if (!State)
- return;
-
// If this allocation function is not declared as non-throwing, failures
// /must/ be signalled by exceptions, and thus the return value will never be
// NULL. -fno-exceptions does not influence this semantics.
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -553,12 +553,8 @@
void ExprEngine::ProcessNewAllocator(const CXXNewExpr *NE,
ExplodedNode *Pred) {
- //TODO: Implement VisitCXXNewAllocatorCall
ExplodedNodeSet Dst;
- NodeBuilder Bldr(Pred, Dst, *currBldrCtx);
- const LocationContext *LCtx = Pred->getLocationContext();
- PostImplicitCall PP(NE->getOperatorNew(), NE->getLocStart(), LCtx);
- Bldr.generateNode(PP, Pred->getState(), Pred);
+ VisitCXXNewAllocatorCall(NE, Pred, Dst);
Engine.enqueue(Dst, currBldrCtx->getBlock(), currStmtIdx);
}
Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -614,15 +614,9 @@
if (Target && isa<ElementRegion>(Target))
return CIP_DisallowedOnce;
- // FIXME: This is a hack. We don't use the correct region for a new
- // expression, so if we inline the constructor its result will just be
- // thrown away. This short-term hack is tracked in <rdar://problem/12180598>
- // and the longer-term possible fix is discussed in PR12014.
const CXXConstructExpr *CtorExpr = Ctor.getOriginExpr();
- if (const Stmt *Parent = CurLC->getParentMap().getParent(CtorExpr))
- if (isa<CXXNewExpr>(Parent))
- return CIP_DisallowedOnce;
+
// Inlining constructors requires including initializers in the CFG.
const AnalysisDeclContext *ADC = CallerSFC->getAnalysisDeclContext();
assert(ADC->getCFGBuildOptions().AddInitializers && "No CFG initializers");
@@ -640,7 +634,7 @@
// FIXME: This is a hack. We don't handle temporary destructors
// right now, so we shouldn't inline their constructors.
if (CtorExpr->getConstructionKind() == CXXConstructExpr::CK_Complete)
- if (!Target || !isa<DeclRegion>(Target))
+ if (!Target)
return CIP_DisallowedOnce;
break;
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===================================================================
--- include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -420,7 +420,9 @@
void VisitCXXDestructor(QualType ObjectType, const MemRegion *Dest,
const Stmt *S, bool IsBaseDtor,
ExplodedNode *Pred, ExplodedNodeSet &Dst);
-
+ void VisitCXXNewAllocatorCall(const CXXNewExpr *CNE,
+ ExplodedNode *Pred,
+ ExplodedNodeSet &Dst);
void VisitCXXNewExpr(const CXXNewExpr *CNE, ExplodedNode *Pred,
ExplodedNodeSet &Dst);
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
