r275820 - [analyzer] Add checker modeling potential C++ self-assignment

Hans Wennborg via cfe-commits cfe-commits at lists.llvm.org
Mon Jul 18 11:59:26 PDT 2016


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 <dcoughlin at 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 <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
>
>


More information about the cfe-commits mailing list