[analyzer] Do not conjure a symbol for return value of a conservatively evaluated function

[NoQ]

Summary

Instead, return a nonloc::LazyCompoundVal of a temporary region for the call expression, with a trivial store in which the temporary region is invalidated.

Returning a conjured record-type symbol is painful because the return value of the function may suddenly change during materialization of a temporary - it will be hotfixed into the same lazy compound value anyway in some cases, eg. in createTemporaryRegionIfNeeded when handling a MaterializeTemporaryExpr. After this patch, we should be receiving the same temporary region during materialization, instead of constructing a new region, and materialization of a conjured structure starts doing a lot less things.

I'm including a change in the yet-to-land iterator checker (D25660), which should or should not be committed depending on the order of patches landing. This change demonstrates how the problem explained above causes checkers to hack around.

In fact, there are other places in which we conjure structure-type symbols, eg. some cases of array invalidations. They aren't known to cause problems though.

[baloghadamsoftware]

Could you please remove the IteratorPastEndChecker file differences from this patch and make D25660 dependent on this one?

[zaks.anna]

Could you please remove the IteratorPastEndChecker file differences from this patch and make D25660
dependent on this one?

Since this has not been reviewed yet while the IteratorPastEndChecker has been, I do not think we should block the checker on this.

[baloghadamsoftware]

Any progress regarding this patch? Is D26837 necessarily a dependency, or we just need isCompoundType() function? This function could be moved to a separate patch so the dependency could be removed.

[zaks.anna]

From what I recall, it is not clear that this patch is the step in the right direction. At least, it need more investigation.

[NoQ]

Yep, because there are a lot more places in which the value of the expression suddenly changes, i think i'd want to either fix all of them, or (if i find some of those really reasonable) make a callback for checkers to subscribe to value replacements (which i'd prefer to treat as a trivial but definite step into alpha-renaming support).

[NoQ]

Outdated by D44131.

lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp

public:
   bool operator!=(const IteratorComparison &X) const {
     return Left != X.Left || Right != X.Right || Equality != X.Equality;
   }
   void Profile(llvm::FoldingSetNodeID &ID) const { ID.AddInteger(Equality); }
 };
 
 class IteratorPastEndChecker
     : public Checker<
-          check::PreCall, check::PostCall, check::PreStmt<CXXOperatorCallExpr>,
-          check::PostStmt<CXXConstructExpr>, check::PostStmt<DeclStmt>,
-          check::PostStmt<MaterializeTemporaryExpr>, check::BeginFunction,
-          check::DeadSymbols, eval::Assume, eval::Call> {
+          check::PreCall, check::PostCall, check::PostStmt<CXXConstructExpr>,
+          check::PostStmt<DeclStmt>, check::PostStmt<MaterializeTemporaryExpr>,
+          check::BeginFunction, check::DeadSymbols, eval::Assume, eval::Call> {
   mutable IdentifierInfo *II_std = nullptr, *II_find = nullptr,
                          *II_find_end = nullptr, *II_find_first_of = nullptr,
                          *II_find_if = nullptr, *II_find_if_not = nullptr,
                          *II_lower_bound = nullptr, *II_upper_bound = nullptr,
                          *II_search = nullptr, *II_search_n = nullptr;
 
   std::unique_ptr<BugType> PastEndBugType;
 
class IteratorPastEndChecker
   void reportPastEndBug(const StringRef &Message, const SVal &Val,
                         CheckerContext &C, ExplodedNode *ErrNode) const;
 
 public:
   IteratorPastEndChecker();
 
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
   void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
-  void checkPreStmt(const CXXOperatorCallExpr *COCE, CheckerContext &C) const;
   void checkBeginFunction(CheckerContext &C) const;
   void checkPostStmt(const CXXConstructExpr *CCE, CheckerContext &C) const;
   void checkPostStmt(const DeclStmt *DS, CheckerContext &C) const;
   void checkPostStmt(const MaterializeTemporaryExpr *MTE,
                      CheckerContext &C) const;
   void checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const;
   ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
                              bool Assumption) const;
if (Func->isOverloadedOperator()) {
     if (!isEndCall(Func))
       return;
     if (!isIteratorType(Call.getResultType()))
       return;
     handleEnd(C, Call.getReturnValue());
   }
 }
 
-void IteratorPastEndChecker::checkPreStmt(const CXXOperatorCallExpr *COCE,
-                                          CheckerContext &C) const {
-  const auto *ThisExpr = COCE->getArg(0);
-
-  auto State = C.getState();
-  const auto *LCtx = C.getPredecessor()->getLocationContext();
-
-  const auto CurrentThis = State->getSVal(ThisExpr, LCtx);
-  if (const auto *Reg = CurrentThis.getAsRegion()) {
-    if (!Reg->getAs<CXXTempObjectRegion>())
-      return;
-    const auto OldState = C.getPredecessor()->getFirstPred()->getState();
-    const auto OldThis = OldState->getSVal(ThisExpr, LCtx);
-    const auto *Pos = getIteratorPosition(OldState, OldThis);
-    if (!Pos)
-      return;
-    State = setIteratorPosition(State, CurrentThis, *Pos);
-    C.addTransition(State);
-  }
-}
-
 void IteratorPastEndChecker::checkBeginFunction(CheckerContext &C) const {
   auto State = C.getState();
   const auto *LCtx = C.getLocationContext();
 
   const auto *Site = cast<StackFrameContext>(LCtx)->getCallSite();
   if (!Site)
     return;
 

lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

if (const ObjCMethodCall *Msg = dyn_cast<ObjCMethodCall>(&Call)) {
 
     return State->BindExpr(E, LCtx, ThisV);
   }
 
   // Conjure a symbol if the return value is unknown.
   QualType ResultTy = Call.getResultType();
   SValBuilder &SVB = getSValBuilder();
   unsigned Count = currBldrCtx->blockCount();
-  SVal R = SVB.conjureSymbolVal(nullptr, E, LCtx, ResultTy, Count);
+  SVal R;
+
+  if (NonLoc::isCompoundType(ResultTy)) {
+    // Return a (lazy) compound value for compound types.
+    MemRegionManager &MemMgr = SVB.getRegionManager();
+    StoreManager &StoreMgr = SVB.getStateManager().getStoreManager();
+
+    const CXXTempObjectRegion *TR = MemMgr.getCXXTempObjectRegion(E, LCtx);
+
+    StoreRef EmptyStore = StoreMgr.getInitialStore(LCtx);
+
+    InvalidatedSymbols IS;
+    RegionAndSymbolInvalidationTraits ITraits;
+    ITraits.setTrait(TR, RegionAndSymbolInvalidationTraits::TK_SuppressEscape);
+    StoreRef ConjuredStore = StoreMgr.invalidateRegions(
+        EmptyStore.getStore(), loc::MemRegionVal(TR), E, Count, LCtx, nullptr,
+        IS, ITraits, nullptr, nullptr);
+
+    R = SVB.makeLazyCompoundVal(ConjuredStore, TR);
+  } else {
+    R = SVB.conjureSymbolVal(nullptr, E, LCtx, ResultTy, Count);
+  }
   return State->BindExpr(E, LCtx, R);
 }
 
 // Conservatively evaluate call by invalidating regions and binding
 // a conjured return value.
 void ExprEngine::conservativeEvalCall(const CallEvent &Call, NodeBuilder &Bldr,
                                       ExplodedNode *Pred,
                                       ProgramStateRef State) {

test/Analysis/explain-svals.cpp

void test_5(int i) {
     clang_analyzer_explain(&x[i]); // expected-warning-re{{{{^pointer to element of type 'int' with index 'argument 'i'' of field 'x' of 'this' object$}}}}
     clang_analyzer_explain(__builtin_alloca(i)); // expected-warning-re{{{{^pointer to region allocated by '__builtin_alloca\(i\)'$}}}}
   }
 };
 } // end of anonymous namespace
 
 void test_6() {
   clang_analyzer_explain(conjure_S()); // expected-warning-re{{{{^lazily frozen compound value of temporary object constructed at statement 'conjure_S\(\)'$}}}}
-  clang_analyzer_explain(conjure_S().z); // expected-warning-re{{{{^value derived from \(symbol of type 'struct S' conjured at statement 'conjure_S\(\)'\) for field 'z' of temporary object constructed at statement 'conjure_S\(\)'$}}}}
+  clang_analyzer_explain(conjure_S().z); // expected-warning-re{{{{^value derived from \(symbol of type 'int' conjured at statement 'conjure_S\(\)'\) for field 'z' of temporary object constructed at statement 'conjure_S\(\)'$}}}}
 }