Thanks, I've merged the revert in r275902. Cheers, Hans
On Mon, Jul 18, 2016 at 12:06 PM, Devin Coughlin <dcough...@apple.com> wrote: > Reverted in r275880. Sorry about that. I’ll have to investigate why I didn’t > get an email from the bot about the failure. > > Thanks > Devin > > On Jul 18, 2016, at 11:59 AM, Hans Wennborg <h...@chromium.org> wrote: > > Oops, as you suspected I failed at copy/paste. This is the bot that's > broken: http://bb.pgr.jp/builders/cmake-clang-x86_64-linux/builds/51780 > > On Mon, Jul 18, 2016 at 10:57 AM, Devin Coughlin <dcough...@apple.com> > wrote: > > Hans, > > I’m happy to revert — but I think r275820 is very unlikely to have been the > root cause of the issue at > http://bb.pgr.jp/builders/cmake-llvm-x86_64-linux/builds/40225/. (The commit > is a change to the clang static analyzer and the bot failure seems to be in > LLVM CodeGen). > > Was this just a link mis-copy/paste? Is there another bot failing? > > Devin > > On Jul 18, 2016, at 10:50 AM, Hans Wennborg <h...@chromium.org> wrote: > > It seems to have broken this buildbot: > http://bb.pgr.jp/builders/cmake-llvm-x86_64-linux/builds/40225/ > > I'm just about to create the 3.9 release branch, so it would be great > if this could fixed/figured out soon. > > Thanks, > Hans > > On Mon, Jul 18, 2016 at 10:23 AM, Devin Coughlin via cfe-commits > <cfe-commits@lists.llvm.org> wrote: > > Author: dcoughlin > Date: Mon Jul 18 12:23:30 2016 > New Revision: 275820 > > URL: http://llvm.org/viewvc/llvm-project?rev=275820&view=rev > Log: > [analyzer] Add checker modeling potential C++ self-assignment > > This checker checks copy and move assignment operators whether they are > protected against self-assignment. Since C++ core guidelines discourages > explicit checking for `&rhs==this` in general we take a different approach: > in > top-frame analysis we branch the exploded graph for two cases, where > &rhs==this > and &rhs!=this and let existing checkers (e.g. unix.Malloc) do the rest of > the > work. It is important that we check all copy and move assignment operator in > top > frame even if we checked them already since self-assignments may happen > undetected even in the same translation unit (e.g. using random indices for > an > array what may or may not be the same). > > A patch by Ádám Balogh! > > Differential Revision: https://reviews.llvm.org/D19311 > > Added: > cfe/trunk/lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp > cfe/trunk/test/Analysis/self-assign.cpp > Modified: > cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td > > cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h > cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt > cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp > cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp > cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp > > Modified: cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td?rev=275820&r1=275819&r2=275820&view=diff > ============================================================================== > --- cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td (original) > +++ cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td Mon Jul 18 > 12:23:30 2016 > @@ -247,6 +247,10 @@ def NewDeleteLeaksChecker : Checker<"New > 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 { > > Modified: > cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h?rev=275820&r1=275819&r2=275820&view=diff > ============================================================================== > --- > cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h > (original) > +++ > cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h > Mon Jul 18 12:23:30 2016 > @@ -331,6 +331,22 @@ 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 > > Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt?rev=275820&r1=275819&r2=275820&view=diff > ============================================================================== > --- cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt (original) > +++ cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt Mon Jul 18 12:23:30 > 2016 > @@ -22,6 +22,7 @@ add_clang_library(clangStaticAnalyzerChe > CheckerDocumentation.cpp > ChrootChecker.cpp > ClangCheckers.cpp > + CXXSelfAssignmentChecker.cpp > DeadStoresChecker.cpp > DebugCheckers.cpp > DereferenceChecker.cpp > > Added: cfe/trunk/lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp?rev=275820&view=auto > ============================================================================== > --- cfe/trunk/lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp > (added) > +++ cfe/trunk/lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp Mon > Jul 18 12:23:30 2016 > @@ -0,0 +1,62 @@ > +//=== 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>(); > +} > > Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp?rev=275820&r1=275819&r2=275820&view=diff > ============================================================================== > --- cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp (original) > +++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp Mon Jul 18 12:23:30 > 2016 > @@ -3104,6 +3104,7 @@ bool GRBugReporter::generatePathDiagnost > 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; > > Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp?rev=275820&r1=275819&r2=275820&view=diff > ============================================================================== > --- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp (original) > +++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp Mon Jul 18 > 12:23:30 2016 > @@ -1693,3 +1693,53 @@ UndefOrNullArgVisitor::VisitNode(const E > } > 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; > +} > > Modified: cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp?rev=275820&r1=275819&r2=275820&view=diff > ============================================================================== > --- cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp (original) > +++ cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp Mon Jul 18 > 12:23:30 2016 > @@ -431,6 +431,13 @@ static bool shouldSkipFunction(const Dec > // 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); > @@ -442,9 +449,7 @@ AnalysisConsumer::getInliningModeForFunc > // 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)) { > - assert(isa<ObjCMethodDecl>(D) && > - "We are only reanalyzing ObjCMethods."); > + if (Visited.count(D) && isa<ObjCMethodDecl>(D)) { > const ObjCMethodDecl *ObjCM = cast<ObjCMethodDecl>(D); > if (ObjCM->getMethodFamily() != OMF_init) > return ExprEngine::Inline_Minimal; > > Added: cfe/trunk/test/Analysis/self-assign.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/self-assign.cpp?rev=275820&view=auto > ============================================================================== > --- cfe/trunk/test/Analysis/self-assign.cpp (added) > +++ cfe/trunk/test/Analysis/self-assign.cpp Mon Jul 18 12:23:30 2016 > @@ -0,0 +1,89 @@ > +// 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; > +} > > > _______________________________________________ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > > > _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
