diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h index fa7769bcfc95..1a20074d04d9 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h @@ -621,16 +621,16 @@ private: 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 diff --git a/clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp index e6592a285e47..90d5c0e36a47 100644 --- a/clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp @@ -24,16 +24,29 @@ using namespace ento; namespace { -class AnalysisOrderChecker : public Checker< check::PreStmt, - check::PostStmt, - check::PreStmt, - check::PostStmt> { - bool isCallbackEnabled(CheckerContext &C, StringRef CallbackName) const { - AnalyzerOptions &Opts = C.getAnalysisManager().getAnalyzerOptions(); +class AnalysisOrderChecker + : public Checker, + check::PostStmt, + check::PreStmt, + check::PostStmt, + 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 @@ public: << ")\n"; } - void checkPreStmt(const ArraySubscriptExpr *SubExpr, CheckerContext &C) const { + void checkPreStmt(const ArraySubscriptExpr *SubExpr, + CheckerContext &C) const { if (isCallbackEnabled(C, "PreStmtArraySubscriptExpr")) llvm::errs() << "PreStmt\n"; } - void checkPostStmt(const ArraySubscriptExpr *SubExpr, CheckerContext &C) const { + void checkPostStmt(const ArraySubscriptExpr *SubExpr, + CheckerContext &C) const { if (isCallbackEnabled(C, "PostStmtArraySubscriptExpr")) llvm::errs() << "PostStmt\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 ExplicitRegions, + ArrayRef Regions, + const LocationContext *LCtx, const CallEvent *Call) const { + if (isCallbackEnabled(State, "RegionChanges")) + llvm::errs() << "RegionChanges\n"; + return State; + } }; -} +} // end anonymous namespace //===----------------------------------------------------------------------===// // Registration. diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index a5d8fb7e4813..9e6ec09010e9 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -182,19 +182,25 @@ ProgramStateRef ExprEngine::getInitialState(const LocationContext *InitLoc) { 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()) + if (!InitValWithAdjustments.getAs()) 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::isLocType(Result->getType()) || + assert(!InitValWithAdjustments.getAs() || + Loc::isLocType(Result->getType()) || Result->getType()->isMemberPointerType()); } @@ -226,7 +232,8 @@ ExprEngine::createTemporaryRegionIfNeeded(ProgramStateRef State, SmallVector CommaLHSs; SmallVector 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 @@ ExprEngine::createTemporaryRegionIfNeeded(ProgramStateRef State, 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 @@ ExprEngine::createTemporaryRegionIfNeeded(ProgramStateRef State, 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(), 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(), InitValWithAdjustments, LC, false); + } else { + State = State->bindLoc(BaseReg.castAs(), 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; } diff --git a/clang/test/Analysis/temporaries-callback-order.cpp b/clang/test/Analysis/temporaries-callback-order.cpp new file mode 100644 index 000000000000..df916cc4e767 --- /dev/null +++ b/clang/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 diff --git a/clang/test/Analysis/temporaries.cpp b/clang/test/Analysis/temporaries.cpp index 2c052f8b5159..99851dd68539 100644 --- a/clang/test/Analysis/temporaries.cpp +++ b/clang/test/Analysis/temporaries.cpp @@ -503,3 +503,30 @@ namespace PR32088 { }); } } + +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(); +} +}