NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet,
rnkovacs.
Herald added subscribers: cfe-commits, Szelethus, mikhail.ramalho,
baloghadamsoftware.
Despite the effort to eliminate the need for
`skipRValueSubobjectAdjustments()`, we still encounter ASTs that require it
from time to time, for example in https://bugs.llvm.org/show_bug.cgi?id=37578
One way of properly modeling such expressions would be to include the
member-expression "adjustment" into the //construction context// of the
temporary, but that's not something i'm planning to do, because such ASTs are
rare and seem to be only becoming more rare over time, so for now i'm just
fixing the old code.
The root cause of the problem in this example is that while evaluating the
`MemberExpr` in
`-MemberExpr 0x7fd5ef0035b8 <col:9, col:13> 'a::(anonymous struct at
test.cpp:2:3)' . 0x7fd5ee06a068
`-CXXTemporaryObjectExpr 0x7fd5ef001f70 <col:9, col:11> 'a' 'void ()
noexcept' zeroing
there's no way for `createTemporaryRegionIfNeeded()` to communicate the newly
created temporary region through the Environment (as it usually does), because
all expressions so far have been prvalues.
The current code works around that problem by binding the region to the
`CXXTemporaryObjectExpr`, which is of course a bad thing to do because we
should not bind `Loc`s to prvalue expressions, and it leads to a crash when
eventually this bad binding propagates to the Store and the Store is unable to
load it.
The solution is to bind the correct [lazy compound] value to the
`CXXTemporaryObjectExpr` and then communicate the region to the caller directly
via an out-parameter.
Repository:
rC Clang
https://reviews.llvm.org/D50855
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
lib/StaticAnalyzer/Core/ExprEngine.cpp
test/Analysis/temporaries.cpp
Index: test/Analysis/temporaries.cpp
===================================================================
--- test/Analysis/temporaries.cpp
+++ test/Analysis/temporaries.cpp
@@ -1152,3 +1152,23 @@
// and the non-definition decl should be found by direct lookup.
void T::foo(C) {}
} // namespace argument_virtual_decl_lookup
+
+namespace union_indirect_field_crash {
+union U {
+ struct {
+ int x;
+ };
+};
+
+template <typename T> class C {
+public:
+ void foo() const {
+ (void)(true ? U().x : 0);
+ }
+};
+
+void test() {
+ C<int> c;
+ c.foo();
+}
+} // namespace union_indirect_field_crash
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -283,11 +283,10 @@
return state;
}
-ProgramStateRef
-ExprEngine::createTemporaryRegionIfNeeded(ProgramStateRef State,
- const LocationContext *LC,
- const Expr *InitWithAdjustments,
- const Expr *Result) {
+ProgramStateRef ExprEngine::createTemporaryRegionIfNeeded(
+ ProgramStateRef State, const LocationContext *LC,
+ const Expr *InitWithAdjustments, const Expr *Result,
+ const SubRegion **OutRegionWithAdjustments) {
// FIXME: This function is a hack that works around the quirky AST
// we're often having with respect to C++ temporaries. If only we modelled
// the actual execution order of statements properly in the CFG,
@@ -297,8 +296,11 @@
if (!Result) {
// If we don't have an explicit result expression, we're in "if needed"
// mode. Only create a region if the current value is a NonLoc.
- if (!InitValWithAdjustments.getAs<NonLoc>())
+ if (!InitValWithAdjustments.getAs<NonLoc>()) {
+ if (OutRegionWithAdjustments)
+ *OutRegionWithAdjustments = nullptr;
return State;
+ }
Result = InitWithAdjustments;
} else {
// We need to create a region no matter what. For sanity, make sure we don't
@@ -418,11 +420,16 @@
// The result expression would now point to the correct sub-region of the
// newly created temporary region. Do this last in order to getSVal of Init
// correctly in case (Result == Init).
- State = State->BindExpr(Result, LC, Reg);
+ if (Result->isGLValue())
+ State = State->BindExpr(Result, LC, Reg);
+ else
+ State = State->BindExpr(Result, LC, InitValWithAdjustments);
// Notify checkers once for two bindLoc()s.
State = processRegionChange(State, TR, LC);
+ if (OutRegionWithAdjustments)
+ *OutRegionWithAdjustments = cast<SubRegion>(Reg.getAsRegion());
return State;
}
@@ -2533,8 +2540,11 @@
}
// Handle regular struct fields / member variables.
- state = createTemporaryRegionIfNeeded(state, LCtx, BaseExpr);
- SVal baseExprVal = state->getSVal(BaseExpr, LCtx);
+ const SubRegion *MR;
+ state =
+ createTemporaryRegionIfNeeded(state, LCtx, BaseExpr, nullptr, &MR);
+ SVal baseExprVal =
+ MR ? loc::MemRegionVal(MR) : state->getSVal(BaseExpr, LCtx);
const auto *field = cast<FieldDecl>(Member);
SVal L = state->getLValue(field, baseExprVal);
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===================================================================
--- include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -734,10 +734,14 @@
///
/// If \p Result is provided, the new region will be bound to this expression
/// instead of \p InitWithAdjustments.
- ProgramStateRef createTemporaryRegionIfNeeded(ProgramStateRef State,
- const LocationContext *LC,
- const Expr *InitWithAdjustments,
- const Expr *Result = nullptr);
+ ///
+ /// Returns the temporary region with adjustments into the optional
+ /// OutRegionWithAdjustments out-parameter if a new region was indeed needed,
+ /// otherwise sets it to nullptr.
+ ProgramStateRef createTemporaryRegionIfNeeded(
+ ProgramStateRef State, const LocationContext *LC,
+ const Expr *InitWithAdjustments, const Expr *Result = nullptr,
+ const SubRegion **OutRegionWithAdjustments = nullptr);
/// Returns a region representing the first element of a (possibly
/// multi-dimensional) array, for the purposes of element construction or
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
