r275820 - [analyzer] Add checker modeling potential C++ self-assignment
Devin Coughlin via cfe-commits
cfe-commits at lists.llvm.org
Mon Jul 18 10:57:45 PDT 2016
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 <hans at 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 at 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 at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160718/895dc8dd/attachment-0001.html>
More information about the cfe-commits
mailing list