Author: Tomasz Kamiński Date: 2022-09-26T11:39:10+02:00 New Revision: 4ff836a138b40a9fc3430bc08afc1f327e5ed281
URL: https://github.com/llvm/llvm-project/commit/4ff836a138b40a9fc3430bc08afc1f327e5ed281 DIFF: https://github.com/llvm/llvm-project/commit/4ff836a138b40a9fc3430bc08afc1f327e5ed281.diff LOG: [analyzer] Pass correct bldrCtx to computeObjectUnderConstruction 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). Reviewed By: NoQ Differential Revision: https://reviews.llvm.org/D132030 Added: Modified: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h clang/lib/StaticAnalyzer/Core/CallEvent.cpp clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp clang/test/Analysis/copy-elision.cpp Removed: ################################################################################ diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h index 220aee759a935..a595d517cd276 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h @@ -214,8 +214,14 @@ struct NodeBuilderContext { 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; } diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h index a905f9097750d..848e43d15fbff 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h @@ -732,6 +732,7 @@ class ExprEngine { /// 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, @@ -748,13 +749,13 @@ class ExprEngine { /// 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, + EvalCallOptions &CallOpts, unsigned Idx = 0) { - 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); diff --git a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp index 57591960a1401..a8b49adfb4c9a 100644 --- a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp +++ b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp @@ -485,9 +485,9 @@ CallEvent::getReturnValueUnderConstruction() const { EvalCallOptions CallOpts; ExprEngine &Engine = getState()->getStateManager().getOwningEngine(); - SVal RetVal = - Engine.computeObjectUnderConstruction(getOriginExpr(), getState(), - getLocationContext(), CC, CallOpts); + SVal RetVal = Engine.computeObjectUnderConstruction( + getOriginExpr(), getState(), &Engine.getBuilderContext(), + getLocationContext(), CC, CallOpts); return RetVal; } diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp index ae878ecbcc34a..476afc598ac6c 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp @@ -111,9 +111,15 @@ SVal ExprEngine::makeElementRegion(ProgramStateRef State, SVal LValue, 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(); @@ -210,8 +216,11 @@ SVal ExprEngine::computeObjectUnderConstruction( 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 @@ -251,7 +260,7 @@ SVal ExprEngine::computeObjectUnderConstruction( 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. @@ -319,7 +328,7 @@ SVal ExprEngine::computeObjectUnderConstruction( 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) @@ -338,7 +347,7 @@ SVal ExprEngine::computeObjectUnderConstruction( // 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; @@ -643,8 +652,8 @@ void ExprEngine::handleConstructor(const Expr *E, } // 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: { diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp index dbfded29c1ae4..48b5db1eb4a52 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp @@ -774,9 +774,9 @@ ProgramStateRef ExprEngine::bindReturnValue(const CallEvent &Call, 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, + RTC->getConstructionContext(), CallOpts); const MemRegion *TargetR = Target.getAsRegion(); assert(TargetR); // Invalidate the region so that it didn't look uninitialized. If this is diff --git a/clang/test/Analysis/copy-elision.cpp b/clang/test/Analysis/copy-elision.cpp index dee1a5fc86e7f..991f325c05853 100644 --- a/clang/test/Analysis/copy-elision.cpp +++ b/clang/test/Analysis/copy-elision.cpp @@ -20,6 +20,7 @@ #endif void clang_analyzer_eval(bool); +void clang_analyzer_dump(int); namespace variable_functional_cast_crash { @@ -418,3 +419,31 @@ void test_copy_elision() { } } // 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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits