NoQ created this revision.
Herald added subscribers: rnkovacs, eraman.
Under the assumption of `-analyzer-config c++-allocator-inlining=true`, which
enables evaluation of `operator new` as a regular function call, this patch
shows what it takes to actually inline the constructor into the return value of
such operator call.
The CFG for a `new C()` expression looks like:
- `CXXNewAllocator` `new C()` (a special kind of `CFGElement` on which operator
new is being evaluated)
- `CXXConstructExpr` `C()` (a regular `CFGStmt` element on which we call the
constructor)
- `CXXNewExpr` `new C()` (a regular `CFGStmt` element on which we should
ideally have nothing to do)
What i did here is:
1. First, i relax the requirements for constructor regions, as discussed on the
mailing list
(http://lists.llvm.org/pipermail/cfe-dev/2017-November/056095.html). We can now
construct into `ElementRegion` unless it actually represents an array element
(we're not dealing with `operator new[]` yet). Also we remove the explicit
bailout for constructions that correspond to operator new parent expressions,
as long as `-analyzer-config c++-allocator-inlining` is enabled. See changes in
`mayInlineCallKind()`.
2. Then, when the constructor is being modeled, we're trying to get back the
value returned by `operator new`. This is done similarly to other construction
cases - by finding the next (!!) element in the CFG, figuring out that it's a
`CXXNewExpr`, and then taking the respective region to construct into from the
Environment. The `CXXNewAllocator` element is not relied upon on this phase -
for now it only triggers the evaluation of `operator new` at the right moment,
so that we had the return value. See changes in
`getRegionForConstructedObject()` and `canHaveDirectConstructor()`.
3. When we reach the actual `CXXNewExpr` CFG element (the third one), we need
to preserve the value we already have for the `CXXNewExpr` in the environment.
The old code that's conjuring a (heap) pointer here would probably still remain
to handle the case when an inlined `operator new` has actually returned an
`UnknownVal`. Still, this needs polishing, as the FIXME at the top of
`VisitCXXNewExpr()` suggests. See the newly added hack in `VisitCXXNewExpr()`.
4. Finally, the `CXXNewExpr` value keeps dying in the Environment. From the
point of view of the current liveness analysis, the new-expression is dead (or
rather not yet born) until the `CXXNewExpr` statement element is reached, so it
immediately gets purged on every step. The really dirty code that prevents
this, //that should never be committed//, is in `shouldRemoveDeadBindings()`:
for the sake of this proof-of-concept, i've crudely disabled garbage collection
on the respective moments of time. I believe that the proper fix would be on
the liveness analysis side: mark the new-expression as live between the
`CXXNewAllocator` element and `CXXNewExpr` statement element.
My todo list before committing this would be:
1. Fix the live expressions hack.
2. See how reliable is `findElementDirectlyInitializedByCurrentConstructor()`
in this case.
3. Understand how my code works for non-object constructors (eg. `new int`).
4. See how much `VisitCXXNewExpr` can be refactored.
Once this lands, i think we should be looking into enabling `-analyzer-config
c++-allocator-inlining` by default.
https://reviews.llvm.org/D40560
Files:
lib/StaticAnalyzer/Core/ExprEngine.cpp
lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
test/Analysis/inline.cpp
test/Analysis/new-ctor.cpp
Index: test/Analysis/new-ctor.cpp
===================================================================
--- /dev/null
+++ test/Analysis/new-ctor.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
@@ -596,21 +596,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 +638,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
@@ -114,14 +114,21 @@
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 (auto *NE = dyn_cast<CXXNewExpr>(StmtElem->getStmt())) {
+ SVal NewVal = State->getSVal(NE, LCtx);
+ if (const MemRegion *MR = NewVal.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();
@@ -165,6 +172,9 @@
if (isa<DeclStmt>(StmtElem->getStmt())) {
return true;
}
+ if (isa<CXXNewExpr>(StmtElem->getStmt())) {
+ return true;
+ }
}
if (Elem.getKind() == CFGElement::Initializer) {
@@ -456,7 +466,7 @@
unsigned blockCount = currBldrCtx->blockCount();
const LocationContext *LCtx = Pred->getLocationContext();
- DefinedOrUnknownSVal symVal = UnknownVal();
+ SVal symVal = UnknownVal();
FunctionDecl *FD = CNE->getOperatorNew();
bool IsStandardGlobalOpNewFunction = false;
@@ -475,11 +485,17 @@
// 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 (const Expr *Init = CNE->getInitializer())
+ if (isa<CXXConstructExpr>(Init))
+ symVal = Pred->getState()->getSVal(CNE, LCtx);
+
+ 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();
@@ -504,7 +520,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
@@ -389,6 +389,15 @@
if (!isa<Expr>(S.getStmt()))
return true;
+ if (const CXXNewExpr *NE = dyn_cast_or_null<CXXNewExpr>(S.getStmt()))
+ return false;
+
+ if (const CXXConstructExpr *CE =
+ dyn_cast_or_null<CXXConstructExpr>(S.getStmt()))
+ if (const Stmt *Parent = LC->getParentMap().getParent(CE))
+ if (isa<CXXNewExpr>(Parent))
+ return false;
+
// Run before processing a call.
if (CallEvent::isCallStmt(S.getStmt()))
return true;
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
