NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet,
rnkovacs, Szelethus.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho,
baloghadamsoftware, mgorny.
NoQ edited the summary of this revision.
NoQ added a dependency: D54372: [analyzer] MisusedMovedObject: NFC: Remove dead
code after D18860.
I'd like to try to enable the use-after-move checker developed heroically by
Peter "@szepet" Szecsi by default. While currently being a good opt-in lint
check for enforcing coding style, currently it has too many false positives for
an on-by-default check. The false positives seem to concentrate around objects
that are fine to be used after they were moved from, even if no "state reset"
method has been called on them, simply because their respective
move-constructor/move-assignment leaves the object that is being moved in a
well-specified state (eg., a null smart pointer or an empty set). This is not
the case for STL objects, but it is the case for many custom objects.
I'll proceed to post a few patches and i'd love to hear your thoughts! The plan
(of which this patch implements the first step) is as follows:
1. Rename the checker. In our tradition, "XChecker" usually means a checker
that checks if "X" is used correctly, not a checker that finds "X". Eg.,
MallocChecker warns on misuse of malloc()/free(), but we don't call it
"MisusedMallocChecker". The proposed name is "MoveChecker", i.e. checker that
checks moves.
2. Address false positives under an on-by-default checker option. The proposed
idea is to only warn by default in the following three cases:
- If the object is a known STL object - because we know that it's left in
unspecified state after move and we can memorize all state reset methods.
- If the object is a local (or parameter) variable and hence there's no
temptation to re-use the storage instead of making another local variable.
- If the object is taken from a local (or parameter) rvalue reference, for
the same reason.
3. Make warning messages more specific based information provided by facilities
introduced on step 2.
4. Add a few missing state-reset methods.
5. Enable the checker, as restricted on step 2, by default. Keep the aggressive
variant around in case someone needs it, but i don't have any specific plans
for it.
Repository:
rC Clang
https://reviews.llvm.org/D54556
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp
lib/StaticAnalyzer/Checkers/MoveChecker.cpp
test/Analysis/MisusedMovedObject.cpp
test/Analysis/use-after-move.cpp
Index: test/Analysis/use-after-move.cpp
===================================================================
--- test/Analysis/use-after-move.cpp
+++ test/Analysis/use-after-move.cpp
@@ -1,5 +1,9 @@
-// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.cplusplus.MisusedMovedObject -std=c++11 -verify -analyzer-output=text -analyzer-config exploration_strategy=unexplored_first_queue,eagerly-assume=false %s
-// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.cplusplus.MisusedMovedObject -std=c++11 -analyzer-config exploration_strategy=dfs,eagerly-assume=false -verify -analyzer-output=text -DDFS=1 %s
+// 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: %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
namespace std {
Index: lib/StaticAnalyzer/Checkers/MoveChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/MoveChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MoveChecker.cpp
@@ -1,4 +1,4 @@
-// MisusedMovedObjectChecker.cpp - Check use of moved-from objects. - C++ -===//
+// MoveChecker.cpp - Check use of moved-from objects. - C++ ---------------===//
//
// The LLVM Compiler Infrastructure
//
@@ -42,7 +42,7 @@
void Profile(llvm::FoldingSetNodeID &ID) const { ID.AddInteger(K); }
};
-class MisusedMovedObjectChecker
+class MoveChecker
: public Checker<check::PreCall, check::PostCall,
check::DeadSymbols, check::RegionChanges> {
public:
@@ -116,9 +116,8 @@
}
std::shared_ptr<PathDiagnosticPiece>
-MisusedMovedObjectChecker::MovedBugVisitor::VisitNode(const ExplodedNode *N,
- BugReporterContext &BRC,
- BugReport &) {
+MoveChecker::MovedBugVisitor::VisitNode(const ExplodedNode *N,
+ BugReporterContext &BRC, BugReport &) {
// We need only the last move of the reported object's region.
// The visitor walks the ExplodedGraph backwards.
if (Found)
@@ -156,8 +155,9 @@
return std::make_shared<PathDiagnosticEventPiece>(Pos, InfoText, true);
}
-const ExplodedNode *MisusedMovedObjectChecker::getMoveLocation(
- const ExplodedNode *N, const MemRegion *Region, CheckerContext &C) const {
+const ExplodedNode *MoveChecker::getMoveLocation(const ExplodedNode *N,
+ const MemRegion *Region,
+ CheckerContext &C) const {
// Walk the ExplodedGraph backwards and find the first node that referred to
// the tracked region.
const ExplodedNode *MoveNode = N;
@@ -172,10 +172,9 @@
return MoveNode;
}
-ExplodedNode *MisusedMovedObjectChecker::reportBug(const MemRegion *Region,
- const CallEvent &Call,
- CheckerContext &C,
- MisuseKind MK) const {
+ExplodedNode *MoveChecker::reportBug(const MemRegion *Region,
+ const CallEvent &Call, CheckerContext &C,
+ MisuseKind MK) const {
if (ExplodedNode *N = C.generateNonFatalErrorNode()) {
if (!BT)
BT.reset(new BugType(this, "Usage of a 'moved-from' object",
@@ -217,8 +216,8 @@
return nullptr;
}
-void MisusedMovedObjectChecker::checkPostCall(const CallEvent &Call,
- CheckerContext &C) const {
+void MoveChecker::checkPostCall(const CallEvent &Call,
+ CheckerContext &C) const {
const auto *AFC = dyn_cast<AnyFunctionCall>(&Call);
if (!AFC)
return;
@@ -265,8 +264,7 @@
C.addTransition(State);
}
-bool MisusedMovedObjectChecker::isMoveSafeMethod(
- const CXXMethodDecl *MethodDec) const {
+bool MoveChecker::isMoveSafeMethod(const CXXMethodDecl *MethodDec) const {
// We abandon the cases where bool/void/void* conversion happens.
if (const auto *ConversionDec =
dyn_cast_or_null<CXXConversionDecl>(MethodDec)) {
@@ -282,8 +280,7 @@
MethodDec->getName().lower() == "isempty"));
}
-bool MisusedMovedObjectChecker::isStateResetMethod(
- const CXXMethodDecl *MethodDec) const {
+bool MoveChecker::isStateResetMethod(const CXXMethodDecl *MethodDec) const {
if (!MethodDec)
return false;
if (MethodDec->hasAttr<ReinitializesAttr>())
@@ -299,8 +296,7 @@
// Don't report an error inside a move related operation.
// We assume that the programmer knows what she does.
-bool MisusedMovedObjectChecker::isInMoveSafeContext(
- const LocationContext *LC) const {
+bool MoveChecker::isInMoveSafeContext(const LocationContext *LC) const {
do {
const auto *CtxDec = LC->getDecl();
auto *CtorDec = dyn_cast_or_null<CXXConstructorDecl>(CtxDec);
@@ -315,8 +311,7 @@
return false;
}
-void MisusedMovedObjectChecker::checkPreCall(const CallEvent &Call,
- CheckerContext &C) const {
+void MoveChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const {
ProgramStateRef State = C.getState();
const LocationContext *LC = C.getLocationContext();
ExplodedNode *N = nullptr;
@@ -420,8 +415,8 @@
C.addTransition(State, N);
}
-void MisusedMovedObjectChecker::checkDeadSymbols(SymbolReaper &SymReaper,
- CheckerContext &C) const {
+void MoveChecker::checkDeadSymbols(SymbolReaper &SymReaper,
+ CheckerContext &C) const {
ProgramStateRef State = C.getState();
TrackedRegionMapTy TrackedRegions = State->get<TrackedRegionMap>();
for (TrackedRegionMapTy::value_type E : TrackedRegions) {
@@ -436,7 +431,7 @@
C.addTransition(State);
}
-ProgramStateRef MisusedMovedObjectChecker::checkRegionChanges(
+ProgramStateRef MoveChecker::checkRegionChanges(
ProgramStateRef State, const InvalidatedSymbols *Invalidated,
ArrayRef<const MemRegion *> ExplicitRegions,
ArrayRef<const MemRegion *> Regions, const LocationContext *LCtx,
@@ -456,10 +451,8 @@
return State;
}
-void MisusedMovedObjectChecker::printState(raw_ostream &Out,
- ProgramStateRef State,
- const char *NL,
- const char *Sep) const {
+void MoveChecker::printState(raw_ostream &Out, ProgramStateRef State,
+ const char *NL, const char *Sep) const {
TrackedRegionMapTy RS = State->get<TrackedRegionMap>();
@@ -475,6 +468,6 @@
}
}
}
-void ento::registerMisusedMovedObjectChecker(CheckerManager &mgr) {
- mgr.registerChecker<MisusedMovedObjectChecker>();
+void ento::registerMoveChecker(CheckerManager &mgr) {
+ mgr.registerChecker<MoveChecker>();
}
Index: lib/StaticAnalyzer/Checkers/CMakeLists.txt
===================================================================
--- lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -51,7 +51,7 @@
MallocOverflowSecurityChecker.cpp
MallocSizeofChecker.cpp
MmapWriteExecChecker.cpp
- MisusedMovedObjectChecker.cpp
+ MoveChecker.cpp
MPI-Checker/MPIBugReporter.cpp
MPI-Checker/MPIChecker.cpp
MPI-Checker/MPIFunctionClassifier.cpp
Index: include/clang/StaticAnalyzer/Checkers/Checkers.td
===================================================================
--- include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -300,9 +300,8 @@
HelpText<"Check for use of iterators of different containers where iterators "
"of the same container are expected">;
-def MisusedMovedObjectChecker: Checker<"MisusedMovedObject">,
- HelpText<"Method calls on a moved-from object and copying a moved-from "
- "object will be reported">;
+def MoveChecker: Checker<"Move">,
+ HelpText<"Find use-after-move bugs in C++">;
def UninitializedObjectChecker: Checker<"UninitializedObject">,
HelpText<"Reports uninitialized fields after object construction">;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
