This revision was automatically updated to reflect the committed changes.
Closed by commit rL298924: [analyzer] When creating a temporary object,
properly copy the value into it. (authored by dergachev).
Changed prior to commit:
https://reviews.llvm.org/D30534?vs=92618&id=93250#toc
Repository:
rL LLVM
https://reviews.llvm.org/D30534
Files:
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
cfe/trunk/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
cfe/trunk/test/Analysis/temporaries-callback-order.cpp
cfe/trunk/test/Analysis/temporaries.cpp
Index: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
===================================================================
--- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -182,19 +182,25 @@
ProgramStateRef
ExprEngine::createTemporaryRegionIfNeeded(ProgramStateRef State,
const LocationContext *LC,
- const Expr *Ex,
+ const Expr *InitWithAdjustments,
const Expr *Result) {
- SVal V = State->getSVal(Ex, LC);
+ // FIXME: This function is a hack that works around the quirky AST
+ // we're often having with respect to C++ temporaries. If only we modelled
+ // the actual execution order of statements properly in the CFG,
+ // all the hassle with adjustments would not be necessary,
+ // and perhaps the whole function would be removed.
+ SVal InitValWithAdjustments = State->getSVal(InitWithAdjustments, LC);
if (!Result) {
// If we don't have an explicit result expression, we're in "if needed"
// mode. Only create a region if the current value is a NonLoc.
- if (!V.getAs<NonLoc>())
+ if (!InitValWithAdjustments.getAs<NonLoc>())
return State;
- Result = Ex;
+ Result = InitWithAdjustments;
} else {
// We need to create a region no matter what. For sanity, make sure we don't
// try to stuff a Loc into a non-pointer temporary region.
- assert(!V.getAs<Loc>() || Loc::isLocType(Result->getType()) ||
+ assert(!InitValWithAdjustments.getAs<Loc>() ||
+ Loc::isLocType(Result->getType()) ||
Result->getType()->isMemberPointerType());
}
@@ -226,7 +232,8 @@
SmallVector<const Expr *, 2> CommaLHSs;
SmallVector<SubobjectAdjustment, 2> Adjustments;
- const Expr *Init = Ex->skipRValueSubobjectAdjustments(CommaLHSs, Adjustments);
+ const Expr *Init = InitWithAdjustments->skipRValueSubobjectAdjustments(
+ CommaLHSs, Adjustments);
const TypedValueRegion *TR = nullptr;
if (const MaterializeTemporaryExpr *MT =
@@ -241,6 +248,7 @@
TR = MRMgr.getCXXTempObjectRegion(Init, LC);
SVal Reg = loc::MemRegionVal(TR);
+ SVal BaseReg = Reg;
// Make the necessary adjustments to obtain the sub-object.
for (auto I = Adjustments.rbegin(), E = Adjustments.rend(); I != E; ++I) {
@@ -254,19 +262,47 @@
break;
case SubobjectAdjustment::MemberPointerAdjustment:
// FIXME: Unimplemented.
- State->bindDefault(Reg, UnknownVal(), LC);
+ State = State->bindDefault(Reg, UnknownVal(), LC);
return State;
}
}
- // Try to recover some path sensitivity in case we couldn't compute the value.
- if (V.isUnknown())
- V = getSValBuilder().conjureSymbolVal(Result, LC, TR->getValueType(),
- currBldrCtx->blockCount());
- // Bind the value of the expression to the sub-object region, and then bind
- // the sub-object region to our expression.
- State = State->bindLoc(Reg, V, LC);
+ // What remains is to copy the value of the object to the new region.
+ // FIXME: In other words, what we should always do is copy value of the
+ // Init expression (which corresponds to the bigger object) to the whole
+ // temporary region TR. However, this value is often no longer present
+ // in the Environment. If it has disappeared, we instead invalidate TR.
+ // Still, what we can do is assign the value of expression Ex (which
+ // corresponds to the sub-object) to the TR's sub-region Reg. At least,
+ // values inside Reg would be correct.
+ SVal InitVal = State->getSVal(Init, LC);
+ if (InitVal.isUnknown()) {
+ InitVal = getSValBuilder().conjureSymbolVal(Result, LC, Init->getType(),
+ currBldrCtx->blockCount());
+ State = State->bindLoc(BaseReg.castAs<Loc>(), InitVal, LC, false);
+
+ // Then we'd need to take the value that certainly exists and bind it over.
+ if (InitValWithAdjustments.isUnknown()) {
+ // Try to recover some path sensitivity in case we couldn't
+ // compute the value.
+ InitValWithAdjustments = getSValBuilder().conjureSymbolVal(
+ Result, LC, InitWithAdjustments->getType(),
+ currBldrCtx->blockCount());
+ }
+ State =
+ State->bindLoc(Reg.castAs<Loc>(), InitValWithAdjustments, LC, false);
+ } else {
+ State = State->bindLoc(BaseReg.castAs<Loc>(), InitVal, LC, false);
+ }
+
+ // The result expression would now point to the correct sub-region of the
+ // newly created temporary region. Do this last in order to getSVal of Init
+ // correctly in case (Result == Init).
State = State->BindExpr(Result, LC, Reg);
+
+ // Notify checkers once for two bindLoc()s.
+ State = processRegionChange(State, TR, LC);
+
return State;
}
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
===================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
@@ -24,16 +24,29 @@
namespace {
-class AnalysisOrderChecker : public Checker< check::PreStmt<CastExpr>,
- check::PostStmt<CastExpr>,
- check::PreStmt<ArraySubscriptExpr>,
- check::PostStmt<ArraySubscriptExpr>> {
- bool isCallbackEnabled(CheckerContext &C, StringRef CallbackName) const {
- AnalyzerOptions &Opts = C.getAnalysisManager().getAnalyzerOptions();
+class AnalysisOrderChecker
+ : public Checker<check::PreStmt<CastExpr>,
+ check::PostStmt<CastExpr>,
+ check::PreStmt<ArraySubscriptExpr>,
+ check::PostStmt<ArraySubscriptExpr>,
+ check::Bind,
+ check::RegionChanges> {
+ bool isCallbackEnabled(AnalyzerOptions &Opts, StringRef CallbackName) const {
return Opts.getBooleanOption("*", false, this) ||
Opts.getBooleanOption(CallbackName, false, this);
}
+ bool isCallbackEnabled(CheckerContext &C, StringRef CallbackName) const {
+ AnalyzerOptions &Opts = C.getAnalysisManager().getAnalyzerOptions();
+ return isCallbackEnabled(Opts, CallbackName);
+ }
+
+ bool isCallbackEnabled(ProgramStateRef State, StringRef CallbackName) const {
+ AnalyzerOptions &Opts = State->getStateManager().getOwningEngine()
+ ->getAnalysisManager().getAnalyzerOptions();
+ return isCallbackEnabled(Opts, CallbackName);
+ }
+
public:
void checkPreStmt(const CastExpr *CE, CheckerContext &C) const {
if (isCallbackEnabled(C, "PreStmtCastExpr"))
@@ -47,17 +60,35 @@
<< ")\n";
}
- void checkPreStmt(const ArraySubscriptExpr *SubExpr, CheckerContext &C) const {
+ void checkPreStmt(const ArraySubscriptExpr *SubExpr,
+ CheckerContext &C) const {
if (isCallbackEnabled(C, "PreStmtArraySubscriptExpr"))
llvm::errs() << "PreStmt<ArraySubscriptExpr>\n";
}
- void checkPostStmt(const ArraySubscriptExpr *SubExpr, CheckerContext &C) const {
+ void checkPostStmt(const ArraySubscriptExpr *SubExpr,
+ CheckerContext &C) const {
if (isCallbackEnabled(C, "PostStmtArraySubscriptExpr"))
llvm::errs() << "PostStmt<ArraySubscriptExpr>\n";
}
+
+ void checkBind(SVal Loc, SVal Val, const Stmt *S, CheckerContext &C) const {
+ if (isCallbackEnabled(C, "Bind"))
+ llvm::errs() << "Bind\n";
+ }
+
+ ProgramStateRef
+ checkRegionChanges(ProgramStateRef State,
+ const InvalidatedSymbols *Invalidated,
+ ArrayRef<const MemRegion *> ExplicitRegions,
+ ArrayRef<const MemRegion *> Regions,
+ const LocationContext *LCtx, const CallEvent *Call) const {
+ if (isCallbackEnabled(State, "RegionChanges"))
+ llvm::errs() << "RegionChanges\n";
+ return State;
+ }
};
-}
+} // end anonymous namespace
//===----------------------------------------------------------------------===//
// Registration.
Index: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===================================================================
--- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -621,16 +621,16 @@
void performTrivialCopy(NodeBuilder &Bldr, ExplodedNode *Pred,
const CallEvent &Call);
- /// If the value of the given expression is a NonLoc, copy it into a new
- /// temporary object region, and replace the value of the expression with
- /// that.
+ /// If the value of the given expression \p InitWithAdjustments is a NonLoc,
+ /// copy it into a new temporary object region, and replace the value of the
+ /// expression with that.
///
- /// If \p ResultE is provided, the new region will be bound to this expression
- /// instead of \p E.
+ /// If \p Result is provided, the new region will be bound to this expression
+ /// instead of \p InitWithAdjustments.
ProgramStateRef createTemporaryRegionIfNeeded(ProgramStateRef State,
const LocationContext *LC,
- const Expr *E,
- const Expr *ResultE = nullptr);
+ const Expr *InitWithAdjustments,
+ const Expr *Result = nullptr);
/// For a DeclStmt or CXXInitCtorInitializer, walk backward in the current CFG
/// block to find the constructor expression that directly constructed into
Index: cfe/trunk/test/Analysis/temporaries.cpp
===================================================================
--- cfe/trunk/test/Analysis/temporaries.cpp
+++ cfe/trunk/test/Analysis/temporaries.cpp
@@ -503,3 +503,30 @@
});
}
}
+
+namespace CopyToTemporaryCorrectly {
+class Super {
+public:
+ void m() {
+ mImpl();
+ }
+ virtual void mImpl() = 0;
+};
+class Sub : public Super {
+public:
+ Sub(const int &p) : j(p) {}
+ virtual void mImpl() override {
+ // Used to be undefined pointer dereference because we didn't copy
+ // the subclass data (j) to the temporary object properly.
+ (void)(j + 1); // no-warning
+ if (j != 22) {
+ clang_analyzer_warnIfReached(); // no-warning
+ }
+ }
+ const int &j;
+};
+void run() {
+ int i = 22;
+ Sub(i).m();
+}
+}
Index: cfe/trunk/test/Analysis/temporaries-callback-order.cpp
===================================================================
--- cfe/trunk/test/Analysis/temporaries-callback-order.cpp
+++ cfe/trunk/test/Analysis/temporaries-callback-order.cpp
@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=debug.AnalysisOrder -analyzer-config debug.AnalysisOrder:Bind=true -analyzer-config debug.AnalysisOrder:RegionChanges=true %s 2>&1 | FileCheck %s
+
+struct Super {
+ virtual void m();
+};
+struct Sub : Super {
+ virtual void m() {}
+};
+
+void testTemporaries() {
+ // This triggers RegionChanges twice:
+ // - Once for zero-initialization of the structure.
+ // - Once for creating a temporary region and copying the structure there.
+ // FIXME: This code shouldn't really produce the extra temporary, however
+ // that's how we behave for now.
+ Sub().m();
+}
+
+void seeIfCheckBindWorks() {
+ // This should trigger checkBind. The rest of the code shouldn't.
+ // This also triggers checkRegionChanges after that.
+ // Note that this function is analyzed first, so the messages would be on top.
+ int x = 1;
+}
+
+// seeIfCheckBindWorks():
+// CHECK: Bind
+// CHECK-NEXT: RegionChanges
+
+// testTemporaries():
+// CHECK-NEXT: RegionChanges
+// CHECK-NEXT: RegionChanges
+
+// Make sure there's no further output.
+// CHECK-NOT: Bind
+// CHECK-NOT: RegionChanges
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
