[clang-tidy] Add new check 'bugprone-unhandled-exception-at-new'.

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

73

Please rebase from trunk.

79

Please indent.

[njames93]

clang-tools-extra/clang-tidy/bugprone/UnhandledExceptionAtNewCheck.cpp

24–25

This can leak bound nodes If it doesn't match.

clang-tools-extra/clang-tidy/bugprone/UnhandledExceptionAtNewCheck.h

29

I'd suggest this check is only ran when exceptions are enabled.

[balazske]

Rebase, fixes according to review comments.

[balazske]

Ping.

[aaron.ballman]

clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-exception-at-new.rst

18

clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-exception-at-new.cpp

21

It would be useful to also have a test with a function-try-block to ensure those are handled properly.

144

You have tests for placement new with nothrow_t, but I think other forms of placement new are also very interesting to test as those typically don't throw.

Additionally, perhaps tests where the allocation functions have been replaced by the user (such as a class allocation function)?

[balazske]

clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-exception-at-new.cpp

144

I realized that the case of user-defined constructor or allocation function allows to throw any kind of exception. So the check should be improved to handle this case: Not rely on the syntax of new expression, instead check if the called allocation function or the called constructor may throw, and if yes, check what exceptions are possible. What is your opinion, is it a better approach?
(And a function to collect all possible exceptions called from a function is needed, ExceptionAnalyzer seems usable.)

[balazske]

clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-exception-at-new.cpp

144

It looks like that the user is free to define custom operator new and any constructor called that may throw any exception. Even in the "nothrow" case it is possible to use a constructor that may throw? If we would analyze every possible throwable exception that may come out from a new-expression, the checker would end up almost in a general checker that checks for uncaught exceptions. At least it is easy to extend.

[balazske]

Use mayThrow, improve test (user and class-specific cases).

[aaron.ballman]

clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-exception-at-new.cpp

144

Not rely on the syntax of new expression, instead check if the called allocation function or the called constructor may throw, and if yes, check what exceptions are possible. What is your opinion, is it a better approach?

I think that's a better approach.

144

Even in the "nothrow" case it is possible to use a constructor that may throw?

Certainly -- the nothrow syntax is specifying that the allocation cannot throw (it either returns a valid pointer to the allocated memory or null), not that the constructor cannot throw if the allocation succeeds.

Is the check intended to care about *allocations* that fail or just exceptions in general around new expressions? If it's allocations that fail, then I wouldn't worry about the ctor's exception specification. If exceptions in general, then I agree that we're talking about a general checker for all uncaught exceptions in some ways.

[balazske]

Removed check of possible exceptions from constructor call.

[balazske]

Rename the check to "unhandled-bad_alloc" (or similar")? And the error message to " missing exception handler 'std::bad_alloc' "?

[balazske]

Fix a crash, update documentation.

[balazske]

Ping.
The check now handles only check of allocation failure at new.

[aaron.ballman]

clang-tools-extra/clang-tidy/bugprone/UnhandledExceptionAtNewCheck.cpp

28–30

I think this should move above the call to InnertMatcher.matches() so that the inner matcher doesn't have to worry quite as much about getting null types.

clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-exception-at-new.rst

7

This isn't quite accurate -- if the exception handler is missing or doesn't handle either std::bad_alloc or std::exception.

17

clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-exception-at-new.cpp

20

Another interesting test case would be when the user subclasses std::bad_alloc and throws errors of the subclass type from the allocation function which are in/correctly caught.

21

Still missing this test -- you should add one like:

void func() try {
  int *i = new int;
} catch (const std::bad_alloc &) {
}

[balazske]

Rebase, changed documentation, small fix in the code, more tests added.

clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-exception-at-new.rst

7

I restructure the text, but do not want to mention std::exception because it is a base class (handling of std::exception means handling of std::bad_alloc), and then why not mention generic exception handler (catch(...)). I think this text is for introduction only, should not be totally precise.

[balazske]

clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-exception-at-new.cpp

20

In this case the check must find every (visible) subclass of std::bad_alloc check if there is a catch block for every case. Even then a custom allocation function may throw other classes (not derived from std::bad_alloc) or classes (may be inherited from std::bad_alloc but not visible in the TU) that are not checked. So this is still a partial solution, probably not better than the rule that a handler for bad_alloc (or more generic) should exist.

[aaron.ballman]

LGTM!

clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-exception-at-new.rst

7

The restructured text is good -- I just wanted the information *somewhere* so that users weren't surprised by the behavior.

clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-exception-at-new.cpp

20

So this is still a partial solution, probably not better than the rule that a handler for bad_alloc (or more generic) should exist.

Okay, that's fair -- we can always address it in follow-up if it turns out to be an issue in practice.

[balazske]

Add another test.

[dyung]

This appears to be failing on the PS4 linux bot likely due to the PS4 target has exceptions disabled by default. Can you take a look?

https://lab.llvm.org/buildbot/#/builders/139/builds/2441

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

 #include "SuspiciousSemicolonCheck.h"
 #include "SuspiciousStringCompareCheck.h"
 #include "SwappedArgumentsCheck.h"
 #include "TerminatingContinueCheck.h"
 #include "ThrowKeywordMissingCheck.h"
 #include "TooSmallLoopVariableCheck.h"
 #include "UndefinedMemoryManipulationCheck.h"
 #include "UndelegatedConstructorCheck.h"
+#include "UnhandledExceptionAtNewCheck.h"
 #include "UnhandledSelfAssignmentCheck.h"
 #include "UnusedRaiiCheck.h"
 #include "UnusedReturnValueCheck.h"
 #include "UseAfterMoveCheck.h"
 #include "VirtualNearMissCheck.h"
 
 namespace clang {
 namespace tidy {
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<TooSmallLoopVariableCheck>(
         "bugprone-too-small-loop-variable");
     CheckFactories.registerCheck<UndefinedMemoryManipulationCheck>(
         "bugprone-undefined-memory-manipulation");
     CheckFactories.registerCheck<UndelegatedConstructorCheck>(
         "bugprone-undelegated-constructor");
     CheckFactories.registerCheck<UnhandledSelfAssignmentCheck>(
         "bugprone-unhandled-self-assignment");
+    CheckFactories.registerCheck<UnhandledExceptionAtNewCheck>(
+        "bugprone-unhandled-exception-at-new");
     CheckFactories.registerCheck<UnusedRaiiCheck>(
         "bugprone-unused-raii");
     CheckFactories.registerCheck<UnusedReturnValueCheck>(
         "bugprone-unused-return-value");
     CheckFactories.registerCheck<UseAfterMoveCheck>(
         "bugprone-use-after-move");
     CheckFactories.registerCheck<VirtualNearMissCheck>(
         "bugprone-virtual-near-miss");

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

add_clang_library(clangTidyBugproneModule
   SuspiciousSemicolonCheck.cpp
   SuspiciousStringCompareCheck.cpp
   SwappedArgumentsCheck.cpp
   TerminatingContinueCheck.cpp
   ThrowKeywordMissingCheck.cpp
   TooSmallLoopVariableCheck.cpp
   UndefinedMemoryManipulationCheck.cpp
   UndelegatedConstructorCheck.cpp
+  UnhandledExceptionAtNewCheck.cpp
   UnhandledSelfAssignmentCheck.cpp
   UnusedRaiiCheck.cpp
   UnusedReturnValueCheck.cpp
   UseAfterMoveCheck.cpp
   VirtualNearMissCheck.cpp
 
   LINK_LIBS
   clangTidy

clang-tools-extra/clang-tidy/bugprone/UnhandledExceptionAtNewCheck.h

+//===--- UnhandledExceptionAtNewCheck.h - clang-tidy ------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNHANDLEDEXCEPTIONATNEWCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNHANDLEDEXCEPTIONATNEWCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Finds calls to 'new' that may throw unhandled exception at allocation
+/// failure.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-unhandled-exception-at-new.html
+class UnhandledExceptionAtNewCheck : public ClangTidyCheck {
+public:
+  UnhandledExceptionAtNewCheck(StringRef Name, ClangTidyContext *Context);
+
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus && LangOpts.CXXExceptions;
+  }
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNHANDLEDEXCEPTIONATNEWCHECK_H

clang-tools-extra/clang-tidy/bugprone/UnhandledExceptionAtNewCheck.cpp

+//===--- UnhandledExceptionAtNewCheck.cpp - clang-tidy --------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "UnhandledExceptionAtNewCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+AST_MATCHER_P(CXXTryStmt, hasHandlerFor,
+              ast_matchers::internal::Matcher<QualType>, InnerMatcher) {
+  for (unsigned NH = Node.getNumHandlers(), I = 0; I < NH; ++I) {
+    const CXXCatchStmt *CatchS = Node.getHandler(I);
+    ast_matchers::internal::BoundNodesTreeBuilder Result(*Builder);
+    if (InnerMatcher.matches(CatchS->getCaughtType(), Finder, &Result)) {
+      *Builder = std::move(Result);
+      return true;
+    }
+    // Generic catch handler should match anything.
+    if (CatchS->getCaughtType().isNull())
+      return true;
+  }
+  return false;
+}
+
+AST_MATCHER(CXXNewExpr, mayThrow) {
+  FunctionDecl *OperatorNew = Node.getOperatorNew();
+  if (!OperatorNew)
+    return false;
+  return !OperatorNew->getType()->castAs<FunctionProtoType>()->isNothrow();
+}
+
+UnhandledExceptionAtNewCheck::UnhandledExceptionAtNewCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context) {}
+
+void UnhandledExceptionAtNewCheck::registerMatchers(MatchFinder *Finder) {
+  auto BadAllocType =
+      recordType(hasDeclaration(cxxRecordDecl(hasName("::std::bad_alloc"))));
+  auto ExceptionType =
+      recordType(hasDeclaration(cxxRecordDecl(hasName("::std::exception"))));
+  auto BadAllocReferenceType = referenceType(pointee(BadAllocType));
+  auto ExceptionReferenceType = referenceType(pointee(ExceptionType));
+
+  auto CatchBadAllocType =
+      qualType(hasCanonicalType(anyOf(BadAllocType, BadAllocReferenceType,
+                                      ExceptionType, ExceptionReferenceType)));
+  auto BadAllocCatchingTryBlock = cxxTryStmt(hasHandlerFor(CatchBadAllocType));
+
+  auto FunctionMayNotThrow = functionDecl(isNoThrow());
+
+  Finder->addMatcher(cxxNewExpr(mayThrow(),
+                                unless(hasAncestor(BadAllocCatchingTryBlock)),
+                                hasAncestor(FunctionMayNotThrow))
+                         .bind("new-expr"),
+                     this);
+}
+
+void UnhandledExceptionAtNewCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *MatchedExpr = Result.Nodes.getNodeAs<CXXNewExpr>("new-expr");
+  if (MatchedExpr)
+    diag(MatchedExpr->getBeginLoc(),
+         "missing exception handler for allocation failure at 'new'");
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
 - The `run-clang-tidy.py` helper script is now installed in `bin/` as
   `run-clang-tidy`. It was previously installed in `share/clang/`.
 
 - Added command line option `--fix-notes` to apply fixes found in notes
   attached to warnings. These are typically cases where we are less confident
   the fix will have the desired effect.
 
 New checks
 ^^^^^^^^^^
 
 - New :doc:`concurrency-thread-canceltype-asynchronous
   <clang-tidy/checks/concurrency-thread-canceltype-asynchronous>` check.
 
   Finds ``pthread_setcanceltype`` function calls where a thread's cancellation
   type is set to asynchronous.
 
 - New :doc:`altera-unroll-loops
   <clang-tidy/checks/altera-unroll-loops>` check.
 
   Finds inner loops that have not been unrolled, as well as fully unrolled
   loops with unknown loops bounds or a large number of iterations.
 
 - New :doc:`cppcoreguidelines-prefer-member-initializer
   <clang-tidy/checks/cppcoreguidelines-prefer-member-initializer>` check.
 
   Finds member initializations in the constructor body which can be placed into
   the initialization list instead.
 
+- New :doc:`bugprone-unhandled-exception-at-new
+  <clang-tidy/checks/bugprone-unhandled-exception-at-new>` check.
+
+  Finds calls to ``new`` that may throw ``std::bad_alloc`` exception and
+  the exception handler is missing.
+
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
 - New alias :doc:`cert-pos47-c
   <clang-tidy/checks/cert-pos47-c>` to
   :doc:`concurrency-thread-canceltype-asynchronous
   <clang-tidy/checks/concurrency-thread-canceltype-asynchronous>` was added.
 

clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-exception-at-new.rst

+.. title:: clang-tidy - bugprone-unhandled-exception-at-new
+
+bugprone-unhandled-exception-at-new
+===================================
+
+Finds calls to ``new`` that may throw ``std::bad_alloc`` exception and
+the exception handler is missing.
+
+.. code-block:: c++
+
+  int *f() noexcept {
+    int *p = new int[1000];
+    // ...
+    return p;
+  }
+
+Calls to ``new`` can throw exception of type ``bad_alloc`` that should be
+handled by the code. Alternatively the nonthrowing form of ``new`` can be
+used. The check verifies that the exception is handled in the function
+that calls ``new``, unless a nonthrowing version is used or the exception
+is allowed to propagate out of the function. It is assumed that any
+user-defined ``operator new`` is either ``noexcept`` or may throw an
+exception of type ``std::bad_alloc`` (or derived from it). Other exception
+types or exceptions occuring in the objects's constructor are not taken into
+account.

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `bugprone-suspicious-semicolon <bugprone-suspicious-semicolon.html>`_, "Yes"
    `bugprone-suspicious-string-compare <bugprone-suspicious-string-compare.html>`_, "Yes"
    `bugprone-swapped-arguments <bugprone-swapped-arguments.html>`_, "Yes"
    `bugprone-terminating-continue <bugprone-terminating-continue.html>`_, "Yes"
    `bugprone-throw-keyword-missing <bugprone-throw-keyword-missing.html>`_,
    `bugprone-too-small-loop-variable <bugprone-too-small-loop-variable.html>`_,
    `bugprone-undefined-memory-manipulation <bugprone-undefined-memory-manipulation.html>`_,
    `bugprone-undelegated-constructor <bugprone-undelegated-constructor.html>`_,
+   `bugprone-unhandled-exception-at-new <bugprone-unhandled-exception-at-new.html>`_,
    `bugprone-unhandled-self-assignment <bugprone-unhandled-self-assignment.html>`_,
    `bugprone-unused-raii <bugprone-unused-raii.html>`_, "Yes"
    `bugprone-unused-return-value <bugprone-unused-return-value.html>`_,
    `bugprone-use-after-move <bugprone-use-after-move.html>`_,
    `bugprone-virtual-near-miss <bugprone-virtual-near-miss.html>`_, "Yes"
    `cert-dcl21-cpp <cert-dcl21-cpp.html>`_,
    `cert-dcl50-cpp <cert-dcl50-cpp.html>`_,
    `cert-dcl58-cpp <cert-dcl58-cpp.html>`_,

clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-exception-at-new.cpp

+// RUN: %check_clang_tidy -std=c++14 %s bugprone-unhandled-exception-at-new %t
+
+namespace std {
+typedef __typeof__(sizeof(0)) size_t;
+enum class align_val_t : std::size_t {};
+class exception {};
+class bad_alloc : public exception {};
+struct nothrow_t {};
+extern const nothrow_t nothrow;
+} // namespace std
+
+void *operator new(std::size_t, const std::nothrow_t &) noexcept;
+void *operator new(std::size_t, std::align_val_t, const std::nothrow_t &) noexcept;
+void *operator new(std::size_t, void *) noexcept;
+
+class A {};
+typedef std::bad_alloc badalloc1;
+using badalloc2 = std::bad_alloc;
+using badalloc3 = std::bad_alloc &;
+
+void *operator new(std::size_t, int, int);
+void *operator new(std::size_t, int, int, int) noexcept;
+
+struct ClassSpecificNew {
+  void *operator new(std::size_t);
+  void *operator new(std::size_t, std::align_val_t);
+  void *operator new(std::size_t, int, int) noexcept;
+  void *operator new(std::size_t, int, int, int);
+};
+
+void f1() noexcept {
+  int *I1 = new int;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: missing exception handler for allocation failure at 'new'
+  try {
+    int *I2 = new int;
+    try {
+      int *I3 = new int;
+    } catch (A) {
+    }
+  } catch (std::bad_alloc) {
+  }
+
+  try {
+    int *I = new int;
+  } catch (std::bad_alloc &) {
+  }
+
+  try {
+    int *I = new int;
+  } catch (const std::bad_alloc &) {
+  }
+
+  try {
+    int *I = new int;
+  } catch (badalloc1) {
+  }
+
+  try {
+    int *I = new int;
+  } catch (badalloc1 &) {
+  }
+
+  try {
+    int *I = new int;
+  } catch (const badalloc1 &) {
+  }
+
+  try {
+    int *I = new int;
+  } catch (badalloc2) {
+  }
+
+  try {
+    int *I = new int;
+  } catch (badalloc3) {
+  }
+
+  try {
+    int *I = new int;
+    // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: missing exception handler for allocation failure at 'new'
+  } catch (std::bad_alloc *) {
+  }
+
+  try {
+    int *I = new int;
+    // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: missing exception handler for allocation failure at 'new'
+  } catch (A) {
+  }
+}
+
+void f2() noexcept {
+  try {
+    int *I = new int;
+  } catch (A) {
+  } catch (std::bad_alloc) {
+  }
+
+  try {
+    int *I = new int;
+  } catch (...) {
+  }
+
+  try {
+    int *I = new int;
+  } catch (const std::exception &) {
+  }
+}
+
+void f_new_nothrow() noexcept {
+  int *I1 = new (std::nothrow) int;
+  int *I2 = new (static_cast<std::align_val_t>(1), std::nothrow) int;
+}
+
+void f_new_placement() noexcept {
+  char buf[100];
+  int *I = new (buf) int;
+}
+
+void f_new_user_defined() noexcept {
+  int *I1 = new (1, 2) int;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: missing exception handler for allocation failure at 'new'
+  int *I2 = new (1, 2, 3) int;
+}
+
+void f_class_specific() noexcept {
+  ClassSpecificNew *N1 = new ClassSpecificNew;
+  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: missing exception handler for allocation failure at 'new'
+  ClassSpecificNew *N2 = new (static_cast<std::align_val_t>(1)) ClassSpecificNew;
+  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: missing exception handler for allocation failure at 'new'
+  ClassSpecificNew *N3 = new (1, 2) ClassSpecificNew;
+  ClassSpecificNew *N4 = new (1, 2, 3) ClassSpecificNew;
+  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: missing exception handler for allocation failure at 'new'
+}
+
+void f_est_none() {
+  int *I = new int;
+}
+
+void f_est_noexcept_false() noexcept(false) {
+  int *I = new int;
+}
+
+void f_est_noexcept_true() noexcept(true) {
+  int *I = new int;
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: missing exception handler for allocation failure at 'new'
+}
+
+void f_est_dynamic_none() throw() {
+  int *I = new int;
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: missing exception handler for allocation failure at 'new'
+}
+
+void f_est_dynamic_1() throw(std::bad_alloc) {
+  int *I = new int;
+}
+
+void f_est_dynamic_2() throw(A) {
+  // the exception specification list is not checked
+  int *I = new int;
+}
+
+template <bool P>
+void f_est_noexcept_dependent_unused() noexcept(P) {
+  int *I = new int;
+}
+
+template <bool P>
+void f_est_noexcept_dependent_used() noexcept(P) {
+  int *I = new int;
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: missing exception handler for allocation failure at 'new'
+}
+
+template <class T>
+void f_dependent_new() {
+  T *X = new T;
+}
+
+void f_1() {
+  f_est_noexcept_dependent_used<true>();
+}