Add a new checker that tests whether a throw expression throws by value, and whether a catch statement catches by reference.

Patch by Tobias Langner!

llvm-svn: 249899

clang-tools-extra/clang-tidy/misc/CMakeLists.txt

@@ -17,6 +17,7 @@ add_clang_library(clangTidyMiscModule
   SizeofContainerCheck.cpp
   StaticAssertCheck.cpp
   SwappedArgumentsCheck.cpp
+  ThrowByValueCatchByReferenceCheck.cpp
   UndelegatedConstructor.cpp
   UnusedAliasDeclsCheck.cpp
   UnusedParametersCheck.cpp

clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp

@@ -25,6 +25,7 @@
 #include "SizeofContainerCheck.h"
 #include "StaticAssertCheck.h"
 #include "SwappedArgumentsCheck.h"
+#include "ThrowByValueCatchByReferenceCheck.h"
 #include "UndelegatedConstructor.h"
 #include "UniqueptrResetReleaseCheck.h"
 #include "UnusedAliasDeclsCheck.h"
@@ -66,6 +67,8 @@ public:
         "misc-static-assert");
     CheckFactories.registerCheck<SwappedArgumentsCheck>(
         "misc-swapped-arguments");
+    CheckFactories.registerCheck<ThrowByValueCatchByReferenceCheck>(
+        "misc-throw-by-value-catch-by-reference");
     CheckFactories.registerCheck<UndelegatedConstructorCheck>(
         "misc-undelegated-constructor");
     CheckFactories.registerCheck<UniqueptrResetReleaseCheck>(

clang-tools-extra/clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp

@@ -0,0 +1,159 @@
+//===--- ThrowByValueCatchByReferenceCheck.cpp - clang-tidy----------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "ThrowByValueCatchByReferenceCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/AST/OperationKinds.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+
+ThrowByValueCatchByReferenceCheck::ThrowByValueCatchByReferenceCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      CheckAnonymousTemporaries(Options.get("CheckThrowTemporaries", true)) {}
+
+void ThrowByValueCatchByReferenceCheck::registerMatchers(MatchFinder *Finder) {
+  // This is a C++ only check thus we register the matchers only for C++
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  Finder->addMatcher(cxxThrowExpr().bind("throw"), this);
+  Finder->addMatcher(cxxCatchStmt().bind("catch"), this);
+}
+
+void ThrowByValueCatchByReferenceCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "CheckThrowTemporaries", true);
+}
+
+void ThrowByValueCatchByReferenceCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  diagnoseThrowLocations(Result.Nodes.getNodeAs<CXXThrowExpr>("throw"));
+  diagnoseCatchLocations(Result.Nodes.getNodeAs<CXXCatchStmt>("catch"),
+                         *Result.Context);
+}
+
+bool ThrowByValueCatchByReferenceCheck::isFunctionParameter(
+    const DeclRefExpr *declRefExpr) {
+  return isa<ParmVarDecl>(declRefExpr->getDecl());
+}
+
+bool ThrowByValueCatchByReferenceCheck::isCatchVariable(
+    const DeclRefExpr *declRefExpr) {
+  auto *valueDecl = declRefExpr->getDecl();
+  if (auto *varDecl = dyn_cast<VarDecl>(valueDecl))
+    return varDecl->isExceptionVariable();
+  return false;
+}
+
+bool ThrowByValueCatchByReferenceCheck::isFunctionOrCatchVar(
+    const DeclRefExpr *declRefExpr) {
+  return isFunctionParameter(declRefExpr) || isCatchVariable(declRefExpr);
+}
+
+void ThrowByValueCatchByReferenceCheck::diagnoseThrowLocations(
+    const CXXThrowExpr *throwExpr) {
+  if (!throwExpr)
+    return;
+  auto *subExpr = throwExpr->getSubExpr();
+  if (!subExpr)
+    return;
+  auto qualType = subExpr->getType();
+  if (qualType->isPointerType()) {
+    // The code is throwing a pointer.
+    // In case it is strng literal, it is safe and we return.
+    auto *inner = subExpr->IgnoreParenImpCasts();
+    if (isa<StringLiteral>(inner))
+      return;
+    // If it's a variable from a catch statement, we return as well.
+    auto *declRef = dyn_cast<DeclRefExpr>(inner);
+    if (declRef && isCatchVariable(declRef)) {
+      return;
+    }
+    diag(subExpr->getLocStart(), "throw expression throws a pointer; it should "
+                                 "throw a non-pointer value instead");
+  }
+  // If the throw statement does not throw by pointer then it throws by value
+  // which is ok.
+  // There are addition checks that emit diagnosis messages if the thrown value
+  // is not an RValue. See:
+  // https://www.securecoding.cert.org/confluence/display/cplusplus/ERR09-CPP.+Throw+anonymous+temporaries
+  // This behavior can be influenced by an option.
+
+  // If we encounter a CXXThrowExpr, we move through all casts until you either
+  // encounter a DeclRefExpr or a CXXConstructExpr.
+  // If it's a DeclRefExpr, we emit a message if the referenced variable is not
+  // a catch variable or function parameter.
+  // When encountering a CopyOrMoveConstructor: emit message if after casts,
+  // the expression is a LValue
+  if (CheckAnonymousTemporaries) {
+    bool emit = false;
+    auto *currentSubExpr = subExpr->IgnoreImpCasts();
+    const DeclRefExpr *variableReference =
+        dyn_cast<DeclRefExpr>(currentSubExpr);
+    const CXXConstructExpr *constructorCall =
+        dyn_cast<CXXConstructExpr>(currentSubExpr);
+    // If we have a DeclRefExpr, we flag for emitting a diagnosis message in
+    // case the referenced variable is neither a function parameter nor a
+    // variable declared in the catch statement.
+    if (variableReference)
+      emit = !isFunctionOrCatchVar(variableReference);
+    else if (constructorCall &&
+             constructorCall->getConstructor()->isCopyOrMoveConstructor()) {
+      // If we have a copy / move construction, we emit a diagnosis message if
+      // the object that we copy construct from is neither a function parameter
+      // nor a variable declared in a catch statement
+      auto argIter =
+          constructorCall
+              ->arg_begin(); // there's only one for copy constructors
+      auto *currentSubExpr = (*argIter)->IgnoreImpCasts();
+      if (currentSubExpr->isLValue()) {
+        if (auto *tmp = dyn_cast<DeclRefExpr>(currentSubExpr))
+          emit = !isFunctionOrCatchVar(tmp);
+        else if (isa<CallExpr>(currentSubExpr))
+          emit = true;
+      }
+    }
+    if (emit)
+      diag(subExpr->getLocStart(),
+           "throw expression should throw anonymous temporary values instead");
+  }
+}
+
+void ThrowByValueCatchByReferenceCheck::diagnoseCatchLocations(
+    const CXXCatchStmt *catchStmt, ASTContext &context) {
+  const char *diagMsgCatchReference = "catch handler catches a pointer value; "
+                                      "should throw a non-pointer value and "
+                                      "catch by reference instead";
+  if (!catchStmt)
+    return;
+  auto caughtType = catchStmt->getCaughtType();
+  if (caughtType.isNull())
+    return;
+  auto *varDecl = catchStmt->getExceptionDecl();
+  if (const auto *PT = caughtType.getCanonicalType()->getAs<PointerType>()) {
+    // We do not diagnose when catching pointer to strings since we also allow
+    // throwing string literals.
+    if (!PT->getPointeeType()->isAnyCharacterType())
+      diag(varDecl->getLocStart(), diagMsgCatchReference);
+  } else if (!caughtType->isReferenceType()) {
+    // If it's not a pointer and not a reference then it must be thrown "by
+    // value". In this case we should emit a diagnosis message unless the type
+    // is trivial.
+    if (!caughtType.isTrivialType(context))
+      diag(varDecl->getLocStart(), diagMsgCatchReference);
+  }
+}
+
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/misc/ThrowByValueCatchByReferenceCheck.h

@@ -0,0 +1,49 @@
+//===--- ThrowByValueCatchByReferenceCheck.h - clang-tidy--------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_THROW_BY_VALUE_CATCH_BY_REFERENCE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_THROW_BY_VALUE_CATCH_BY_REFERENCE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+///\brief checks for locations that do not throw by value
+// or catch by reference.
+// The check is C++ only. It checks that all throw locations
+// throw by value and not by pointer. Additionally it
+// contains an option ("CheckThrowTemporaries", default value "true") that
+// checks that thrown objects are anonymous temporaries. It is also
+// acceptable for this check to throw string literals.
+// This test checks that exceptions are caught by reference
+// and not by value or pointer. It will not warn when catching
+// pointer to char, wchar_t, char16_t or char32_t. This is
+// due to not warning on throwing string literals.
+class ThrowByValueCatchByReferenceCheck : public ClangTidyCheck {
+public:
+  ThrowByValueCatchByReferenceCheck(StringRef Name, ClangTidyContext *Context);
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  void diagnoseThrowLocations(const CXXThrowExpr *throwExpr);
+  void diagnoseCatchLocations(const CXXCatchStmt *catchStmt,
+                              ASTContext &context);
+  bool isFunctionParameter(const DeclRefExpr *declRefExpr);
+  bool isCatchVariable(const DeclRefExpr *declRefExpr);
+  bool isFunctionOrCatchVar(const DeclRefExpr *declRefExpr);
+  const bool CheckAnonymousTemporaries;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_THROW_BY_VALUE_CATCH_BY_REFERENCE_H

clang-tools-extra/test/clang-tidy/misc-throw-by-value-catch-by-reference.cpp

@@ -0,0 +1,156 @@
+// RUN: %python %S/check_clang_tidy.py %s misc-throw-by-value-catch-by-reference %t
+
+
+class logic_error {
+public:
+  logic_error(const char *message) {}
+};
+
+typedef logic_error *logic_ptr;
+typedef logic_ptr logic_double_typedef;
+
+int lastException;
+
+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 <typename T> typename remove_reference<T>::type &&move(T &&arg) {
+  return static_cast<typename remove_reference<T>::type &&>(arg);
+}
+
+logic_error CreateException() { return logic_error("created"); }
+
+void testThrowFunc() {
+  throw new logic_error("by pointer");
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: throw expression throws a pointer; it should throw a non-pointer value instead [misc-throw-by-value-catch-by-reference]
+  logic_ptr tmp = new logic_error("by pointer");
+  throw tmp;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: throw expression should throw anonymous temporary values instead [misc-throw-by-value-catch-by-reference]
+  // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: throw expression throws a pointer; it should throw a non-pointer value instead [misc-throw-by-value-catch-by-reference]
+  throw logic_error("by value");
+  auto *literal = "test";
+  throw logic_error(literal);
+  throw "test string literal";
+  throw L"throw wide string literal";
+  const char *characters = 0;
+  throw characters;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: throw expression should throw anonymous temporary values instead [misc-throw-by-value-catch-by-reference]
+  // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: throw expression throws a pointer; it should throw a non-pointer value instead [misc-throw-by-value-catch-by-reference]
+  logic_error lvalue("lvalue");
+  throw lvalue;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: throw expression should throw anonymous temporary values instead [misc-throw-by-value-catch-by-reference]
+
+  throw move(lvalue);
+  int &ex = lastException;
+  throw ex;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: throw expression should throw anonymous temporary values instead [misc-throw-by-value-catch-by-reference]
+  throw CreateException();
+}
+
+void throwReferenceFunc(logic_error &ref) { throw ref; }
+
+void catchByPointer() {
+  try {
+    testThrowFunc();
+  } catch (logic_error *e) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: catch handler catches a pointer value; should throw a non-pointer value and catch by reference instead [misc-throw-by-value-catch-by-reference]
+  }
+}
+
+void catchByValue() {
+  try {
+    testThrowFunc();
+  } catch (logic_error e) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: catch handler catches a pointer value; should throw a non-pointer value and catch by reference instead [misc-throw-by-value-catch-by-reference]
+  }
+}
+
+void catchByReference() {
+  try {
+    testThrowFunc();
+  } catch (logic_error &e) {
+  }
+}
+
+void catchByConstReference() {
+  try {
+    testThrowFunc();
+  } catch (const logic_error &e) {
+  }
+}
+
+void catchTypedef() {
+  try {
+    testThrowFunc();
+  } catch (logic_ptr) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: catch handler catches a pointer value; should throw a non-pointer value and catch by reference instead [misc-throw-by-value-catch-by-reference]
+  }
+}
+
+void catchAll() {
+  try {
+    testThrowFunc();
+  } catch (...) {
+  }
+}
+
+void catchLiteral() {
+  try {
+    testThrowFunc();
+  } catch (const char *) {
+  } catch (const wchar_t *) {
+    // disabled for now until it is clear
+    // how to enable them in the test
+    //} catch (const char16_t*) {
+    //} catch (const char32_t*) {
+  }
+}
+
+// catching fundamentals should not warn
+void catchFundamental() {
+  try {
+    testThrowFunc();
+  } catch (int) {
+  } catch (double) {
+  } catch (unsigned long) {
+  }
+}
+
+struct TrivialType {
+  double x;
+  double y;
+};
+
+void catchTrivial() {
+  try {
+    testThrowFunc();
+  } catch (TrivialType) {
+  }
+}
+
+typedef logic_error &fine;
+void additionalTests() {
+  try {
+  } catch (int i) {  // ok
+    throw i;         // ok
+  } catch (fine e) { // ok
+    throw e;         // ok
+  } catch (logic_error *e) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: catch handler catches a pointer value; should throw a non-pointer value and catch by reference instead [misc-throw-by-value-catch-by-reference]
+    throw e;      // ok, despite throwing a pointer
+  } catch (...) { // ok
+    throw;        // ok
+  }
+}
+
+struct S {};
+
+S &returnByReference();
+S returnByValue();
+
+void f() {
+  throw returnByReference(); // Should diagnose
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: throw expression should throw anonymous temporary values instead [misc-throw-by-value-catch-by-reference]
+  throw returnByValue(); // Should not diagnose
+}