[analyzer] ExprEngine: Do not 'bindLoc' twice for testing purposes

[Charusso]

Summary

As a side effect of D63720 we do not need to test whether we cannot
bindLoc a value since we have already escaped it in
ExprEngine::VisitBinaryOperator() and other places.

[Charusso]

May this is somewhat hand-waving, but this double-binding does not seem useful.

[NoQ]

I suspect that there are a lot more potential drops of symbols that this branch catches. Even if there are currently no tests for them, i'd rather keep it until we completely get rid of unknown values.

I also doubt D63720 had anything to do with this code. In D63720 the information is lost much earlier than the bind happens.

[Charusso]

Okay! Thanks for the review.

clang/lib/StaticAnalyzer/Core/ExprEngine.cpp

void ExprEngine::VisitAtomicExpr(const AtomicExpr *AE, ExplodedNode *Pred,
   getCheckerManager().runCheckersForPostStmt(Dst, AfterInvalidateSet, AE, *this);
 }
 
 // A value escapes in four possible cases:
 // (1) We are binding to something that is not a memory region.
 // (2) We are binding to a MemRegion that does not have stack storage.
 // (3) We are binding to a top-level parameter region with a non-trivial
 //     destructor. We won't see the destructor during analysis, but it's there.
+//
 // (4) We are binding to a MemRegion with stack storage that the store
-//     does not understand.
+//     does not understand. - We handle it in ExprEngine::VisitBinaryOperator().
 ProgramStateRef ExprEngine::processPointerEscapedOnBind(
     ProgramStateRef State, ArrayRef<std::pair<SVal, SVal>> LocAndVals,
     const LocationContext *LCtx, PointerEscapeKind Kind,
     const CallEvent *Call) {
   SmallVector<SVal, 8> Escaped;
   for (const std::pair<SVal, SVal> &LocAndVal : LocAndVals) {
     // Cases (1) and (2).
     const MemRegion *MR = LocAndVal.first.getAsRegion();
     if (!MR || !MR->hasStackStorage()) {
       Escaped.push_back(LocAndVal.second);
       continue;
     }
 
     // Case (3).
     if (const auto *VR = dyn_cast<VarRegion>(MR->getBaseRegion()))
       if (VR->hasStackParametersStorage() && VR->getStackFrame()->inTopFrame())
         if (const auto *RD = VR->getValueType()->getAsCXXRecordDecl())
           if (!RD->hasTrivialDestructor()) {
             Escaped.push_back(LocAndVal.second);
             continue;
           }
-
-    // Case (4): in order to test that, generate a new state with the binding
-    // added. If it is the same state, then it escapes (since the store cannot
-    // represent the binding).
-    // Do this only if we know that the store is not supposed to generate the
-    // same state.
-    SVal StoredVal = State->getSVal(MR);
-    if (StoredVal != LocAndVal.second)
-      if (State ==
-          (State->bindLoc(loc::MemRegionVal(MR), LocAndVal.second, LCtx)))
-        Escaped.push_back(LocAndVal.second);
   }
 
   if (Escaped.empty())
     return State;
 
   return escapeValues(State, Escaped, Kind, Call);
 }
 

clang/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 once for zero-initialization of the structure.
   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
 
 // Make sure there's no further output.
 // CHECK-NOT: Bind
 // CHECK-NOT: RegionChanges