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/ <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
