r349233 - Revert "[analyzer] MoveChecker: Add checks for dereferencing a smart pointer..."
Artem Dergachev via cfe-commits
cfe-commits at lists.llvm.org
Fri Dec 14 18:55:55 PST 2018
Author: dergachev
Date: Fri Dec 14 18:55:55 2018
New Revision: 349233
URL: http://llvm.org/viewvc/llvm-project?rev=349233&view=rev
Log:
Revert "[analyzer] MoveChecker: Add checks for dereferencing a smart pointer..."
This reverts commit r349226.
Fails on an MSVC buildbot.
Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/MoveChecker.cpp
cfe/trunk/test/Analysis/use-after-move.cpp
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MoveChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MoveChecker.cpp?rev=349233&r1=349232&r2=349233&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MoveChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MoveChecker.cpp Fri Dec 14 18:55:55 2018
@@ -61,35 +61,19 @@ public:
const char *NL, const char *Sep) const override;
private:
- enum MisuseKind { MK_FunCall, MK_Copy, MK_Move, MK_Dereference };
- enum StdObjectKind { SK_NonStd, SK_Unsafe, SK_Safe, SK_SmartPtr };
-
- static bool misuseCausesCrash(MisuseKind MK) {
- return MK == MK_Dereference;
- }
+ enum MisuseKind { MK_FunCall, MK_Copy, MK_Move };
struct ObjectKind {
- // Is this a local variable or a local rvalue reference?
- bool IsLocal : 1;
- // Is this an STL object? If so, of what kind?
- StdObjectKind StdKind : 2;
- };
-
- // STL smart pointers are automatically re-initialized to null when moved
- // from. So we can't warn on many methods, but we can warn when it is
- // dereferenced, which is UB even if the resulting lvalue never gets read.
- const llvm::StringSet<> StdSmartPtrClasses = {
- "shared_ptr",
- "unique_ptr",
- "weak_ptr",
+ bool Local : 1; // Is this a local variable or a local rvalue reference?
+ bool STL : 1; // Is this an object of a standard type?
};
// Not all of these are entirely move-safe, but they do provide *some*
// guarantees, and it means that somebody is using them after move
// in a valid manner.
- // TODO: We can still try to identify *unsafe* use after move,
- // like we did with smart pointers.
- const llvm::StringSet<> StdSafeClasses = {
+ // TODO: We can still try to identify *unsafe* use after move, such as
+ // dereference of a moved-from smart pointer (which is guaranteed to be null).
+ const llvm::StringSet<> StandardMoveSafeClasses = {
"basic_filebuf",
"basic_ios",
"future",
@@ -98,8 +82,11 @@ private:
"promise",
"shared_future",
"shared_lock",
+ "shared_ptr",
"thread",
+ "unique_ptr",
"unique_lock",
+ "weak_ptr",
};
// Should we bother tracking the state of the object?
@@ -110,25 +97,10 @@ private:
// their user to re-use the storage. The latter is possible because STL
// objects are known to end up in a valid but unspecified state after the
// move and their state-reset methods are also known, which allows us to
- // predict precisely when use-after-move is invalid.
- // Some STL objects are known to conform to additional contracts after move,
- // so they are not tracked. However, smart pointers specifically are tracked
- // because we can perform extra checking over them.
- // In aggressive mode, warn on any use-after-move because the user has
- // intentionally asked us to completely eliminate use-after-move
- // in his code.
- return IsAggressive || OK.IsLocal
- || OK.StdKind == SK_Unsafe || OK.StdKind == SK_SmartPtr;
- }
-
- // Some objects only suffer from some kinds of misuses, but we need to track
- // them anyway because we cannot know in advance what misuse will we find.
- bool shouldWarnAbout(ObjectKind OK, MisuseKind MK) const {
- // Additionally, only warn on smart pointers when they are dereferenced (or
- // local or we are aggressive).
- return shouldBeTracked(OK) &&
- (IsAggressive || OK.IsLocal
- || OK.StdKind != SK_SmartPtr || MK == MK_Dereference);
+ // predict precisely when use-after-move is invalid. In aggressive mode,
+ // warn on any use-after-move because the user has intentionally asked us
+ // to completely eliminate use-after-move in his code.
+ return IsAggressive || OK.Local || OK.STL;
}
// Obtains ObjectKind of an object. Because class declaration cannot always
@@ -136,17 +108,17 @@ private:
ObjectKind classifyObject(const MemRegion *MR, const CXXRecordDecl *RD) const;
// Classifies the object and dumps a user-friendly description string to
- // the stream.
- void explainObject(llvm::raw_ostream &OS, const MemRegion *MR,
- const CXXRecordDecl *RD, MisuseKind MK) const;
+ // the stream. Return value is equivalent to classifyObject.
+ ObjectKind explainObject(llvm::raw_ostream &OS,
+ const MemRegion *MR, const CXXRecordDecl *RD) const;
- bool belongsTo(const CXXRecordDecl *RD, const llvm::StringSet<> &Set) const;
+ bool isStandardMoveSafeClass(const CXXRecordDecl *RD) const;
class MovedBugVisitor : public BugReporterVisitor {
public:
- MovedBugVisitor(const MoveChecker &Chk, const MemRegion *R,
- const CXXRecordDecl *RD, MisuseKind MK)
- : Chk(Chk), Region(R), RD(RD), MK(MK), Found(false) {}
+ MovedBugVisitor(const MoveChecker &Chk,
+ const MemRegion *R, const CXXRecordDecl *RD)
+ : Chk(Chk), Region(R), RD(RD), Found(false) {}
void Profile(llvm::FoldingSetNodeID &ID) const override {
static int X = 0;
@@ -168,8 +140,6 @@ private:
const MemRegion *Region;
// The class of the tracked object.
const CXXRecordDecl *RD;
- // How exactly the object was misused.
- const MisuseKind MK;
bool Found;
};
@@ -262,37 +232,18 @@ MoveChecker::MovedBugVisitor::VisitNode(
SmallString<128> Str;
llvm::raw_svector_ostream OS(Str);
- ObjectKind OK = Chk.classifyObject(Region, RD);
- switch (OK.StdKind) {
- case SK_SmartPtr:
- if (MK == MK_Dereference) {
- OS << "Smart pointer";
- Chk.explainObject(OS, Region, RD, MK);
- OS << " is reset to null when moved from";
- break;
- }
-
- // If it's not a dereference, we don't care if it was reset to null
- // or that it is even a smart pointer.
- LLVM_FALLTHROUGH;
- case SK_NonStd:
- case SK_Safe:
- OS << "Object";
- Chk.explainObject(OS, Region, RD, MK);
- OS << " is moved";
- break;
- case SK_Unsafe:
- OS << "Object";
- Chk.explainObject(OS, Region, RD, MK);
- OS << " is left in a valid but unspecified state after move";
- break;
- }
+ OS << "Object";
+ ObjectKind OK = Chk.explainObject(OS, Region, RD);
+ if (OK.STL)
+ OS << " is left in a valid but unspecified state after move";
+ else
+ OS << " is moved";
// Generate the extra diagnostic.
PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
N->getLocationContext());
return std::make_shared<PathDiagnosticEventPiece>(Pos, OS.str(), true);
-}
+ }
const ExplodedNode *MoveChecker::getMoveLocation(const ExplodedNode *N,
const MemRegion *Region,
@@ -316,48 +267,25 @@ void MoveChecker::modelUse(ProgramStateR
CheckerContext &C) const {
assert(!C.isDifferent() && "No transitions should have been made by now");
const RegionState *RS = State->get<TrackedRegionMap>(Region);
- ObjectKind OK = classifyObject(Region, RD);
-
- // Just in case: if it's not a smart pointer but it does have operator *,
- // we shouldn't call the bug a dereference.
- if (MK == MK_Dereference && OK.StdKind != SK_SmartPtr)
- MK = MK_FunCall;
- if (!RS || !shouldWarnAbout(OK, MK)
+ if (!RS || isAnyBaseRegionReported(State, Region)
|| isInMoveSafeContext(C.getLocationContext())) {
// Finalize changes made by the caller.
C.addTransition(State);
return;
}
- // Don't report it in case if any base region is already reported.
- // But still generate a sink in case of UB.
- // And still finalize changes made by the caller.
- if (isAnyBaseRegionReported(State, Region)) {
- if (misuseCausesCrash(MK)) {
- C.generateSink(State, C.getPredecessor());
- } else {
- C.addTransition(State);
- }
- return;
- }
-
ExplodedNode *N = reportBug(Region, RD, C, MK);
- // If the program has already crashed on this path, don't bother.
- if (N->isSink())
- return;
-
State = State->set<TrackedRegionMap>(Region, RegionState::getReported());
C.addTransition(State, N);
}
ExplodedNode *MoveChecker::reportBug(const MemRegion *Region,
- const CXXRecordDecl *RD, CheckerContext &C,
+ const CXXRecordDecl *RD,
+ CheckerContext &C,
MisuseKind MK) const {
- if (ExplodedNode *N = misuseCausesCrash(MK) ? C.generateErrorNode()
- : C.generateNonFatalErrorNode()) {
-
+ if (ExplodedNode *N = C.generateNonFatalErrorNode()) {
if (!BT)
BT.reset(new BugType(this, "Use-after-move",
"C++ move semantics"));
@@ -376,28 +304,24 @@ ExplodedNode *MoveChecker::reportBug(con
switch(MK) {
case MK_FunCall:
OS << "Method called on moved-from object";
- explainObject(OS, Region, RD, MK);
+ explainObject(OS, Region, RD);
break;
case MK_Copy:
OS << "Moved-from object";
- explainObject(OS, Region, RD, MK);
+ explainObject(OS, Region, RD);
OS << " is copied";
break;
case MK_Move:
OS << "Moved-from object";
- explainObject(OS, Region, RD, MK);
+ explainObject(OS, Region, RD);
OS << " is moved";
break;
- case MK_Dereference:
- OS << "Dereference of null smart pointer";
- explainObject(OS, Region, RD, MK);
- break;
}
auto R =
llvm::make_unique<BugReport>(*BT, OS.str(), N, LocUsedForUniqueing,
MoveNode->getLocationContext()->getDecl());
- R->addVisitor(llvm::make_unique<MovedBugVisitor>(*this, Region, RD, MK));
+ R->addVisitor(llvm::make_unique<MovedBugVisitor>(*this, Region, RD));
C.emitReport(std::move(R));
return N;
}
@@ -509,10 +433,9 @@ bool MoveChecker::isInMoveSafeContext(co
return false;
}
-bool MoveChecker::belongsTo(const CXXRecordDecl *RD,
- const llvm::StringSet<> &Set) const {
+bool MoveChecker::isStandardMoveSafeClass(const CXXRecordDecl *RD) const {
const IdentifierInfo *II = RD->getIdentifier();
- return II && Set.count(II->getName());
+ return II && StandardMoveSafeClasses.count(II->getName());
}
MoveChecker::ObjectKind
@@ -522,23 +445,18 @@ MoveChecker::classifyObject(const MemReg
// For the purposes of this checker, we classify move-safe STL types
// as not-"STL" types, because that's how the checker treats them.
MR = unwrapRValueReferenceIndirection(MR);
- bool IsLocal =
- MR && isa<VarRegion>(MR) && isa<StackSpaceRegion>(MR->getMemorySpace());
-
- if (!RD || !RD->getDeclContext()->isStdNamespace())
- return { IsLocal, SK_NonStd };
-
- if (belongsTo(RD, StdSmartPtrClasses))
- return { IsLocal, SK_SmartPtr };
-
- if (belongsTo(RD, StdSafeClasses))
- return { IsLocal, SK_Safe };
-
- return { IsLocal, SK_Unsafe };
+ return {
+ /*Local=*/
+ MR && isa<VarRegion>(MR) && isa<StackSpaceRegion>(MR->getMemorySpace()),
+ /*STL=*/
+ RD && RD->getDeclContext()->isStdNamespace() &&
+ !isStandardMoveSafeClass(RD)
+ };
}
-void MoveChecker::explainObject(llvm::raw_ostream &OS, const MemRegion *MR,
- const CXXRecordDecl *RD, MisuseKind MK) const {
+MoveChecker::ObjectKind
+MoveChecker::explainObject(llvm::raw_ostream &OS, const MemRegion *MR,
+ const CXXRecordDecl *RD) const {
// We may need a leading space every time we actually explain anything,
// and we never know if we are to explain anything until we try.
if (const auto DR =
@@ -546,22 +464,11 @@ void MoveChecker::explainObject(llvm::ra
const auto *RegionDecl = cast<NamedDecl>(DR->getDecl());
OS << " '" << RegionDecl->getNameAsString() << "'";
}
-
ObjectKind OK = classifyObject(MR, RD);
- switch (OK.StdKind) {
- case SK_NonStd:
- case SK_Safe:
- break;
- case SK_SmartPtr:
- if (MK != MK_Dereference)
- break;
-
- // We only care about the type if it's a dereference.
- LLVM_FALLTHROUGH;
- case SK_Unsafe:
- OS << " of type '" << RD->getQualifiedNameAsString() << "'";
- break;
- };
+ if (OK.STL) {
+ OS << " of type '" << RD->getQualifiedNameAsString() << "'";
+ }
+ return OK;
}
void MoveChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const {
@@ -636,11 +543,6 @@ void MoveChecker::checkPreCall(const Cal
C.addTransition(State);
return;
}
-
- if (OOK == OO_Star || OOK == OO_Arrow) {
- modelUse(State, ThisRegion, RD, MK_Dereference, C);
- return;
- }
}
modelUse(State, ThisRegion, RD, MK_FunCall, C);
Modified: cfe/trunk/test/Analysis/use-after-move.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/use-after-move.cpp?rev=349233&r1=349232&r2=349233&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/use-after-move.cpp (original)
+++ cfe/trunk/test/Analysis/use-after-move.cpp Fri Dec 14 18:55:55 2018
@@ -1,26 +1,20 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\
// RUN: -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
-// RUN: -analyzer-config exploration_strategy=unexplored_first_queue\
-// RUN: -analyzer-checker debug.ExprInspection
+// RUN: -analyzer-config exploration_strategy=unexplored_first_queue
// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\
// RUN: -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
-// RUN: -analyzer-config exploration_strategy=dfs -DDFS=1\
-// RUN: -analyzer-checker debug.ExprInspection
+// RUN: -analyzer-config exploration_strategy=dfs -DDFS=1
// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\
// RUN: -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
// RUN: -analyzer-config exploration_strategy=unexplored_first_queue\
-// RUN: -analyzer-config alpha.cplusplus.Move:Aggressive=true -DAGGRESSIVE\
-// RUN: -analyzer-checker debug.ExprInspection
+// RUN: -analyzer-config alpha.cplusplus.Move:Aggressive=true -DAGGRESSIVE
// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.Move -verify %s\
// RUN: -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
// RUN: -analyzer-config exploration_strategy=dfs -DDFS=1\
-// RUN: -analyzer-config alpha.cplusplus.Move:Aggressive=true -DAGGRESSIVE\
-// RUN: -analyzer-checker debug.ExprInspection
+// RUN: -analyzer-config alpha.cplusplus.Move:Aggressive=true -DAGGRESSIVE
#include "Inputs/system-header-simulator-cxx.h"
-void clang_analyzer_warnIfReached();
-
class B {
public:
B() = default;
@@ -816,19 +810,7 @@ class HasSTLField {
// expected-note at -4{{Object 'P' is moved}}
// expected-note at -4{{Method called on moved-from object 'P'}}
#endif
-
- // Because that well-defined state is null, dereference is still UB.
- // Note that in aggressive mode we already warned about 'P',
- // so no extra warning is generated.
- *P += 1;
-#ifndef AGGRESSIVE
- // expected-warning at -2{{Dereference of null smart pointer 'P' of type 'std::unique_ptr'}}
- // expected-note at -14{{Smart pointer 'P' of type 'std::unique_ptr' is reset to null when moved from}}
- // expected-note at -4{{Dereference of null smart pointer 'P' of type 'std::unique_ptr'}}
-#endif
-
- // The program should have crashed by now.
- clang_analyzer_warnIfReached(); // no-warning
+ *P += 1; // FIXME: Should warn that the pointer is null.
}
};
@@ -845,9 +827,3 @@ void localUniquePtr(std::unique_ptr<int>
P.get(); // expected-warning{{Method called on moved-from object 'P'}}
// expected-note at -1{{Method called on moved-from object 'P'}}
}
-
-void localUniquePtrWithArrow(std::unique_ptr<A> P) {
- std::unique_ptr<A> Q = std::move(P); // expected-note{{Smart pointer 'P' of type 'std::unique_ptr' is reset to null when moved from}}
- P->foo(); // expected-warning{{Dereference of null smart pointer 'P' of type 'std::unique_ptr'}}
- // expected-note at -1{{Dereference of null smart pointer 'P' of type 'std::unique_ptr'}}
-}
More information about the cfe-commits
mailing list