[clang] 4ff836a - [analyzer] Pass correct bldrCtx to computeObjectUnderConstruction
Tomasz Kamiński via cfe-commits
cfe-commits at lists.llvm.org
Mon Sep 26 02:45:47 PDT 2022
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
More information about the cfe-commits
mailing list