From 12f7c2baccdfb7da56d018f34f29a954968ead6d Mon Sep 17 00:00:00 2001 From: Artem Dergachev Date: Fri, 14 Dec 2018 20:47:58 +0000 Subject: [PATCH] [analyzer] MoveChecker: Improve invalidation policies. If a moved-from object is passed into a conservatively evaluated function by pointer or by reference, we assume that the function may reset its state. Make sure it doesn't apply to const pointers and const references. Add a test that demonstrates that it does apply to rvalue references. Additionally, make sure that the object is invalidated when its contents change for reasons other than invalidation caused by evaluating a call conservatively. In particular, when the object's fields are manipulated directly, we should assume that some sort of reset may be happening. Differential Revision: https://reviews.llvm.org/D55289 llvm-svn: 349190 --- clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp | 46 ++++++++++++++------- clang/test/Analysis/use-after-move.cpp | 49 ++++++++++++++++++++--- 2 files changed, 75 insertions(+), 20 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp index c93ba9d..dca0cbe 100644 --- a/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp @@ -53,8 +53,8 @@ public: ProgramStateRef checkRegionChanges(ProgramStateRef State, const InvalidatedSymbols *Invalidated, - ArrayRef ExplicitRegions, - ArrayRef Regions, + ArrayRef RequestedRegions, + ArrayRef InvalidatedRegions, const LocationContext *LCtx, const CallEvent *Call) const; void printState(raw_ostream &Out, ProgramStateRef State, const char *NL, const char *Sep) const override; @@ -525,19 +525,35 @@ void MoveChecker::checkDeadSymbols(SymbolReaper &SymReaper, ProgramStateRef MoveChecker::checkRegionChanges( ProgramStateRef State, const InvalidatedSymbols *Invalidated, - ArrayRef ExplicitRegions, - ArrayRef Regions, const LocationContext *LCtx, - const CallEvent *Call) const { - // In case of an InstanceCall don't remove the ThisRegion from the GDM since - // it is handled in checkPreCall and checkPostCall. - const MemRegion *ThisRegion = nullptr; - if (const auto *IC = dyn_cast_or_null(Call)) { - ThisRegion = IC->getCXXThisVal().getAsRegion(); - } - - for (const auto *Region : ExplicitRegions) { - if (ThisRegion != Region) - State = removeFromState(State, Region); + ArrayRef RequestedRegions, + ArrayRef InvalidatedRegions, + const LocationContext *LCtx, const CallEvent *Call) const { + if (Call) { + // Relax invalidation upon function calls: only invalidate parameters + // that are passed directly via non-const pointers or non-const references + // or rvalue references. + // In case of an InstanceCall don't invalidate the this-region since + // it is fully handled in checkPreCall and checkPostCall. + const MemRegion *ThisRegion = nullptr; + if (const auto *IC = dyn_cast(Call)) + ThisRegion = IC->getCXXThisVal().getAsRegion(); + + // Requested ("explicit") regions are the regions passed into the call + // directly, but not all of them end up being invalidated. + // But when they do, they appear in the InvalidatedRegions array as well. + for (const auto *Region : RequestedRegions) { + if (ThisRegion != Region) { + if (llvm::find(InvalidatedRegions, Region) != + std::end(InvalidatedRegions)) { + State = removeFromState(State, Region); + } + } + } + } else { + // For invalidations that aren't caused by calls, assume nothing. In + // particular, direct write into an object's field invalidates the status. + for (const auto *Region : InvalidatedRegions) + State = removeFromState(State, Region->getBaseRegion()); } return State; diff --git a/clang/test/Analysis/use-after-move.cpp b/clang/test/Analysis/use-after-move.cpp index 3134720e..1ef04a0 100644 --- a/clang/test/Analysis/use-after-move.cpp +++ b/clang/test/Analysis/use-after-move.cpp @@ -116,6 +116,19 @@ public: bool empty() const; bool isEmpty() const; operator bool() const; + + void testUpdateField() { + A a; + A b = std::move(a); + a.i = 1; + a.foo(); // no-warning + } + void testUpdateFieldDouble() { + A a; + A b = std::move(a); + a.d = 1.0; + a.foo(); // no-warning + } }; int bignum(); @@ -594,24 +607,50 @@ void paramEvaluateOrderTest() { foobar(a.getI(), std::move(a)); //no-warning } -void not_known(A &a); -void not_known(A *a); +void not_known_pass_by_ref(A &a); +void not_known_pass_by_const_ref(const A &a); +void not_known_pass_by_rvalue_ref(A &&a); +void not_known_pass_by_ptr(A *a); +void not_known_pass_by_const_ptr(const A *a); void regionAndPointerEscapeTest() { { A a; A b; b = std::move(a); - not_known(a); - a.foo(); //no-warning + not_known_pass_by_ref(a); + a.foo(); // no-warning + } + { + A a; + A b; + b = std::move(a); // expected-note{{Object 'a' is moved}} + not_known_pass_by_const_ref(a); + a.foo(); // expected-warning{{Method called on moved-from object 'a'}} + // expected-note@-1{{Method called on moved-from object 'a'}} + } + { + A a; + A b; + b = std::move(a); + not_known_pass_by_rvalue_ref(std::move(a)); + a.foo(); // no-warning } { A a; A b; b = std::move(a); - not_known(&a); + not_known_pass_by_ptr(&a); a.foo(); // no-warning } + { + A a; + A b; + b = std::move(a); // expected-note{{Object 'a' is moved}} + not_known_pass_by_const_ptr(&a); + a.foo(); // expected-warning{{Method called on moved-from object 'a'}} + // expected-note@-1{{Method called on moved-from object 'a'}} + } } // A declaration statement containing multiple declarations sequences the -- 2.7.4