[analyzer] When creating a temporary object copy, properly copy the value into it.

[NoQ]

Summary

The test case provided demonstrates a regression due to my earlier attempt to fix temporaries in D26839. Neither before nor after, we did not properly copy the symbolic rvalue of the object to the newly created memory region; in different ways though.

In the test case:

1. there are two classes - Super (smaller) and Sub (bigger);
2. a temporary object of Sub is created;
3. a Sub-specific field not present in Super is initialized during construction;
4. the temporary Sub-class object is casted to Super to call method m() that is declared in Super;
5. a temporary region is retroactively created to represent this-region during method call;
6. value of the Super-object is copied over the c++ base-object region;
7. the rest of the temporary remains uninitialized;
8. m() calls a virtual method mImpl() that resolves to the definition in Sub;
9. mImpl() uses the Sub-specific field initialized on step 3 but not copied on step 6, as explained on step 7;
10. a false warning regarding usage of uninitialized value is thrown.

The problem (apart from step 5 which is the reason for all the hassle, but needs AST fixes) is of course on step 6, where we should have just copied the whole Sub object. But - surprise! - the value of this object is no longer there in the Environment, because the cast on step 4 was already successfully modeled.

I'm uncertain of the proper solution. We could probably prevent objects from disappearing from the Environment upon unchecked-derived-to-base casts, but that'd require performance evaluations on C++-rich codebases.

For now, i propose to invalidate the Sub-object before binding the Super-object. This way at least we don't produce false accusations.

There's also one open question for the case when the value wasn't removed from the environment, but we still need to bind Super - i'm seeing failing tests when i disable binding Super, but i didn't investigate them yet.

[a.sidorin]

Hi Artem! Thank you for this patch. It looks very promising, but I have some questions and remarks.

lib/StaticAnalyzer/Core/ExprEngine.cpp

187

If we are touching names, should we rename Ex to InitWithAdjustments (or smth like this) and ExV correspondingly?

313

Should we do all these operations with ExV/Reg if the InitV is known? There is a FIXME but I think it is related to all this code, not to the bindLoc only. And what happens if we remove this code?

317

Seems like WipeV in comment should be InitV?

318

If I understand correcly, if we call bindLoc(), we call checkRegionChanges() callbacks. And if we bindLoc() twice, we call them twice too. Is this what we want here?

[NoQ]

lib/StaticAnalyzer/Core/ExprEngine.cpp

318

This is actually an excellent question.

[NoQ]

Fix review comments!

Add a callback order test to ensure that checkRegionChanges is called as many (or rather as few) times as necessary.

lib/StaticAnalyzer/Core/ExprEngine.cpp

313

My bad: i overwrote SVal of Result above, and Result might have been equal to Init, which caused conflict between InitV and ExV.

include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h

private:
 
   bool replayWithoutInlining(ExplodedNode *P, const LocationContext *CalleeLC);
 
   /// Models a trivial copy or move constructor or trivial assignment operator
   /// call with a simple bind.
   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
   /// the storage for this statement. Returns null if the constructor for this
   /// statement created a temporary object region rather than directly
   /// constructing into an existing region.
   const CXXConstructExpr *findDirectConstructorForCurrentCFGElement();
 

lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp

 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 
 using namespace clang;
 using namespace ento;
 
 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<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"))
       llvm::errs() << "PreStmt<CastExpr> (Kind : " << CE->getCastKindName()
                    << ")\n";
   }
 
   void checkPostStmt(const CastExpr *CE, CheckerContext &C) const {
     if (isCallbackEnabled(C, "PostStmtCastExpr"))
       llvm::errs() << "PostStmt<CastExpr> (Kind : " << CE->getCastKindName()
                    << ")\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.
 //===----------------------------------------------------------------------===//
 
 void ento::registerAnalysisOrderChecker(CheckerManager &mgr) {
   mgr.registerChecker<AnalysisOrderChecker>();
 }

lib/StaticAnalyzer/Core/ExprEngine.cpp

ProgramStateRef ExprEngine::getInitialState(const LocationContext *InitLoc) {
   }
 
   return state;
 }
 
 ProgramStateRef
 ExprEngine::createTemporaryRegionIfNeeded(ProgramStateRef State,
                                           const LocationContext *LC,
-                                          const Expr *Ex,
+                                          const Expr *InitWithAdjustments,
                                           const Expr *Result) {
-  SVal V = State->getSVal(Ex, LC);
+  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());
   }
 
   ProgramStateManager &StateMgr = State->getStateManager();
   MemRegionManager &MRMgr = StateMgr.getRegionManager();
   StoreManager &StoreMgr = StateMgr.getStoreManager();
 
   // MaterializeTemporaryExpr may appear out of place, after a few field and
ExprEngine::createTemporaryRegionIfNeeded(ProgramStateRef State,
   // Use the usual methods for obtaining the expression of the base object,
   // and record the adjustments that we need to make to obtain the sub-object
   // that the whole expression 'Ex' refers to. This trick is usual,
   // in the sense that CodeGen takes a similar route.
 
   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 =
           dyn_cast<MaterializeTemporaryExpr>(Result)) {
     StorageDuration SD = MT->getStorageDuration();
     // If this object is bound to a reference with static storage duration, we
     // put it in a different region to prevent "address leakage" warnings.
     if (SD == SD_Static || SD == SD_Thread)
       TR = MRMgr.getCXXStaticTempObjectRegion(Init);
   }
   if (!TR)
     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) {
     const SubobjectAdjustment &Adj = *I;
     switch (Adj.Kind) {
     case SubobjectAdjustment::DerivedToBaseAdjustment:
       Reg = StoreMgr.evalDerivedToBase(Reg, Adj.DerivedToBase.BasePath);
       break;
     case SubobjectAdjustment::FieldAdjustment:
       Reg = StoreMgr.getLValueField(Adj.Field, Reg);
       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(),
+  // 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());
-  // 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);
+    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;
 }
 
 //===----------------------------------------------------------------------===//
 // Top-level transfer function logic (Dispatcher).
 //===----------------------------------------------------------------------===//
 
 /// evalAssume - Called by ConstraintManager. Used to call checker-specific
 ///  logic for handling assumptions on symbolic values.
 ProgramStateRef ExprEngine::processAssume(ProgramStateRef state,
                                               SVal cond, bool assumption) {
   return getCheckerManager().runCheckersForEvalAssume(state, cond, assumption);
 }
 
 ProgramStateRef
 ExprEngine::processRegionChanges(ProgramStateRef state,
                                  const InvalidatedSymbols *invalidated,
                                  ArrayRef<const MemRegion *> Explicits,
                                  ArrayRef<const MemRegion *> Regions,
                                  const LocationContext *LCtx,
                                  const CallEvent *Call) {
   return getCheckerManager().runCheckersForRegionChanges(state, invalidated,
                                                          Explicits, Regions,
                                                          LCtx, Call);
 }
 

test/Analysis/temporaries-callback-order.cpp

+// 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

test/Analysis/temporaries.cpp

namespace PR32088 {
   void testReturnFromStmtExprInitializer() {
     // We shouldn't try to destroy the object pointed to by `obj' upon return.
     const NonTrivial &obj = ({
       return; // no-crash
       NonTrivial(42);
     });
   }
 }
+
+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();
+}
+}