[clang-tidy] New option for misc-throw-by-value-catch-by-reference

[baloghadamsoftware]

Summary

Catching trivial objects by value is not dangerous but may be inefficient if they are too large. This patch adds an option WarnOnLargeObject to the checker to also warn if such an object is caught by value. An object is considered as "large" if its size is greater than MaxSize which is another option. Default value is the machine word of the architecture (size of the type size_t).

[torbjoernk]

docs/clang-tidy/checks/misc-throw-by-value-catch-by-reference.rst

35–46

I think it might be worth adding a note on the fallback to sizeof(size_t) if MaxSize is not set or manually set to std::numeric_limits<uint64_t>::max() by the user.

[aaron.ballman]

clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp

25

Missing a full stop at the end of the comment.

docs/clang-tidy/checks/misc-throw-by-value-catch-by-reference.rst

37–40

large trivial object -> large, trivial object
affects the perofrmance -> affects the performance
using option MaxSize -> using the MaxSize option.

[lebedev.ri]

docs/ReleaseNotes.rst

194–196

for check :doc:..<..> check

(repeated 'check' word)

[baloghadamsoftware]

Updated according to the comments.

[lebedev.ri]

Looks good

clang-tidy/misc/ThrowByValueCatchByReferenceCheck.h

45

because

[aaron.ballman]

LGTM (with the typo fix).

clang-tidy/misc/ThrowByValueCatchByReferenceCheck.h

 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;
+  const bool WarnOnLargeObject;
+  uint64_t MaxSize; // No `const` bacause we have to set it in two steps.
 };
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_THROW_BY_VALUE_CATCH_BY_REFERENCE_H

clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp

 
 namespace clang {
 namespace tidy {
 namespace misc {
 
 ThrowByValueCatchByReferenceCheck::ThrowByValueCatchByReferenceCheck(
     StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
-      CheckAnonymousTemporaries(Options.get("CheckThrowTemporaries", true)) {}
+      CheckAnonymousTemporaries(Options.get("CheckThrowTemporaries", true)),
+      WarnOnLargeObject(Options.get("WarnOnLargeObject", false)),
+      // Cannot access `ASTContext` from here so set it to an extremal value.
+      MaxSize(Options.get("MaxSize", std::numeric_limits<uint64_t>::max())) {}
 
 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);
if (const auto *PT = caughtType.getCanonicalType()->getAs<PointerType>()) {
     if (!PT->getPointeeType()->isAnyCharacterType())
       diag(varDecl->getBeginLoc(), diagMsgCatchReference);
   } else if (!caughtType->isReferenceType()) {
     const char *diagMsgCatchReference = "catch handler catches by value; "
                                         "should catch by reference instead";
     // If it's not a pointer and not a reference then it must be caught "by
     // value". In this case we should emit a diagnosis message unless the type
     // is trivial.
-    if (!caughtType.isTrivialType(context))
+    if (!caughtType.isTrivialType(context)) {
       diag(varDecl->getBeginLoc(), diagMsgCatchReference);
+    } else if (WarnOnLargeObject) {
+      // If the type is trivial, then catching it by reference is not dangerous.
+      // However, catching large objects by value decreases the performance.
+
+      // We can now access `ASTContext` so if `MaxSize` is an extremal value
+      // then set it to the size of `size_t`.
+      if (MaxSize == std::numeric_limits<uint64_t>::max())
+        MaxSize = context.getTypeSize(context.getSizeType());
+      if (context.getTypeSize(caughtType) > MaxSize)
+        diag(varDecl->getBeginLoc(), diagMsgCatchReference);
+    }
   }
 }
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang

docs/ReleaseNotes.rst

- New :doc:`llvm-prefer-isa-or-dyn-cast-in-conditionals
   finds and replaces cases that match the pattern ``var &&
   isa<X>(var)``, where ``var`` is evaluated twice.
 
 - New :doc:`modernize-use-trailing-return-type
   <clang-tidy/checks/modernize-use-trailing-return-type>` check.
 
   Rewrites function signatures to use a trailing return type.
 
+- The :doc:`misc-throw-by-value-catch-by-reference
+  <clang-tidy/misc-throw-by-value-catch-by-reference.rst>` now supports
+  `WarnOnLargeObject` and `MaxSize` options to warn on any large trivial
+  object caught by value.
+
 Improvements to include-fixer
 -----------------------------
 
 - New :doc:`openmp-exception-escape
   <clang-tidy/checks/openmp-exception-escape>` check.
 
   Analyzes OpenMP Structured Blocks and checks that no exception escapes
   out of the Structured Block it was thrown in.
 
 - New :doc:`openmp-use-default-none
   <clang-tidy/checks/openmp-use-default-none>` check.
 
   Finds OpenMP directives that are allowed to contain a ``default`` clause,
   but either don't specify it or the clause is specified but with the kind
   other than ``none``, and suggests to use the ``default(none)`` clause.
 
 Improvements to clang-include-fixer
 -----------------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...
 
 Improvements to pp-trace
 ------------------------
 
 - Added a new option `-callbacks` to filter preprocessor callbacks. It replaces
   the `-ignore` option.

docs/clang-tidy/checks/misc-throw-by-value-catch-by-reference.rst

 -------
 
 .. option:: CheckThrowTemporaries
 
    Triggers detection of violations of the rule `Throw anonymous temporaries
    <https://www.securecoding.cert.org/confluence/display/cplusplus/ERR09-CPP.+Throw+anonymous+temporaries>`_.
    Default is `1`.
 
+.. option:: WarnOnLargeObject
+
+   Also warns for any large, trivial object caught by value. Catching a large
+   object by value is not dangerous but affects the performance negatively. The
+   maximum size of an object allowed to be caught without warning can be set
+   using the `MaxSize` option.
+   Default is `0`.
+
+.. option:: MaxSize
+
+   Determines the maximum size of an object allowed to be caught without
+   warning. Only applicable if `WarnOnLargeObject` is set to `1`. If option is
+   set by the user to `std::numeric_limits<uint64_t>::max()` then it reverts to
+   the default value.
+   Default is the size of `size_t`.

test/clang-tidy/misc-throw-by-value-catch-by-reference-warn-on-large-object.cpp

+// RUN: %check_clang_tidy %s misc-throw-by-value-catch-by-reference %t -- -config="{CheckOptions: [{key: misc-throw-by-value-catch-by-reference.WarnOnLargeObject, value: 1}]}" -- -std=c++11 -fcxx-exceptions
+
+struct SmallType {
+  long n;
+};
+
+struct LargeType {
+  long v[255];
+};
+
+void testThrowFunc() {
+  throw SmallType();
+  throw LargeType();
+}
+
+void catchSmall() {
+  try {
+    testThrowFunc();
+  } catch (SmallType s) {
+  }
+}
+
+void catchLarge() {
+  try {
+    testThrowFunc();
+  } catch (LargeType l) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: catch handler catches by value; should catch by reference instead [misc-throw-by-value-catch-by-reference]
+  }
+}
+
+void catchByReference() {
+  try {
+    testThrowFunc();
+  } catch (LargeType &l) {
+  }
+}