<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Hans,<div class=""><br class=""></div><div class="">I’m happy to revert — but I think r275820 is very unlikely to have been the root cause of the issue at <a href="http://bb.pgr.jp/builders/cmake-llvm-x86_64-linux/builds/40225/" class="">http://bb.pgr.jp/builders/cmake-llvm-x86_64-linux/builds/40225/</a>. (The commit is a change to the clang static analyzer and the bot failure seems to be in LLVM CodeGen).</div><div class=""><br class=""></div><div class="">Was this just a link mis-copy/paste? Is there another bot failing?</div><div class=""><br class=""></div><div class="">Devin</div><div class=""><br class=""></div><div class=""><div><blockquote type="cite" class=""><div class="">On Jul 18, 2016, at 10:50 AM, Hans Wennborg <<a href="mailto:hans@chromium.org" class="">hans@chromium.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="">It seems to have broken this buildbot:<br class=""><a href="http://bb.pgr.jp/builders/cmake-llvm-x86_64-linux/builds/40225/" class="">http://bb.pgr.jp/builders/cmake-llvm-x86_64-linux/builds/40225/</a><br class=""><br class="">I'm just about to create the 3.9 release branch, so it would be great<br class="">if this could fixed/figured out soon.<br class=""><br class="">Thanks,<br class="">Hans<br class=""><br class="">On Mon, Jul 18, 2016 at 10:23 AM, Devin Coughlin via cfe-commits<br class=""><cfe-commits@lists.llvm.org> wrote:<br class=""><blockquote type="cite" class="">Author: dcoughlin<br class="">Date: Mon Jul 18 12:23:30 2016<br class="">New Revision: 275820<br class=""><br class="">URL: http://llvm.org/viewvc/llvm-project?rev=275820&view=rev<br class="">Log:<br class="">[analyzer] Add checker modeling potential C++ self-assignment<br class=""><br class="">This checker checks copy and move assignment operators whether they are<br class="">protected against self-assignment. Since C++ core guidelines discourages<br class="">explicit checking for `&rhs==this` in general we take a different approach: in<br class="">top-frame analysis we branch the exploded graph for two cases, where &rhs==this<br class="">and &rhs!=this and let existing checkers (e.g. unix.Malloc) do the rest of the<br class="">work. It is important that we check all copy and move assignment operator in top<br class="">frame even if we checked them already since self-assignments may happen<br class="">undetected even in the same translation unit (e.g. using random indices for an<br class="">array what may or may not be the same).<br class=""><br class="">A patch by Ádám Balogh!<br class=""><br class="">Differential Revision: https://reviews.llvm.org/D19311<br class=""><br class="">Added:<br class=""> cfe/trunk/lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp<br class=""> cfe/trunk/test/Analysis/self-assign.cpp<br class="">Modified:<br class=""> cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td<br class=""> cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h<br class=""> cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt<br class=""> cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp<br class=""> cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp<br class=""> cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp<br class=""><br class="">Modified: cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td<br class="">URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td?rev=275820&r1=275819&r2=275820&view=diff<br class="">==============================================================================<br class="">--- cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td (original)<br class="">+++ cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td Mon Jul 18 12:23:30 2016<br class="">@@ -247,6 +247,10 @@ def NewDeleteLeaksChecker : Checker<"New<br class=""> HelpText<"Check for memory leaks. Traces memory managed by new/delete.">,<br class=""> DescFile<"MallocChecker.cpp">;<br class=""><br class="">+def CXXSelfAssignmentChecker : Checker<"SelfAssignment">,<br class="">+ HelpText<"Checks C++ copy and move assignment operators for self assignment">,<br class="">+ DescFile<"CXXSelfAssignmentChecker.cpp">;<br class="">+<br class=""> } // end: "cplusplus"<br class=""><br class=""> let ParentPackage = CplusplusAlpha in {<br class=""><br class="">Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h<br class="">URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h?rev=275820&r1=275819&r2=275820&view=diff<br class="">==============================================================================<br class="">--- cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h (original)<br class="">+++ cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h Mon Jul 18 12:23:30 2016<br class="">@@ -331,6 +331,22 @@ public:<br class=""> BugReport &BR) override;<br class=""> };<br class=""><br class="">+class CXXSelfAssignmentBRVisitor final<br class="">+ : public BugReporterVisitorImpl<CXXSelfAssignmentBRVisitor> {<br class="">+<br class="">+ bool Satisfied;<br class="">+<br class="">+public:<br class="">+ CXXSelfAssignmentBRVisitor() : Satisfied(false) {}<br class="">+<br class="">+ void Profile(llvm::FoldingSetNodeID &ID) const override {}<br class="">+<br class="">+ PathDiagnosticPiece *VisitNode(const ExplodedNode *Succ,<br class="">+ const ExplodedNode *Pred,<br class="">+ BugReporterContext &BRC,<br class="">+ BugReport &BR) override;<br class="">+};<br class="">+<br class=""> namespace bugreporter {<br class=""><br class=""> /// Attempts to add visitors to trace a null or undefined value back to its<br class=""><br class="">Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt<br class="">URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt?rev=275820&r1=275819&r2=275820&view=diff<br class="">==============================================================================<br class="">--- cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt (original)<br class="">+++ cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt Mon Jul 18 12:23:30 2016<br class="">@@ -22,6 +22,7 @@ add_clang_library(clangStaticAnalyzerChe<br class=""> CheckerDocumentation.cpp<br class=""> ChrootChecker.cpp<br class=""> ClangCheckers.cpp<br class="">+ CXXSelfAssignmentChecker.cpp<br class=""> DeadStoresChecker.cpp<br class=""> DebugCheckers.cpp<br class=""> DereferenceChecker.cpp<br class=""><br class="">Added: cfe/trunk/lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp<br class="">URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp?rev=275820&view=auto<br class="">==============================================================================<br class="">--- cfe/trunk/lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp (added)<br class="">+++ cfe/trunk/lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp Mon Jul 18 12:23:30 2016<br class="">@@ -0,0 +1,62 @@<br class="">+//=== CXXSelfAssignmentChecker.cpp -----------------------------*- C++ -*--===//<br class="">+//<br class="">+// The LLVM Compiler Infrastructure<br class="">+//<br class="">+// This file is distributed under the University of Illinois Open Source<br class="">+// License. See LICENSE.TXT for details.<br class="">+//<br class="">+//===----------------------------------------------------------------------===//<br class="">+//<br class="">+// This file defines CXXSelfAssignmentChecker, which tests all custom defined<br class="">+// copy and move assignment operators for the case of self assignment, thus<br class="">+// where the parameter refers to the same location where the this pointer<br class="">+// points to. The checker itself does not do any checks at all, but it<br class="">+// causes the analyzer to check every copy and move assignment operator twice:<br class="">+// once for when 'this' aliases with the parameter and once for when it may not.<br class="">+// It is the task of the other enabled checkers to find the bugs in these two<br class="">+// different cases.<br class="">+//<br class="">+//===----------------------------------------------------------------------===//<br class="">+<br class="">+#include "ClangSACheckers.h"<br class="">+#include "clang/StaticAnalyzer/Core/Checker.h"<br class="">+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"<br class="">+<br class="">+using namespace clang;<br class="">+using namespace ento;<br class="">+<br class="">+namespace {<br class="">+<br class="">+class CXXSelfAssignmentChecker : public Checker<check::BeginFunction> {<br class="">+public:<br class="">+ CXXSelfAssignmentChecker();<br class="">+ void checkBeginFunction(CheckerContext &C) const;<br class="">+};<br class="">+}<br class="">+<br class="">+CXXSelfAssignmentChecker::CXXSelfAssignmentChecker() {}<br class="">+<br class="">+void CXXSelfAssignmentChecker::checkBeginFunction(CheckerContext &C) const {<br class="">+ if (!C.inTopFrame())<br class="">+ return;<br class="">+ const auto *LCtx = C.getLocationContext();<br class="">+ const auto *MD = dyn_cast<CXXMethodDecl>(LCtx->getDecl());<br class="">+ if (!MD)<br class="">+ return;<br class="">+ if (!MD->isCopyAssignmentOperator() && !MD->isMoveAssignmentOperator())<br class="">+ return;<br class="">+ auto &State = C.getState();<br class="">+ auto &SVB = C.getSValBuilder();<br class="">+ auto ThisVal =<br class="">+ State->getSVal(SVB.getCXXThis(MD, LCtx->getCurrentStackFrame()));<br class="">+ auto Param = SVB.makeLoc(State->getRegion(MD->getParamDecl(0), LCtx));<br class="">+ auto ParamVal = State->getSVal(Param);<br class="">+ ProgramStateRef SelfAssignState = State->bindLoc(Param, ThisVal);<br class="">+ C.addTransition(SelfAssignState);<br class="">+ ProgramStateRef NonSelfAssignState = State->bindLoc(Param, ParamVal);<br class="">+ C.addTransition(NonSelfAssignState);<br class="">+}<br class="">+<br class="">+void ento::registerCXXSelfAssignmentChecker(CheckerManager &Mgr) {<br class="">+ Mgr.registerChecker<CXXSelfAssignmentChecker>();<br class="">+}<br class=""><br class="">Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp<br class="">URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp?rev=275820&r1=275819&r2=275820&view=diff<br class="">==============================================================================<br class="">--- cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp (original)<br class="">+++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp Mon Jul 18 12:23:30 2016<br class="">@@ -3104,6 +3104,7 @@ bool GRBugReporter::generatePathDiagnost<br class=""> R->addVisitor(llvm::make_unique<NilReceiverBRVisitor>());<br class=""> R->addVisitor(llvm::make_unique<ConditionBRVisitor>());<br class=""> R->addVisitor(llvm::make_unique<LikelyFalsePositiveSuppressionBRVisitor>());<br class="">+ R->addVisitor(llvm::make_unique<CXXSelfAssignmentBRVisitor>());<br class=""><br class=""> BugReport::VisitorList visitors;<br class=""> unsigned origReportConfigToken, finalReportConfigToken;<br class=""><br class="">Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp<br class="">URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp?rev=275820&r1=275819&r2=275820&view=diff<br class="">==============================================================================<br class="">--- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp (original)<br class="">+++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp Mon Jul 18 12:23:30 2016<br class="">@@ -1693,3 +1693,53 @@ UndefOrNullArgVisitor::VisitNode(const E<br class=""> }<br class=""> return nullptr;<br class=""> }<br class="">+<br class="">+PathDiagnosticPiece *<br class="">+CXXSelfAssignmentBRVisitor::VisitNode(const ExplodedNode *Succ,<br class="">+ const ExplodedNode *Pred,<br class="">+ BugReporterContext &BRC, BugReport &BR) {<br class="">+ if (Satisfied)<br class="">+ return nullptr;<br class="">+<br class="">+ auto Edge = Succ->getLocation().getAs<BlockEdge>();<br class="">+ if (!Edge.hasValue())<br class="">+ return nullptr;<br class="">+<br class="">+ auto Tag = Edge->getTag();<br class="">+ if (!Tag)<br class="">+ return nullptr;<br class="">+<br class="">+ if (Tag->getTagDescription() != "cplusplus.SelfAssignment")<br class="">+ return nullptr;<br class="">+<br class="">+ Satisfied = true;<br class="">+<br class="">+ const auto *Met =<br class="">+ dyn_cast<CXXMethodDecl>(Succ->getCodeDecl().getAsFunction());<br class="">+ assert(Met && "Not a C++ method.");<br class="">+ assert((Met->isCopyAssignmentOperator() || Met->isMoveAssignmentOperator()) &&<br class="">+ "Not a copy/move assignment operator.");<br class="">+<br class="">+ const auto *LCtx = Edge->getLocationContext();<br class="">+<br class="">+ const auto &State = Succ->getState();<br class="">+ auto &SVB = State->getStateManager().getSValBuilder();<br class="">+<br class="">+ const auto Param =<br class="">+ State->getSVal(State->getRegion(Met->getParamDecl(0), LCtx));<br class="">+ const auto This =<br class="">+ State->getSVal(SVB.getCXXThis(Met, LCtx->getCurrentStackFrame()));<br class="">+<br class="">+ auto L = PathDiagnosticLocation::create(Met, BRC.getSourceManager());<br class="">+<br class="">+ if (!L.isValid() || !L.asLocation().isValid())<br class="">+ return nullptr;<br class="">+<br class="">+ const auto Msg = "Assuming " + Met->getParamDecl(0)->getName() +<br class="">+ ((Param == This) ? " == " : " != ") + "*this";<br class="">+<br class="">+ auto *Piece = new PathDiagnosticEventPiece(L, Msg.str());<br class="">+ Piece->addRange(Met->getSourceRange());<br class="">+<br class="">+ return Piece;<br class="">+}<br class=""><br class="">Modified: cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp<br class="">URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp?rev=275820&r1=275819&r2=275820&view=diff<br class="">==============================================================================<br class="">--- cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp (original)<br class="">+++ cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp Mon Jul 18 12:23:30 2016<br class="">@@ -431,6 +431,13 @@ static bool shouldSkipFunction(const Dec<br class=""> // Count naming convention errors more aggressively.<br class=""> if (isa<ObjCMethodDecl>(D))<br class=""> return false;<br class="">+ // We also want to reanalyze all C++ copy and move assignment operators to<br class="">+ // separately check the two cases where 'this' aliases with the parameter and<br class="">+ // where it may not. (cplusplus.SelfAssignmentChecker)<br class="">+ if (const auto *MD = dyn_cast<CXXMethodDecl>(D)) {<br class="">+ if (MD->isCopyAssignmentOperator() || MD->isMoveAssignmentOperator())<br class="">+ return false;<br class="">+ }<br class=""><br class=""> // Otherwise, if we visited the function before, do not reanalyze it.<br class=""> return Visited.count(D);<br class="">@@ -442,9 +449,7 @@ AnalysisConsumer::getInliningModeForFunc<br class=""> // We want to reanalyze all ObjC methods as top level to report Retain<br class=""> // Count naming convention errors more aggressively. But we should tune down<br class=""> // inlining when reanalyzing an already inlined function.<br class="">- if (Visited.count(D)) {<br class="">- assert(isa<ObjCMethodDecl>(D) &&<br class="">- "We are only reanalyzing ObjCMethods.");<br class="">+ if (Visited.count(D) && isa<ObjCMethodDecl>(D)) {<br class=""> const ObjCMethodDecl *ObjCM = cast<ObjCMethodDecl>(D);<br class=""> if (ObjCM->getMethodFamily() != OMF_init)<br class=""> return ExprEngine::Inline_Minimal;<br class=""><br class="">Added: cfe/trunk/test/Analysis/self-assign.cpp<br class="">URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/self-assign.cpp?rev=275820&view=auto<br class="">==============================================================================<br class="">--- cfe/trunk/test/Analysis/self-assign.cpp (added)<br class="">+++ cfe/trunk/test/Analysis/self-assign.cpp Mon Jul 18 12:23:30 2016<br class="">@@ -0,0 +1,89 @@<br class="">+// RUN: %clang_cc1 -std=c++11 -analyze -analyzer-checker=core,cplusplus,unix.Malloc,debug.ExprInspection %s -verify -analyzer-output=text<br class="">+<br class="">+extern "C" char *strdup(const char* s);<br class="">+extern "C" void free(void* ptr);<br class="">+<br class="">+namespace std {<br class="">+template<class T> struct remove_reference { typedef T type; };<br class="">+template<class T> struct remove_reference<T&> { typedef T type; };<br class="">+template<class T> struct remove_reference<T&&> { typedef T type; };<br class="">+template<class T> typename remove_reference<T>::type&& move(T&& t);<br class="">+}<br class="">+<br class="">+void clang_analyzer_eval(int);<br class="">+<br class="">+class StringUsed {<br class="">+public:<br class="">+ StringUsed(const char *s = "") : str(strdup(s)) {}<br class="">+ StringUsed(const StringUsed &rhs) : str(strdup(rhs.str)) {}<br class="">+ ~StringUsed();<br class="">+ StringUsed& operator=(const StringUsed &rhs);<br class="">+ StringUsed& operator=(StringUsed &&rhs);<br class="">+ operator const char*() const;<br class="">+private:<br class="">+ char *str;<br class="">+};<br class="">+<br class="">+StringUsed::~StringUsed() {<br class="">+ free(str);<br class="">+}<br class="">+<br class="">+StringUsed& StringUsed::operator=(const StringUsed &rhs) { // expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs != *this}}<br class="">+ clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}}<br class="">+ free(str); // expected-note{{Memory is released}}<br class="">+ str = strdup(rhs.str); // expected-warning{{Use of memory after it is freed}} expected-note{{Use of memory after it is freed}}<br class="">+ return *this;<br class="">+}<br class="">+<br class="">+StringUsed& StringUsed::operator=(StringUsed &&rhs) { // expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs != *this}}<br class="">+ clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}}<br class="">+ str = rhs.str;<br class="">+ rhs.str = nullptr; // FIXME: An improved leak checker should warn here<br class="">+ return *this;<br class="">+}<br class="">+<br class="">+StringUsed::operator const char*() const {<br class="">+ return str;<br class="">+}<br class="">+<br class="">+class StringUnused {<br class="">+public:<br class="">+ StringUnused(const char *s = "") : str(strdup(s)) {}<br class="">+ StringUnused(const StringUnused &rhs) : str(strdup(rhs.str)) {}<br class="">+ ~StringUnused();<br class="">+ StringUnused& operator=(const StringUnused &rhs);<br class="">+ StringUnused& operator=(StringUnused &&rhs);<br class="">+ operator const char*() const;<br class="">+private:<br class="">+ char *str;<br class="">+};<br class="">+<br class="">+StringUnused::~StringUnused() {<br class="">+ free(str);<br class="">+}<br class="">+<br class="">+StringUnused& StringUnused::operator=(const StringUnused &rhs) { // expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs != *this}}<br class="">+ clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}}<br class="">+ free(str); // expected-note{{Memory is released}}<br class="">+ str = strdup(rhs.str); // expected-warning{{Use of memory after it is freed}} expected-note{{Use of memory after it is freed}}<br class="">+ return *this;<br class="">+}<br class="">+<br class="">+StringUnused& StringUnused::operator=(StringUnused &&rhs) { // expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs != *this}}<br class="">+ clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}}<br class="">+ str = rhs.str;<br class="">+ rhs.str = nullptr; // FIXME: An improved leak checker should warn here<br class="">+ return *this;<br class="">+}<br class="">+<br class="">+StringUnused::operator const char*() const {<br class="">+ return str;<br class="">+}<br class="">+<br class="">+<br class="">+int main() {<br class="">+ StringUsed s1 ("test"), s2;<br class="">+ s2 = s1;<br class="">+ s2 = std::move(s1);<br class="">+ return 0;<br class="">+}<br class=""><br class=""><br class="">_______________________________________________<br class="">cfe-commits mailing list<br class="">cfe-commits@lists.llvm.org<br class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits<br class=""></blockquote></div></div></blockquote></div><br class=""></div></body></html>