[analyzer] Pass correct bldrCtx to computeObjectUnderConstruction

[tomasz-kaminski-sonarsource]

Summary

In case when the prvalue is returned from the function (kind is one
of SimpleReturnedValueKind, CXX17ElidedCopyReturnedValueKind),
then it construction happens in context of the caller.
We pass BldrCtx explicitly, as currBldrCtx will always refer to callee
context.

In the following example:

struct Result {int value; };
Result create() { return Result{10}; }
int accessValue(Result r) { return r.value; }

void test() {
   for (int i = 0; i < 2; ++i)
      accessValue(create());
}

In case when the returned object was constructed directly into the
argument to a function call accessValue(create()), this led to
inappropriate value of blockCount being used to locate parameter region,
and as a consequence resulting object (from create()) was constructed
into a different region, that was later read by inlined invocation of
outer function (accessValue).
This manifests itself only in case when calling block is visited more
than once (loop in above example), as otherwise there is no difference
in blockCount value between callee and caller context.
This happens only in case when copy elision is disabled (before C++17).

[tomasz-kaminski-sonarsource]

Fixed warrning checks and added newline.

[tomasz-kaminski-sonarsource]

This is now ready for review.

[tomasz-kaminski-sonarsource]

Fixed formatting.

[NoQ]

Beautiful find, thanks!

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

757–758

You probably want an updated builder context here as well. This function should be a simple wrapper, it should be completely interchangeable with calling both functions directly.

[tomasz-kaminski-sonarsource]

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

757–758

Could you please elaborate more? I would see a reason to create a context here if I would expect that currBlrdCtx refers to a different Block that we want to perform construction in. And there is no indication on another Block being inplay here, and I would construct NodeBlockCtx with same block as currBldrCtx.
In other words, I expect this function to be handeConstructionContext in current Block.

[tomasz-kaminski-sonarsource]

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

757–758

Or to say it differently, I expect BldCtx not being currBldrCtx to be an unusual situation, that is limited to the construction of return value. Thus having it in convenience would only make it more currbesome.

[tomasz-kaminski-sonarsource]

@NoQ Do you agree with my view on handleConstructionContext, if so is this patch ready to land?

[tomasz-kaminski-sonarsource]

Ping.

[NoQ]

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

757–758

No-no, I mean, it's not any concrete bug that I see, it's maintaining API contracts to make the code easy to understand and make it harder to make mistakes in the future.

The function handleConstructionContext() , by design, is supposed to be equivalent to calling computeObjectUnderConstruction() and updateObjectsUnderConstruction(). Whenever someone has to perform both steps, they can combine them together with this convenience wrapper.

After your patch, they won't be able to combine the two calls in a situation when they need to pass a non-current builder context to computeObjectUnderConstruction(). I hope they'll be able to figure out that they need to add another parameter but I'm worried that they may think "oh this big function doesn't need that parameter, let's drop it". Because the bug you've found is quite subtle, it's easy to miss why we even need to pass that parameter.

So I suggest to add the new parameter to handleConstructionContext() as well, and pass it manually on all call sites.

[tomasz-kaminski-sonarsource]

• Pass BldrCtx to handleConstructionContext

[tomasz-kaminski-sonarsource]

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

757–758

Change applied.

[tomasz-kaminski-sonarsource]

Ping and reformat.

[NoQ]

LGTM!

Thanks again for your investigation. And patience!

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

 };
 
 // TODO: Turn into a class.
 struct NodeBuilderContext {
   const CoreEngine &Eng;
   const CFGBlock *Block;
   const LocationContext *LC;
 
+  NodeBuilderContext(const CoreEngine &E, const CFGBlock *B,
+                     const LocationContext *L)
+      : Eng(E), Block(B), LC(L) {
+    assert(B);
+  }
+
   NodeBuilderContext(const CoreEngine &E, const CFGBlock *B, ExplodedNode *N)
-      : Eng(E), Block(B), LC(N->getLocationContext()) { assert(B); }
+      : NodeBuilderContext(E, B, N->getLocationContext()) {}
 
   /// Return the CFGBlock associated with this builder.
   const CFGBlock *getBlock() const { return Block; }
 
   /// Returns the number of times the current basic block has been
   /// visited on the exploded graph path.
   unsigned blockCount() const {
     return Eng.WList->getBlockCounter().getNumVisited(

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

public:
   /// construct an array of objects. In that case it points to the index
   /// of the continuous memory region.
   /// E.g.:
   /// For `int arr[4]` this index can be 0,1,2,3.
   /// For `int arr2[3][3]` this index can be 0,1,...,7,8.
   /// A multi-dimensional array is also a continuous memory location in a
   /// row major order, so for arr[0][0] Idx is 0 and for arr[2][2] Idx is 8.
   SVal computeObjectUnderConstruction(const Expr *E, ProgramStateRef State,
+                                      const NodeBuilderContext *BldrCtx,
                                       const LocationContext *LCtx,
                                       const ConstructionContext *CC,
                                       EvalCallOptions &CallOpts,
                                       unsigned Idx = 0);
 
   /// Update the program state with all the path-sensitive information
   /// that's necessary to perform construction of an object with a given
   /// syntactic construction context. V and CallOpts have to be obtained from
   /// computeObjectUnderConstruction() invoked with the same set of
   /// the remaining arguments (E, State, LCtx, CC).
   ProgramStateRef updateObjectsUnderConstruction(
       SVal V, const Expr *E, ProgramStateRef State, const LocationContext *LCtx,
       const ConstructionContext *CC, const EvalCallOptions &CallOpts);
 
   /// A convenient wrapper around computeObjectUnderConstruction
   /// and updateObjectsUnderConstruction.
-  std::pair<ProgramStateRef, SVal>
-  handleConstructionContext(const Expr *E, ProgramStateRef State,
-                            const LocationContext *LCtx,
-                            const ConstructionContext *CC,
-                            EvalCallOptions &CallOpts, unsigned Idx = 0) {
+  std::pair<ProgramStateRef, SVal> handleConstructionContext(
+      const Expr *E, ProgramStateRef State, const NodeBuilderContext *BldrCtx,
+      const LocationContext *LCtx, const ConstructionContext *CC,
 
-    SVal V = computeObjectUnderConstruction(E, State, LCtx, CC, CallOpts, Idx);
+    SVal V = computeObjectUnderConstruction(E, State, BldrCtx, LCtx, CC,
+                                            CallOpts, Idx);
     State = updateObjectsUnderConstruction(V, E, State, LCtx, CC, CallOpts);
 
     return std::make_pair(State, V);
   }
 
 private:
   ProgramStateRef finishArgumentConstruction(ProgramStateRef State,
                                              const CallEvent &Call);

clang/lib/StaticAnalyzer/Core/CallEvent.cpp

 Optional<SVal>
 CallEvent::getReturnValueUnderConstruction() const {
   const auto *CC = getConstructionContext();
   if (!CC)
     return None;
 
   EvalCallOptions CallOpts;
   ExprEngine &Engine = getState()->getStateManager().getOwningEngine();
-  SVal RetVal =
-    Engine.computeObjectUnderConstruction(getOriginExpr(), getState(),
-                                          getLocationContext(), CC, CallOpts);
+  SVal RetVal = Engine.computeObjectUnderConstruction(
+      getOriginExpr(), getState(), &Engine.getBuilderContext(),
   return RetVal;
 }
 
 ArrayRef<ParmVarDecl*> AnyFunctionCall::parameters() const {
   const FunctionDecl *D = getDecl();
   if (!D)
     return None;
   return D->parameters();

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

if (const ArrayType *AT = Ctx.getAsArrayType(Ty)) {
     }
     LValue = State->getLValue(Ty, SVB.makeArrayIndex(Idx), LValue);
     IsArray = true;
   }
 
   return LValue;
 }
 
+// In case when the prvalue is returned from the function (kind is one of
+// SimpleReturnedValueKind, CXX17ElidedCopyReturnedValueKind), then
+// it's materialization happens in context of the caller.
+// We pass BldrCtx explicitly, as currBldrCtx always refers to callee's context.
 SVal ExprEngine::computeObjectUnderConstruction(
-    const Expr *E, ProgramStateRef State, const LocationContext *LCtx,
-    const ConstructionContext *CC, EvalCallOptions &CallOpts, unsigned Idx) {
+    const Expr *E, ProgramStateRef State, const NodeBuilderContext *BldrCtx,
+    const LocationContext *LCtx, const ConstructionContext *CC,
+    EvalCallOptions &CallOpts, unsigned Idx) {
+
   SValBuilder &SVB = getSValBuilder();
   MemRegionManager &MRMgr = SVB.getRegionManager();
   ASTContext &ACtx = SVB.getContext();
 
   // Compute the target region by exploring the construction context.
   if (CC) {
     switch (CC->getKind()) {
     case ConstructionContext::CXX17ElidedCopyVariableKind:
case ConstructionContext::CXX17ElidedCopyReturnedValueKind: {
           break;
         }
         if (isa<BlockInvocationContext>(CallerLCtx)) {
           // Unwrap block invocation contexts. They're mostly part of
           // the current stack frame.
           CallerLCtx = CallerLCtx->getParent();
           assert(!isa<BlockInvocationContext>(CallerLCtx));
         }
+
+        NodeBuilderContext CallerBldrCtx(getCoreEngine(),
+                                         SFC->getCallSiteBlock(), CallerLCtx);
         return computeObjectUnderConstruction(
-            cast<Expr>(SFC->getCallSite()), State, CallerLCtx,
+            cast<Expr>(SFC->getCallSite()), State, &CallerBldrCtx, CallerLCtx,
             RTC->getConstructionContext(), CallOpts);
       } else {
         // We are on the top frame of the analysis. We do not know where is the
         // object returned to. Conjure a symbolic region for the return value.
         // TODO: We probably need a new MemRegion kind to represent the storage
         // of that SymbolicRegion, so that we cound produce a fancy symbol
         // instead of an anonymous conjured symbol.
         // TODO: Do we need to track the region to avoid having it dead
case ConstructionContext::ElidedTemporaryObjectKind: {
       // also sets the CallOpts flags for us.
       // If the elided copy/move constructor is not supported, there's still
       // benefit in trying to model the non-elided constructor.
       // Stash our state before trying to elide, as it'll get overwritten.
       ProgramStateRef PreElideState = State;
       EvalCallOptions PreElideCallOpts = CallOpts;
 
       SVal V = computeObjectUnderConstruction(
-          TCC->getConstructorAfterElision(), State, LCtx,
+          TCC->getConstructorAfterElision(), State, BldrCtx, LCtx,
           TCC->getConstructionContextAfterElision(), CallOpts);
 
       // FIXME: This definition of "copy elision has not failed" is unreliable.
       // It doesn't indicate that the constructor will actually be inlined
       // later; this is still up to evalCall() to decide.
       if (!CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion)
         return V;
 
case ConstructionContext::ArgumentKind: {
 
       const auto *ACC = cast<ArgumentConstructionContext>(CC);
       const Expr *E = ACC->getCallLikeExpr();
       unsigned Idx = ACC->getIndex();
 
       CallEventManager &CEMgr = getStateManager().getCallEventManager();
       auto getArgLoc = [&](CallEventRef<> Caller) -> Optional<SVal> {
         const LocationContext *FutureSFC =
-            Caller->getCalleeStackFrame(currBldrCtx->blockCount());
+            Caller->getCalleeStackFrame(BldrCtx->blockCount());
         // Return early if we are unable to reliably foresee
         // the future stack frame.
         if (!FutureSFC)
           return None;
 
         // This should be equivalent to Caller->getDecl() for now, but
         // FutureSFC->getDecl() is likely to support better stuff (like
         // virtual functions) earlier.
         const Decl *CalleeD = FutureSFC->getDecl();
 
         // FIXME: Support for variadic arguments is not implemented here yet.
         if (CallEvent::isVariadic(CalleeD))
           return None;
 
         // Operator arguments do not correspond to operator parameters
         // because this-argument is implemented as a normal argument in
         // operator call expressions but not in operator declarations.
         const TypedValueRegion *TVR = Caller->getParameterLocation(
-            *Caller->getAdjustedParameterIndex(Idx), currBldrCtx->blockCount());
+            *Caller->getAdjustedParameterIndex(Idx), BldrCtx->blockCount());
         if (!TVR)
           return None;
 
         return loc::MemRegionVal(TVR);
       };
 
       if (const auto *CE = dyn_cast<CallExpr>(E)) {
         CallEventRef<> Caller = CEMgr.getSimpleCall(CE, State, LCtx);
if (AILE) {
             State, CE, LCtx,
             getContext().getArrayInitLoopExprElementCount(AILE));
 
       State = bindRequiredArrayElementToEnvironment(
           State, AILE, LCtx, svalBuilder.makeArrayIndex(Idx));
     }
 
     // The target region is found from construction context.
-    std::tie(State, Target) =
-        handleConstructionContext(CE, State, LCtx, CC, CallOpts, Idx);
+    std::tie(State, Target) = handleConstructionContext(
+        CE, State, currBldrCtx, LCtx, CC, CallOpts, Idx);
     break;
   }
   case CXXConstructExpr::CK_VirtualBase: {
     // Make sure we are not calling virtual base class initializers twice.
     // Only the most-derived object should initialize virtual base classes.
     const auto *OuterCtor = dyn_cast_or_null<CXXConstructExpr>(
         LCtx->getStackFrame()->getCallSite());
     assert(

clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

ProgramStateRef ExprEngine::bindReturnValue(const CallEvent &Call,
   SVal R;
   QualType ResultTy = Call.getResultType();
   unsigned Count = currBldrCtx->blockCount();
   if (auto RTC = getCurrentCFGElement().getAs<CFGCXXRecordTypedCall>()) {
     // Conjure a temporary if the function returns an object by value.
     SVal Target;
     assert(RTC->getStmt() == Call.getOriginExpr());
     EvalCallOptions CallOpts; // FIXME: We won't really need those.
-    std::tie(State, Target) =
-        handleConstructionContext(Call.getOriginExpr(), State, LCtx,
-                                  RTC->getConstructionContext(), CallOpts);
+    std::tie(State, Target) = handleConstructionContext(
+        Call.getOriginExpr(), State, currBldrCtx, LCtx,
     const MemRegion *TargetR = Target.getAsRegion();
     assert(TargetR);
     // Invalidate the region so that it didn't look uninitialized. If this is
     // a field or element constructor, we do not want to invalidate
     // the whole structure. Pointer escape is meaningless because
     // the structure is a product of conservative evaluation
     // and therefore contains nothing interesting at this point.
     RegionAndSymbolInvalidationTraits ITraits;

clang/test/Analysis/copy-elision.cpp

#if __cplusplus >= 201703L
   #define ELIDE 1
 #else
   #ifndef NO_ELIDE_FLAG
     #define ELIDE 1
   #endif
 #endif
 
 void clang_analyzer_eval(bool);
+void clang_analyzer_dump(int);
 
 namespace variable_functional_cast_crash {
 
 struct A {
   A(int) {}
 };
 
 void foo() {

 void test_copy_elision() {
   Foo *z;
   // If the copy elided, 'z' points to 'tmp', otherwise it's a dangling pointer.
   Foo tmp = make1(&z);
   (void)tmp;
 }
 
 } // namespace address_vector_tests
+
+namespace arg_directly_from_return_in_loop {
+
+struct Result {
+  int value;
+};
+
+Result create() {
+  return Result{10};
+}
+
+int accessValue(Result r) {
+  return r.value;
+}
+
+void test() {
+  for (int i = 0; i < 3; ++i) {
+    int v = accessValue(create());
+    if (i == 0) {
+      clang_analyzer_dump(v); // expected-warning {{10 S32b}}
+    } else {
+      clang_analyzer_dump(v); // expected-warning {{10 S32b}}
+                              // was {{reg_${{[0-9]+}}<int r.value> }} for C++11
+    }
+  }
+}
+
+} // namespace arg_directly_from_return_in_loop