NoQ updated this revision to Diff 129362.
NoQ added a comment.
Rename getters and setters for the new state trait (i.e. "push" -> "set", etc.,
because we no longer have a stack).
Also make them static, so that it was potentially possible to access them from
elsewhere, eg. from `CallEvent`, if deemed necessary.
https://reviews.llvm.org/D40560
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
lib/StaticAnalyzer/Core/ExprEngine.cpp
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
test/Analysis/inline.cpp
test/Analysis/new-ctor-conservative.cpp
test/Analysis/new-ctor-inlined.cpp
test/Analysis/new-ctor-recursive.cpp
Index: test/Analysis/new-ctor-recursive.cpp
===================================================================
--- /dev/null
+++ test/Analysis/new-ctor-recursive.cpp
@@ -0,0 +1,118 @@
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus.NewDelete,cplusplus.NewDeleteLeaks,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+
+void clang_analyzer_eval(bool);
+void clang_analyzer_dump(int);
+
+typedef __typeof__(sizeof(int)) size_t;
+
+void *conjure();
+void exit(int);
+
+struct S;
+
+S *global_s;
+
+// Recursive operator kinda placement new.
+void *operator new(size_t size, S *place);
+
+enum class ConstructionKind : char {
+ Garbage,
+ Recursive
+};
+
+struct S {
+public:
+ int x;
+ S(): x(1) {}
+ S(int y): x(y) {}
+
+ S(ConstructionKind k) {
+ switch (k) {
+ case ConstructionKind::Recursive: { // Call one more operator new 'r'ecursively.
+ S *s = new (nullptr) S(5);
+ x = s->x + 1;
+ global_s = s;
+ return;
+ }
+ case ConstructionKind::Garbage: {
+ // Leaves garbage in 'x'.
+ }
+ }
+ }
+ ~S() {}
+};
+
+// Do not try this at home!
+void *operator new(size_t size, S *place) {
+ if (!place)
+ return new S();
+ return place;
+}
+
+void testThatCharConstructorIndeedYieldsGarbage() {
+ S *s = new S(ConstructionKind::Garbage);
+ clang_analyzer_eval(s->x == 0); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(s->x == 1); // expected-warning{{UNKNOWN}}
+ // FIXME: This should warn, but MallocChecker doesn't default-bind regions
+ // returned by standard operator new to garbage.
+ s->x += 1; // no-warning
+ delete s;
+}
+
+
+void testChainedOperatorNew() {
+ S *s;
+ // * Evaluate standard new.
+ // * Evaluate constructor S(3).
+ // * Bind value for standard new.
+ // * Evaluate our custom new.
+ // * Evaluate constructor S(Garbage).
+ // * Bind value for our custom new.
+ s = new (new S(3)) S(ConstructionKind::Garbage);
+ clang_analyzer_eval(s->x == 3); // expected-warning{{TRUE}}
+ // expected-warning@+9{{Potential leak of memory pointed to by 's'}}
+
+ // * Evaluate standard new.
+ // * Evaluate constructor S(Garbage).
+ // * Bind value for standard new.
+ // * Evaluate our custom new.
+ // * Evaluate constructor S(4).
+ // * Bind value for our custom new.
+ s = new (new S(ConstructionKind::Garbage)) S(4);
+ clang_analyzer_eval(s->x == 4); // expected-warning{{TRUE}}
+ delete s;
+
+ // -> Enter our custom new (nullptr).
+ // * Evaluate standard new.
+ // * Inline constructor S().
+ // * Bind value for standard new.
+ // <- Exit our custom new (nullptr).
+ // * Evaluate constructor S(Garbage).
+ // * Bind value for our custom new.
+ s = new (nullptr) S(ConstructionKind::Garbage);
+ clang_analyzer_eval(s->x == 1); // expected-warning{{TRUE}}
+ delete s;
+
+ // -> Enter our custom new (nullptr).
+ // * Evaluate standard new.
+ // * Inline constructor S().
+ // * Bind value for standard new.
+ // <- Exit our custom new (nullptr).
+ // -> Enter constructor S(Recursive).
+ // -> Enter our custom new (nullptr).
+ // * Evaluate standard new.
+ // * Inline constructor S().
+ // * Bind value for standard new.
+ // <- Exit our custom new (nullptr).
+ // * Evaluate constructor S(5).
+ // * Bind value for our custom new (nullptr).
+ // * Assign that value to global_s.
+ // <- Exit constructor S(Recursive).
+ // * Bind value for our custom new (nullptr).
+ global_s = nullptr;
+ s = new (nullptr) S(ConstructionKind::Recursive);
+ clang_analyzer_eval(global_s); // expected-warning{{TRUE}}
+ clang_analyzer_eval(global_s->x == 5); // expected-warning{{TRUE}}
+ clang_analyzer_eval(s->x == 6); // expected-warning{{TRUE}}
+ delete s;
+}
Index: test/Analysis/new-ctor-inlined.cpp
===================================================================
--- /dev/null
+++ test/Analysis/new-ctor-inlined.cpp
@@ -0,0 +1,40 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+
+void clang_analyzer_eval(bool);
+
+typedef __typeof__(sizeof(int)) size_t;
+
+void *conjure();
+void exit(int);
+
+void *operator new(size_t size) throw() {
+ void *x = conjure();
+ if (x == 0)
+ exit(1);
+ return x;
+}
+
+struct S {
+ int x;
+ S() : x(1) {}
+ ~S() {}
+};
+
+void checkNewAndConstructorInlining() {
+ S *s = new S;
+ // Check that the symbol for 's' is not dying.
+ clang_analyzer_eval(s != 0); // expected-warning{{TRUE}}
+ // Check that bindings are correct (and also not dying).
+ clang_analyzer_eval(s->x == 1); // expected-warning{{TRUE}}
+}
+
+struct Sp {
+ Sp *p;
+ Sp(Sp *p): p(p) {}
+ ~Sp() {}
+};
+
+void checkNestedNew() {
+ Sp *p = new Sp(new Sp(0));
+ clang_analyzer_eval(p->p->p == 0); // expected-warning{{TRUE}}
+}
Index: test/Analysis/new-ctor-conservative.cpp
===================================================================
--- /dev/null
+++ test/Analysis/new-ctor-conservative.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+
+void clang_analyzer_eval(bool);
+
+struct S {
+ int x;
+ S() : x(1) {}
+ ~S() {}
+};
+
+void checkConstructorInlining() {
+ S *s = new S;
+ clang_analyzer_eval(s->x == 1); // expected-warning{{TRUE}}
+}
Index: test/Analysis/inline.cpp
===================================================================
--- test/Analysis/inline.cpp
+++ test/Analysis/inline.cpp
@@ -315,17 +315,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(true); // expected-warning{{TRUE}}
}
};
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;
}
@@ -335,8 +331,9 @@
clang_analyzer_eval(alias == obj); // expected-warning{{TRUE}}
- // should be TRUE
- clang_analyzer_eval(obj->value == 42); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(obj->value == 42); // expected-warning{{TRUE}}
+ // Because malloc() was never free()d:
+ // expected-warning@-2{{Potential leak of memory pointed to by 'alias'}}
}
}
Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -276,6 +276,14 @@
state = state->BindExpr(CCE, callerCtx, ThisV);
}
+
+ if (const CXXNewExpr *CNE = dyn_cast<CXXNewExpr>(CE)) {
+ // We are currently evaluating a CXXNewAllocator CFGElement. It takes a
+ // while to reach the actual CXXNewExpr element from here, so keep the
+ // region for later use.
+ state = setCXXNewAllocatorValue(state, CNE, calleeCtx->getParent(),
+ state->getSVal(CE, callerCtx));
+ }
}
// Step 3: BindedRetNode -> CleanedNodes
@@ -596,21 +604,30 @@
const CXXConstructorCall &Ctor = cast<CXXConstructorCall>(Call);
+ const CXXConstructExpr *CtorExpr = Ctor.getOriginExpr();
+ const Stmt *ParentExpr = CurLC->getParentMap().getParent(CtorExpr);
+
+ if (ParentExpr && isa<CXXNewExpr>(ParentExpr) &&
+ !Opts.mayInlineCXXAllocator())
+ return CIP_DisallowedOnce;
+
// FIXME: We don't handle constructors or destructors for arrays properly.
// Even once we do, we still need to be careful about implicitly-generated
// initializers for array fields in default move/copy constructors.
+ // We still allow construction into ElementRegion targets when they don't
+ // represent array elements.
const MemRegion *Target = Ctor.getCXXThisVal().getAsRegion();
- 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;
+ if (Target && isa<ElementRegion>(Target)) {
+ if (ParentExpr)
+ if (const CXXNewExpr *NewExpr = dyn_cast<CXXNewExpr>(ParentExpr))
+ if (NewExpr->isArray())
+ return CIP_DisallowedOnce;
+
+ if (const TypedValueRegion *TR = dyn_cast<TypedValueRegion>(
+ cast<SubRegion>(Target)->getSuperRegion()))
+ if (TR->getValueType()->isArrayType())
+ return CIP_DisallowedOnce;
+ }
// Inlining constructors requires including initializers in the CFG.
const AnalysisDeclContext *ADC = CallerSFC->getAnalysisDeclContext();
@@ -629,7 +646,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 || isa<CXXTempObjectRegion>(Target))
return CIP_DisallowedOnce;
break;
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -115,14 +115,25 @@
if (auto Elem = findElementDirectlyInitializedByCurrentConstructor()) {
if (Optional<CFGStmt> StmtElem = Elem->getAs<CFGStmt>()) {
- auto *DS = cast<DeclStmt>(StmtElem->getStmt());
- if (const auto *Var = dyn_cast<VarDecl>(DS->getSingleDecl())) {
- if (Var->getInit() && Var->getInit()->IgnoreImplicit() == CE) {
- SVal LValue = State->getLValue(Var, LCtx);
- QualType Ty = Var->getType();
- LValue = makeZeroElementRegion(State, LValue, Ty);
- return LValue.getAsRegion();
+ if (const CXXNewExpr *CNE = dyn_cast<CXXNewExpr>(StmtElem->getStmt())) {
+ if (AMgr.getAnalyzerOptions().mayInlineCXXAllocator()) {
+ // TODO: Detect when the allocator returns a null pointer.
+ // Constructor shall not be called in this case.
+ if (const MemRegion *MR =
+ getCXXNewAllocatorValue(State, CNE, LCtx).getAsRegion())
+ return MR;
}
+ } else if (auto *DS = dyn_cast<DeclStmt>(StmtElem->getStmt())) {
+ if (const auto *Var = dyn_cast<VarDecl>(DS->getSingleDecl())) {
+ if (Var->getInit() && Var->getInit()->IgnoreImplicit() == CE) {
+ SVal LValue = State->getLValue(Var, LCtx);
+ QualType Ty = Var->getType();
+ LValue = makeZeroElementRegion(State, LValue, Ty);
+ return LValue.getAsRegion();
+ }
+ }
+ } else {
+ llvm_unreachable("Unexpected directly initialized element!");
}
} else if (Optional<CFGInitializer> InitElem = Elem->getAs<CFGInitializer>()) {
const CXXCtorInitializer *Init = InitElem->getInitializer();
@@ -166,6 +177,9 @@
if (isa<DeclStmt>(StmtElem->getStmt())) {
return true;
}
+ if (isa<CXXNewExpr>(StmtElem->getStmt())) {
+ return true;
+ }
}
if (Elem.getKind() == CFGElement::Initializer) {
@@ -455,12 +469,23 @@
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,
+ ExplodedNodeSet DstPostCall;
+ StmtNodeBuilder CallBldr(DstPreCall, DstPostCall, *currBldrCtx);
+ for (auto I : DstPreCall)
+ defaultEvalCall(CallBldr, I, *Call);
+
+ // Store return value of operator new() for future use, until the actual
+ // CXXNewExpr gets processed.
+ ExplodedNodeSet DstPostValue;
+ StmtNodeBuilder ValueBldr(DstPostCall, DstPostValue, *currBldrCtx);
+ for (auto I : DstPostCall) {
+ ProgramStateRef State = I->getState();
+ ValueBldr.generateNode(
+ CNE, I,
+ setCXXNewAllocatorValue(State, CNE, LCtx, State->getSVal(CNE, LCtx)));
+ }
+
+ getCheckerManager().runCheckersForPostCall(Dst, DstPostValue,
*Call, *this);
}
@@ -474,7 +499,7 @@
unsigned blockCount = currBldrCtx->blockCount();
const LocationContext *LCtx = Pred->getLocationContext();
- DefinedOrUnknownSVal symVal = UnknownVal();
+ SVal symVal = UnknownVal();
FunctionDecl *FD = CNE->getOperatorNew();
bool IsStandardGlobalOpNewFunction = false;
@@ -490,26 +515,37 @@
IsStandardGlobalOpNewFunction = (FD->getNumParams() == 1);
}
+ ProgramStateRef State = Pred->getState();
+
+ // Retrieve the stored operator new() return value.
+ if (AMgr.getAnalyzerOptions().mayInlineCXXAllocator()) {
+ symVal = getCXXNewAllocatorValue(State, CNE, LCtx);
+ State = clearCXXNewAllocatorValue(State, CNE, LCtx);
+ }
+
// We assume all standard global 'operator new' functions allocate memory in
// heap. We realize this is an approximation that might not correctly model
// a custom global allocator.
- if (IsStandardGlobalOpNewFunction)
- symVal = svalBuilder.getConjuredHeapSymbolVal(CNE, LCtx, blockCount);
- else
- symVal = svalBuilder.conjureSymbolVal(nullptr, CNE, LCtx, CNE->getType(),
- blockCount);
+ if (symVal.isUnknown()) {
+ if (IsStandardGlobalOpNewFunction)
+ symVal = svalBuilder.getConjuredHeapSymbolVal(CNE, LCtx, blockCount);
+ else
+ symVal = svalBuilder.conjureSymbolVal(nullptr, 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 (!AMgr.getAnalyzerOptions().mayInlineCXXAllocator()) {
+ // 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
@@ -522,7 +558,8 @@
QualType Ty = FD->getType();
if (const FunctionProtoType *ProtoType = Ty->getAs<FunctionProtoType>())
if (!ProtoType->isNothrow(getContext()))
- State = State->assume(symVal, true);
+ if (auto dSymVal = symVal.getAs<DefinedOrUnknownSVal>())
+ State = State->assume(*dSymVal, true);
}
StmtNodeBuilder Bldr(Pred, Dst, *currBldrCtx);
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -63,6 +63,20 @@
REGISTER_TRAIT_WITH_PROGRAMSTATE(InitializedTemporariesSet,
llvm::ImmutableSet<CXXBindTemporaryContext>)
+typedef llvm::ImmutableMap<std::pair<const CXXNewExpr *,
+ const LocationContext *>, SVal>
+ CXXNewAllocatorValuesTy;
+
+// Keeps track of return values of various operator new() calls between
+// evaluation of the inlined operator new(), through the constructor call,
+// to the actual evaluation of the CXXNewExpr.
+// TODO: Refactor the key for this trait into a LocationContext sub-class,
+// which would be put on the stack of location contexts before operator new()
+// is evaluated, and removed from the stack when the whole CXXNewExpr
+// is fully evaluated.
+// Probably do something similar to the previous trait as well.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(CXXNewAllocatorValues, CXXNewAllocatorValuesTy)
+
//===----------------------------------------------------------------------===//
// Engine construction and deletion.
//===----------------------------------------------------------------------===//
@@ -308,6 +322,28 @@
return State;
}
+ProgramStateRef
+ExprEngine::setCXXNewAllocatorValue(ProgramStateRef State,
+ const CXXNewExpr *CNE,
+ const LocationContext *CallerLC, SVal V) {
+ assert(!State->get<CXXNewAllocatorValues>(std::make_pair(CNE, CallerLC)) &&
+ "Allocator value already set!");
+ return State->set<CXXNewAllocatorValues>(std::make_pair(CNE, CallerLC), V);
+}
+
+SVal ExprEngine::getCXXNewAllocatorValue(ProgramStateRef State,
+ const CXXNewExpr *CNE,
+ const LocationContext *CallerLC) {
+ return *State->get<CXXNewAllocatorValues>(std::make_pair(CNE, CallerLC));
+}
+
+ProgramStateRef
+ExprEngine::clearCXXNewAllocatorValue(ProgramStateRef State,
+ const CXXNewExpr *CNE,
+ const LocationContext *CallerLC) {
+ return State->remove<CXXNewAllocatorValues>(std::make_pair(CNE, CallerLC));
+}
+
//===----------------------------------------------------------------------===//
// Top-level transfer function logic (Dispatcher).
//===----------------------------------------------------------------------===//
@@ -429,6 +465,13 @@
const StackFrameContext *SFC = LC ? LC->getCurrentStackFrame() : nullptr;
SymbolReaper SymReaper(SFC, ReferenceStmt, SymMgr, getStoreManager());
+ for (auto I : CleanedState->get<CXXNewAllocatorValues>()) {
+ if (SymbolRef Sym = I.second.getAsSymbol())
+ SymReaper.markLive(Sym);
+ if (const MemRegion *MR = I.second.getAsRegion())
+ SymReaper.markElementIndicesLive(MR);
+ }
+
getCheckerManager().runCheckersForLiveSymbols(CleanedState, SymReaper);
// Create a state in which dead bindings are removed from the environment
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===================================================================
--- include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -642,8 +642,8 @@
const CXXConstructExpr *findDirectConstructorForCurrentCFGElement();
/// For a CXXConstructExpr, walk forward in the current CFG block to find the
- /// CFGElement for the DeclStmt or CXXInitCtorInitializer for which is
- /// directly constructed by this constructor. Returns None if the current
+ /// CFGElement for the DeclStmt or CXXInitCtorInitializer or CXXNewExpr which
+ /// is directly constructed by this constructor. Returns None if the current
/// constructor expression did not directly construct into an existing
/// region.
Optional<CFGElement> findElementDirectlyInitializedByCurrentConstructor();
@@ -655,6 +655,24 @@
/// if not.
const MemRegion *getRegionForConstructedObject(const CXXConstructExpr *CE,
ExplodedNode *Pred);
+
+ /// Store the region returned by operator new() so that the constructor
+ /// that follows it knew what location to initialize. The value should be
+ /// cleared once the respective CXXNewExpr CFGStmt element is processed.
+ static ProgramStateRef
+ setCXXNewAllocatorValue(ProgramStateRef State, const CXXNewExpr *CNE,
+ const LocationContext *CallerLC, SVal V);
+
+ /// Retrieve the location returned by the current operator new().
+ static SVal
+ getCXXNewAllocatorValue(ProgramStateRef State, const CXXNewExpr *CNE,
+ const LocationContext *CallerLC);
+
+ /// Clear the location returned by the respective operator new(). This needs
+ /// to be done as soon as CXXNewExpr CFG block is evaluated.
+ static ProgramStateRef
+ clearCXXNewAllocatorValue(ProgramStateRef State, const CXXNewExpr *CNE,
+ const LocationContext *CallerLC);
};
/// Traits for storing the call processing policy inside GDM.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
