NoQ updated this revision to Diff 125850.
NoQ added a comment.
Replaced the live expression hack with a slightly better approach. It doesn't
update the live variables analysis to take `CFGNewAllocator` into account, but
at least tests now pass.
In order to keep the return value produced by the `operator new()` call around
until `CXXNewExpr` is evaluated, i added a program state trait,
`CXXNewAllocatorValueStack`:
1. Upon evaluating `CFGNewAllocator`, the return `SVal` of the evaluated
allocator call is put here;
2. Upon evaluating `CXXConstructExpr`, that return value is retrieved from here;
3. Upon evaluating `CXXNewExpr`, the return value is retrieved from here again
and then wiped.
In order to support nested allocator calls, this state trait is organized as a
stack/FIFO, with push in `CFGNewAllocator` and pop in `CXXNewExpr`. The
`llvm::ImmutableList` thing offers some asserts for warning us when we popped
more times than we pushed; we might want to add more asserts here to detect
other mismatches, but i don't see a need for implementing this as an
environment-like map from (`Expr`, `LocationContext`) to SVal.
Why `SVal` and not `MemRegion`? Because i believe that ideally we want to
produce constructor calls with null or undefined `this`-arguments. Constructors
into null pointers should be skipped - this is how `operator new(std::nothrow)`
works, for instance. Constructors into garbage pointers should be immediately
caught by the checkers (to throw a warning and generate a sink), but we still
need to produce them so that the checkers could catch them. But for now
`CXXConstructorCall` has room only for one pointer, which is currently `const
MemRegion *`, so this still remains to be tackled.
Also we need to make sure that not only the expression lives, but also its
value lives, with all the various traits attached to it. Hence the new facility
in `ExprEngine::removeDead()` to mark the values in `CXXNewAllocatorValueStack`
as live. Test included in `new-ctor-inlined.cpp` (the one with `s != 0`) covers
this situation.
Some doxygen comments updated.
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
Index: test/Analysis/new-ctor-inlined.cpp
===================================================================
--- /dev/null
+++ test/Analysis/new-ctor-inlined.cpp
@@ -0,0 +1,27 @@
+// 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 foo() {
+ S *s = new S;
+ clang_analyzer_eval(s != 0); // expected-warning{{TRUE}}
+ clang_analyzer_eval(s->x == 1); // 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 foo() {
+ 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;
}
Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -276,6 +276,13 @@
state = state->BindExpr(CCE, callerCtx, ThisV);
}
+
+ if (isa<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 = pushCXXNewAllocatorValue(state, state->getSVal(CE, callerCtx));
+ }
}
// Step 3: BindedRetNode -> CleanedNodes
@@ -596,21 +603,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 +645,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
@@ -107,22 +107,36 @@
const MemRegion *
ExprEngine::getRegionForConstructedObject(const CXXConstructExpr *CE,
ExplodedNode *Pred) {
+ // TODO: We can only construct a CXXConstructorCall with a region.
+ // It means that we cannot handle construction into null or garbage pointers.
+ // Such cosntructors need to be handled by checkers to ensure that a warning
+ // is displayed to the user and that analysis doesn't explore such paths
+ // any further. However, in order to act on such events, checker still need
+ // to receive them, so we need to be able to return non-regions from here.
const LocationContext *LCtx = Pred->getLocationContext();
ProgramStateRef State = Pred->getState();
// See if we're constructing an existing region by looking at the next
// element in the CFG.
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 (isa<CXXNewExpr>(StmtElem->getStmt())) {
+ if (AMgr.getAnalyzerOptions().mayInlineCXXAllocator())
+ if (const MemRegion *MR =
+ peekCXXNewAllocatorValue(State).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 +180,9 @@
if (isa<DeclStmt>(StmtElem->getStmt())) {
return true;
}
+ if (isa<CXXNewExpr>(StmtElem->getStmt())) {
+ return true;
+ }
}
if (Elem.getKind() == CFGElement::Initializer) {
@@ -452,12 +469,22 @@
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, pushCXXNewAllocatorValue(State, State->getSVal(CNE, LCtx)));
+ }
+
+ getCheckerManager().runCheckersForPostCall(Dst, DstPostValue,
*Call, *this);
}
@@ -471,7 +498,7 @@
unsigned blockCount = currBldrCtx->blockCount();
const LocationContext *LCtx = Pred->getLocationContext();
- DefinedOrUnknownSVal symVal = UnknownVal();
+ SVal symVal = UnknownVal();
FunctionDecl *FD = CNE->getOperatorNew();
bool IsStandardGlobalOpNewFunction = false;
@@ -487,16 +514,24 @@
IsStandardGlobalOpNewFunction = (FD->getNumParams() == 1);
}
+ ProgramStateRef State = Pred->getState();
+
+ // Retrieve the stored operator new() return value.
+ if (AMgr.getAnalyzerOptions().mayInlineCXXAllocator())
+ symVal = peekCXXNewAllocatorValue(State);
+ State = popCXXNewAllocatorValue(State);
+
// 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);
@@ -519,7 +554,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
@@ -62,6 +62,11 @@
// functions do not interfere.
REGISTER_TRAIT_WITH_PROGRAMSTATE(InitializedTemporariesSet,
llvm::ImmutableSet<CXXBindTemporaryContext>)
+// 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.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(CXXNewAllocatorValueStack,
+ llvm::ImmutableList<SVal>);
//===----------------------------------------------------------------------===//
// Engine construction and deletion.
@@ -308,6 +313,20 @@
return State;
}
+ProgramStateRef ExprEngine::pushCXXNewAllocatorValue(ProgramStateRef State,
+ SVal V) {
+ return State->add<CXXNewAllocatorValueStack>(V);
+}
+
+SVal ExprEngine::peekCXXNewAllocatorValue(ProgramStateRef State) {
+ return State->get<CXXNewAllocatorValueStack>().getHead();
+}
+
+ProgramStateRef ExprEngine::popCXXNewAllocatorValue(ProgramStateRef State) {
+ return State->set<CXXNewAllocatorValueStack>(
+ State->get<CXXNewAllocatorValueStack>().getTail());
+}
+
//===----------------------------------------------------------------------===//
// Top-level transfer function logic (Dispatcher).
//===----------------------------------------------------------------------===//
@@ -429,6 +448,13 @@
const StackFrameContext *SFC = LC ? LC->getCurrentStackFrame() : nullptr;
SymbolReaper SymReaper(SFC, ReferenceStmt, SymMgr, getStoreManager());
+ for (auto I: CleanedState->get<CXXNewAllocatorValueStack>()) {
+ if (SymbolRef Sym = I.getAsSymbol())
+ SymReaper.markLive(Sym);
+ if (const MemRegion *MR = I.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,19 @@
/// 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.
+ ProgramStateRef pushCXXNewAllocatorValue(ProgramStateRef State,
+ SVal V);
+
+ /// Retrieve the location returned by the current operator new().
+ SVal peekCXXNewAllocatorValue(ProgramStateRef State);
+
+ /// Clear the location returned by the respective operator new(). This needs
+ /// to be done as soon as CXXNewExpr CFG block is evaluated.
+ ProgramStateRef popCXXNewAllocatorValue(ProgramStateRef State);
};
/// 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
