[PATCH] D50855: [analyzer] pr37578: Fix lvalue/rvalue problem in field-of-temporary adjustments.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL340977: [CFG] [analyzer] Disable argument construction
contexts for variadic functions. (authored by dergachev, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D50855?vs=162755=163202#toc
Repository:
rL LLVM
https://reviews.llvm.org/D50855
Files:
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
cfe/trunk/test/Analysis/temporaries.cpp
Index: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===
--- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ cfe/trunk/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
Index: cfe/trunk/test/Analysis/temporaries.cpp
===
--- cfe/trunk/test/Analysis/temporaries.cpp
+++ cfe/trunk/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 class C {
+public:
+ void foo() const {
+(void)(true ? U().x : 0);
+ }
+};
+
+void test() {
+ C c;
+ c.foo();
+}
+} // namespace union_indirect_field_crash
Index: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ cfe/trunk/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())
+if (!InitValWithAdjustments.getAs()) {
+ 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,17 @@
// 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(Reg.getAsRegion());
return State;
}
@@ -2532,8 +2540,12 @@
}
// Handle regular struct fields / member variables.
- state = createTemporaryRegionIfNeeded(state, LCtx, BaseExpr);
- SVal baseExprVal = state->getSVal(BaseExpr, LCtx);
+ const SubRegion *MR = nullptr;
+ state =
[PATCH] D50855: [analyzer] pr37578: Fix lvalue/rvalue problem in field-of-temporary adjustments.
NoQ updated this revision to Diff 162755.
NoQ added a comment.
Address comments.
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 class C {
+public:
+ void foo() const {
+(void)(true ? U().x : 0);
+ }
+};
+
+void test() {
+ C 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())
+if (!InitValWithAdjustments.getAs()) {
+ 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,17 @@
// 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(Reg.getAsRegion());
return State;
}
@@ -2532,8 +2540,12 @@
}
// Handle regular struct fields / member variables.
- state = createTemporaryRegionIfNeeded(state, LCtx, BaseExpr);
- SVal baseExprVal = state->getSVal(BaseExpr, LCtx);
+ const SubRegion *MR = nullptr;
+ state = createTemporaryRegionIfNeeded(state, LCtx, BaseExpr,
+/*Result=*/nullptr,
+/*OutRegionWithAdjustments=*/);
+ SVal baseExprVal =
+ MR ? loc::MemRegionVal(MR) : state->getSVal(BaseExpr, LCtx);
const auto *field = cast(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
___
[PATCH] D50855: [analyzer] pr37578: Fix lvalue/rvalue problem in field-of-temporary adjustments.
george.karpenkov requested changes to this revision. george.karpenkov added a comment. This revision now requires changes to proceed. looks good apart from minor nits Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h:744 + const Expr *InitWithAdjustments, const Expr *Result = nullptr, + const SubRegion **OutRegionWithAdjustments = nullptr); In general I would say that pair is still preferable, but I agree that with pre-c++17 codebase it gets too verbose. Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:423 // correctly in case (Result == Init). - State = State->BindExpr(Result, LC, Reg); + if (Result->isGLValue()) +State = State->BindExpr(Result, LC, Reg); could we have braces here? Once we have "else" I would say we should have those. Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:2543 // 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, Let's initialize that to nullptr https://reviews.llvm.org/D50855 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50855: [analyzer] pr37578: Fix lvalue/rvalue problem in field-of-temporary adjustments.
NoQ added a comment. ...and adding the aforementioned assertion for prvalues increases the number of crashes on tests to 196. It seems that the analyzer assigns values of improper loc-less very often, but then immediately overwrites them :/ I wonder how much performance could be gained by fixing these bugs. https://reviews.llvm.org/D50855 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50855: [analyzer] pr37578: Fix lvalue/rvalue problem in field-of-temporary adjustments.
NoQ added a comment. > which is of course a bad thing to do because we should not bind `Loc`s to > `prvalue` expressions ... of non-pointer type. On the other hand, binding `NonLoc`s to glvalue expressions is always a bad thing to do; but, for the reference, adding this as an assertion currently crashes 92 tests. https://reviews.llvm.org/D50855 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50855: [analyzer] pr37578: Fix lvalue/rvalue problem in field-of-temporary adjustments.
NoQ updated this revision to Diff 161073.
NoQ added a comment.
Add comments for optional arguments.
The reason why i chose an out-parameter rather than returning a std::pair is
because most callers //presumably// don't need this feature.
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 class C {
+public:
+ void foo() const {
+(void)(true ? U().x : 0);
+ }
+};
+
+void test() {
+ C 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())
+if (!InitValWithAdjustments.getAs()) {
+ 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(Reg.getAsRegion());
return State;
}
@@ -2533,8 +2540,12 @@
}
// 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,
+/*Result=*/nullptr,
+/*OutRegionWithAdjustments=*/);
+ SVal baseExprVal =
+ MR ? loc::MemRegionVal(MR) : state->getSVal(BaseExpr, LCtx);
const auto *field = cast(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
[PATCH] D50855: [analyzer] pr37578: Fix lvalue/rvalue problem in field-of-temporary adjustments.
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 'a::(anonymous struct at
test.cpp:2:3)' . 0x7fd5ee06a068
`-CXXTemporaryObjectExpr 0x7fd5ef001f70 '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 class C {
+public:
+ void foo() const {
+(void)(true ? U().x : 0);
+ }
+};
+
+void test() {
+ C 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())
+if (!InitValWithAdjustments.getAs()) {
+ 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(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, );
+ SVal baseExprVal =
+ MR ? loc::MemRegionVal(MR) :
