From d4ee3d8f37decfb48eef46f6c1da366d90959893 Mon Sep 17 00:00:00 2001
From: Devin Coughlin <dcoughlin@apple.com>
Date: Mon, 18 Jul 2016 18:57:50 +0000
Subject: [PATCH] Revert "[analyzer] Add checker modeling potential C++
self-assignment"
This reverts commit r275820. It is failing on the bots.
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@275880 91177308-0d34-0410-b5e6-96231b3b80d8
---
.../clang/StaticAnalyzer/Checkers/Checkers.td | 4 -
.../Core/BugReporter/BugReporterVisitor.h | 16 ----
lib/StaticAnalyzer/Checkers/CMakeLists.txt | 1 -
.../Checkers/CXXSelfAssignmentChecker.cpp | 62 -------------
lib/StaticAnalyzer/Core/BugReporter.cpp | 1 -
.../Core/BugReporterVisitors.cpp | 50 -----------
.../Frontend/AnalysisConsumer.cpp | 11 +--
test/Analysis/self-assign.cpp | 89 -------------------
8 files changed, 3 insertions(+), 231 deletions(-)
delete mode 100644 lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp
delete mode 100644 test/Analysis/self-assign.cpp
diff --git a/include/clang/StaticAnalyzer/Checkers/Checkers.td b/include/clang/StaticAnalyzer/Checkers/Checkers.td
index edf82a775ef..785e064e72a 100644
--- a/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -247,10 +247,6 @@ def NewDeleteLeaksChecker : Checker<"NewDeleteLeaks">,
HelpText<"Check for memory leaks. Traces memory managed by new/delete.">,
DescFile<"MallocChecker.cpp">;
-def CXXSelfAssignmentChecker : Checker<"SelfAssignment">,
- HelpText<"Checks C++ copy and move assignment operators for self assignment">,
- DescFile<"CXXSelfAssignmentChecker.cpp">;
-
} // end: "cplusplus"
let ParentPackage = CplusplusAlpha in {
diff --git a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h
index cb785f344ce..c954bbfad8c 100644
--- a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h
+++ b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h
@@ -331,22 +331,6 @@ public:
BugReport &BR) override;
};
-class CXXSelfAssignmentBRVisitor final
- : public BugReporterVisitorImpl<CXXSelfAssignmentBRVisitor> {
-
- bool Satisfied;
-
-public:
- CXXSelfAssignmentBRVisitor() : Satisfied(false) {}
-
- void Profile(llvm::FoldingSetNodeID &ID) const override {}
-
- PathDiagnosticPiece *VisitNode(const ExplodedNode *Succ,
- const ExplodedNode *Pred,
- BugReporterContext &BRC,
- BugReport &BR) override;
-};
-
namespace bugreporter {
/// Attempts to add visitors to trace a null or undefined value back to its
diff --git a/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/lib/StaticAnalyzer/Checkers/CMakeLists.txt
index 3510c510c75..62ccc3cb497 100644
--- a/lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ b/lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -22,7 +22,6 @@ add_clang_library(clangStaticAnalyzerCheckers
CheckerDocumentation.cpp
ChrootChecker.cpp
ClangCheckers.cpp
- CXXSelfAssignmentChecker.cpp
DeadStoresChecker.cpp
DebugCheckers.cpp
DereferenceChecker.cpp
diff --git a/lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp b/lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp
deleted file mode 100644
index 7631322d255..00000000000
--- a/lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp
+++ /dev/null
@@ -1,62 +0,0 @@
-//=== CXXSelfAssignmentChecker.cpp -----------------------------*- C++ -*--===//
-//
-// The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===----------------------------------------------------------------------===//
-//
-// This file defines CXXSelfAssignmentChecker, which tests all custom defined
-// copy and move assignment operators for the case of self assignment, thus
-// where the parameter refers to the same location where the this pointer
-// points to. The checker itself does not do any checks at all, but it
-// causes the analyzer to check every copy and move assignment operator twice:
-// once for when 'this' aliases with the parameter and once for when it may not.
-// It is the task of the other enabled checkers to find the bugs in these two
-// different cases.
-//
-//===----------------------------------------------------------------------===//
-
-#include "ClangSACheckers.h"
-#include "clang/StaticAnalyzer/Core/Checker.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
-
-using namespace clang;
-using namespace ento;
-
-namespace {
-
-class CXXSelfAssignmentChecker : public Checker<check::BeginFunction> {
-public:
- CXXSelfAssignmentChecker();
- void checkBeginFunction(CheckerContext &C) const;
-};
-}
-
-CXXSelfAssignmentChecker::CXXSelfAssignmentChecker() {}
-
-void CXXSelfAssignmentChecker::checkBeginFunction(CheckerContext &C) const {
- if (!C.inTopFrame())
- return;
- const auto *LCtx = C.getLocationContext();
- const auto *MD = dyn_cast<CXXMethodDecl>(LCtx->getDecl());
- if (!MD)
- return;
- if (!MD->isCopyAssignmentOperator() && !MD->isMoveAssignmentOperator())
- return;
- auto &State = C.getState();
- auto &SVB = C.getSValBuilder();
- auto ThisVal =
- State->getSVal(SVB.getCXXThis(MD, LCtx->getCurrentStackFrame()));
- auto Param = SVB.makeLoc(State->getRegion(MD->getParamDecl(0), LCtx));
- auto ParamVal = State->getSVal(Param);
- ProgramStateRef SelfAssignState = State->bindLoc(Param, ThisVal);
- C.addTransition(SelfAssignState);
- ProgramStateRef NonSelfAssignState = State->bindLoc(Param, ParamVal);
- C.addTransition(NonSelfAssignState);
-}
-
-void ento::registerCXXSelfAssignmentChecker(CheckerManager &Mgr) {
- Mgr.registerChecker<CXXSelfAssignmentChecker>();
-}
diff --git a/lib/StaticAnalyzer/Core/BugReporter.cpp b/lib/StaticAnalyzer/Core/BugReporter.cpp
index e04aa395d49..488126b0088 100644
--- a/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ b/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -3104,7 +3104,6 @@ bool GRBugReporter::generatePathDiagnostic(PathDiagnostic& PD,
R->addVisitor(llvm::make_unique<NilReceiverBRVisitor>());
R->addVisitor(llvm::make_unique<ConditionBRVisitor>());
R->addVisitor(llvm::make_unique<LikelyFalsePositiveSuppressionBRVisitor>());
- R->addVisitor(llvm::make_unique<CXXSelfAssignmentBRVisitor>());
BugReport::VisitorList visitors;
unsigned origReportConfigToken, finalReportConfigToken;
diff --git a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index 5e1a4828022..0e505463bb5 100644
--- a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1693,53 +1693,3 @@ UndefOrNullArgVisitor::VisitNode(const ExplodedNode *N,
}
return nullptr;
}
-
-PathDiagnosticPiece *
-CXXSelfAssignmentBRVisitor::VisitNode(const ExplodedNode *Succ,
- const ExplodedNode *Pred,
- BugReporterContext &BRC, BugReport &BR) {
- if (Satisfied)
- return nullptr;
-
- auto Edge = Succ->getLocation().getAs<BlockEdge>();
- if (!Edge.hasValue())
- return nullptr;
-
- auto Tag = Edge->getTag();
- if (!Tag)
- return nullptr;
-
- if (Tag->getTagDescription() != "cplusplus.SelfAssignment")
- return nullptr;
-
- Satisfied = true;
-
- const auto *Met =
- dyn_cast<CXXMethodDecl>(Succ->getCodeDecl().getAsFunction());
- assert(Met && "Not a C++ method.");
- assert((Met->isCopyAssignmentOperator() || Met->isMoveAssignmentOperator()) &&
- "Not a copy/move assignment operator.");
-
- const auto *LCtx = Edge->getLocationContext();
-
- const auto &State = Succ->getState();
- auto &SVB = State->getStateManager().getSValBuilder();
-
- const auto Param =
- State->getSVal(State->getRegion(Met->getParamDecl(0), LCtx));
- const auto This =
- State->getSVal(SVB.getCXXThis(Met, LCtx->getCurrentStackFrame()));
-
- auto L = PathDiagnosticLocation::create(Met, BRC.getSourceManager());
-
- if (!L.isValid() || !L.asLocation().isValid())
- return nullptr;
-
- const auto Msg = "Assuming " + Met->getParamDecl(0)->getName() +
- ((Param == This) ? " == " : " != ") + "*this";
-
- auto *Piece = new PathDiagnosticEventPiece(L, Msg.str());
- Piece->addRange(Met->getSourceRange());
-
- return Piece;
-}
diff --git a/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp b/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
index 01324d0edd6..8ac229fc658 100644
--- a/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
+++ b/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
@@ -431,13 +431,6 @@ static bool shouldSkipFunction(const Decl *D,
// Count naming convention errors more aggressively.
if (isa<ObjCMethodDecl>(D))
return false;
- // We also want to reanalyze all C++ copy and move assignment operators to
- // separately check the two cases where 'this' aliases with the parameter and
- // where it may not. (cplusplus.SelfAssignmentChecker)
- if (const auto *MD = dyn_cast<CXXMethodDecl>(D)) {
- if (MD->isCopyAssignmentOperator() || MD->isMoveAssignmentOperator())
- return false;
- }
// Otherwise, if we visited the function before, do not reanalyze it.
return Visited.count(D);
@@ -449,7 +442,9 @@ AnalysisConsumer::getInliningModeForFunction(const Decl *D,
// We want to reanalyze all ObjC methods as top level to report Retain
// Count naming convention errors more aggressively. But we should tune down
// inlining when reanalyzing an already inlined function.
- if (Visited.count(D) && isa<ObjCMethodDecl>(D)) {
+ if (Visited.count(D)) {
+ assert(isa<ObjCMethodDecl>(D) &&
+ "We are only reanalyzing ObjCMethods.");
const ObjCMethodDecl *ObjCM = cast<ObjCMethodDecl>(D);
if (ObjCM->getMethodFamily() != OMF_init)
return ExprEngine::Inline_Minimal;
diff --git a/test/Analysis/self-assign.cpp b/test/Analysis/self-assign.cpp
deleted file mode 100644
index 74fb0fea247..00000000000
--- a/test/Analysis/self-assign.cpp
+++ /dev/null
@@ -1,89 +0,0 @@
-// RUN: %clang_cc1 -std=c++11 -analyze -analyzer-checker=core,cplusplus,unix.Malloc,debug.ExprInspection %s -verify -analyzer-output=text
-
-extern "C" char *strdup(const char* s);
-extern "C" void free(void* ptr);
-
-namespace std {
-template<class T> struct remove_reference { typedef T type; };
-template<class T> struct remove_reference<T&> { typedef T type; };
-template<class T> struct remove_reference<T&&> { typedef T type; };
-template<class T> typename remove_reference<T>::type&& move(T&& t);
-}
-
-void clang_analyzer_eval(int);
-
-class StringUsed {
-public:
- StringUsed(const char *s = "") : str(strdup(s)) {}
- StringUsed(const StringUsed &rhs) : str(strdup(rhs.str)) {}
- ~StringUsed();
- StringUsed& operator=(const StringUsed &rhs);
- StringUsed& operator=(StringUsed &&rhs);
- operator const char*() const;
-private:
- char *str;
-};
-
-StringUsed::~StringUsed() {
- free(str);
-}
-
-StringUsed& StringUsed::operator=(const StringUsed &rhs) { // expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs != *this}}
- clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}}
- free(str); // expected-note{{Memory is released}}
- str = strdup(rhs.str); // expected-warning{{Use of memory after it is freed}} expected-note{{Use of memory after it is freed}}
- return *this;
-}
-
-StringUsed& StringUsed::operator=(StringUsed &&rhs) { // expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs != *this}}
- clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}}
- str = rhs.str;
- rhs.str = nullptr; // FIXME: An improved leak checker should warn here
- return *this;
-}
-
-StringUsed::operator const char*() const {
- return str;
-}
-
-class StringUnused {
-public:
- StringUnused(const char *s = "") : str(strdup(s)) {}
- StringUnused(const StringUnused &rhs) : str(strdup(rhs.str)) {}
- ~StringUnused();
- StringUnused& operator=(const StringUnused &rhs);
- StringUnused& operator=(StringUnused &&rhs);
- operator const char*() const;
-private:
- char *str;
-};
-
-StringUnused::~StringUnused() {
- free(str);
-}
-
-StringUnused& StringUnused::operator=(const StringUnused &rhs) { // expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs != *this}}
- clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}}
- free(str); // expected-note{{Memory is released}}
- str = strdup(rhs.str); // expected-warning{{Use of memory after it is freed}} expected-note{{Use of memory after it is freed}}
- return *this;
-}
-
-StringUnused& StringUnused::operator=(StringUnused &&rhs) { // expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs != *this}}
- clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}}
- str = rhs.str;
- rhs.str = nullptr; // FIXME: An improved leak checker should warn here
- return *this;
-}
-
-StringUnused::operator const char*() const {
- return str;
-}
-
-
-int main() {
- StringUsed s1 ("test"), s2;
- s2 = s1;
- s2 = std::move(s1);
- return 0;
-}
--
GitLab