https://github.com/PiotrZSL updated https://github.com/llvm/llvm-project/pull/86448
>From 1b5c351acca7375c577111a4c55506146ef108ef Mon Sep 17 00:00:00 2001 From: Piotr Zegar <piotr.ze...@nokia.com> Date: Sun, 24 Mar 2024 18:39:54 +0000 Subject: [PATCH 1/6] [clang-tidy] Added bugprone-exception-rethrow check Identifies problematic exception rethrowing, especially with caught exception variables or empty throw statements outside catch blocks. Closes #71292 --- .../bugprone/BugproneTidyModule.cpp | 3 + .../clang-tidy/bugprone/CMakeLists.txt | 1 + .../bugprone/ExceptionRethrowCheck.cpp | 50 ++++++++++++++ .../bugprone/ExceptionRethrowCheck.h | 37 ++++++++++ clang-tools-extra/docs/ReleaseNotes.rst | 6 ++ .../checks/bugprone/exception-rethrow.rst | 68 +++++++++++++++++++ .../docs/clang-tidy/checks/list.rst | 1 + .../checkers/bugprone/exception-rethrow.cpp | 60 ++++++++++++++++ 8 files changed, 226 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/bugprone/ExceptionRethrowCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/bugprone/ExceptionRethrowCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-rethrow.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-rethrow.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index 1b92d2e60cc17..7466d3f2e4fc2 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -26,6 +26,7 @@ #include "EasilySwappableParametersCheck.h" #include "EmptyCatchCheck.h" #include "ExceptionEscapeCheck.h" +#include "ExceptionRethrowCheck.h" #include "FoldInitTypeCheck.h" #include "ForwardDeclarationNamespaceCheck.h" #include "ForwardingReferenceOverloadCheck.h" @@ -127,6 +128,8 @@ class BugproneModule : public ClangTidyModule { CheckFactories.registerCheck<EmptyCatchCheck>("bugprone-empty-catch"); CheckFactories.registerCheck<ExceptionEscapeCheck>( "bugprone-exception-escape"); + CheckFactories.registerCheck<ExceptionRethrowCheck>( + "bugprone-exception-rethrow"); CheckFactories.registerCheck<FoldInitTypeCheck>("bugprone-fold-init-type"); CheckFactories.registerCheck<ForwardDeclarationNamespaceCheck>( "bugprone-forward-declaration-namespace"); diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index 2d303191f8865..345ae420398e6 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -20,6 +20,7 @@ add_clang_library(clangTidyBugproneModule EasilySwappableParametersCheck.cpp EmptyCatchCheck.cpp ExceptionEscapeCheck.cpp + ExceptionRethrowCheck.cpp FoldInitTypeCheck.cpp ForwardDeclarationNamespaceCheck.cpp ForwardingReferenceOverloadCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/ExceptionRethrowCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ExceptionRethrowCheck.cpp new file mode 100644 index 0000000000000..4855ccc2724a9 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/ExceptionRethrowCheck.cpp @@ -0,0 +1,50 @@ +//===--- ExceptionRethrowCheck.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 "ExceptionRethrowCheck.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::bugprone { + +namespace { +AST_MATCHER(VarDecl, isExceptionVariable) { return Node.isExceptionVariable(); } +} // namespace + +void ExceptionRethrowCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + cxxThrowExpr(unless(isExpansionInSystemHeader()), + anyOf(unless(has(expr())), + has(declRefExpr(to(varDecl(isExceptionVariable()))))), + optionally(hasAncestor(cxxCatchStmt().bind("catch")))) + .bind("throw"), + this); +} + +void ExceptionRethrowCheck::check(const MatchFinder::MatchResult &Result) { + const auto *MatchedThrow = Result.Nodes.getNodeAs<CXXThrowExpr>("throw"); + + if (const Expr *ThrownObject = MatchedThrow->getSubExpr()) { + diag(MatchedThrow->getThrowLoc(), + "throwing a copy of the caught %0 exception, remove the argument to " + "throw the original exception object") + << ThrownObject->getType().getNonReferenceType(); + return; + } + + const bool HasCatchAnsestor = + Result.Nodes.getNodeAs<Stmt>("catch") != nullptr; + if (!HasCatchAnsestor) { + diag(MatchedThrow->getThrowLoc(), + "empty 'throw' outside a catch block without an exception can trigger " + "'std::terminate'"); + } +} + +} // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/clang-tidy/bugprone/ExceptionRethrowCheck.h b/clang-tools-extra/clang-tidy/bugprone/ExceptionRethrowCheck.h new file mode 100644 index 0000000000000..bbb30f779cf25 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/ExceptionRethrowCheck.h @@ -0,0 +1,37 @@ +//===--- ExceptionRethrowCheck.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_EXCEPTIONRETHROWCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_EXCEPTIONRETHROWCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang::tidy::bugprone { + +/// Identifies problematic exception rethrowing, especially with caught +/// exception variables or empty throw statements outside catch blocks. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/exception-rethrow.html +class ExceptionRethrowCheck : public ClangTidyCheck { +public: + ExceptionRethrowCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + std::optional<TraversalKind> getCheckTraversalKind() const override { + return TK_IgnoreUnlessSpelledInSource; + } + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus && LangOpts.CXXExceptions; + } +}; + +} // namespace clang::tidy::bugprone + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_EXCEPTIONRETHROWCHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 3038d2b125f20..776edb1da2ad0 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -119,6 +119,12 @@ New checks Detects error-prone Curiously Recurring Template Pattern usage, when the CRTP can be constructed outside itself and the derived class. +- New :doc:`bugprone-exception-rethrow + <clang-tidy/checks/bugprone/exception-rethrow>` check. + + Identifies problematic exception rethrowing, especially with caught exception + variables or empty throw statements outside catch blocks. + - New :doc:`bugprone-return-const-ref-from-parameter <clang-tidy/checks/bugprone/return-const-ref-from-parameter>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-rethrow.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-rethrow.rst new file mode 100644 index 0000000000000..981dfff852d58 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-rethrow.rst @@ -0,0 +1,68 @@ +.. title:: clang-tidy - bugprone-exception-rethrow + +bugprone-exception-rethrow +========================== + +Identifies problematic exception rethrowing, especially with caught exception +variables or empty throw statements outside catch blocks. + +In C++ exception handling, a common pitfall occurs when developers rethrow +caught exceptions within catch blocks by directly passing the caught exception +variable to the ``throw`` statement. While this approach can propagate +exceptions to higher levels of the program, it often leads to code that is less +clear and more error-prone. Rethrowing caught exceptions with the same exception +object within catch blocks can obscure the original context of the exception and +make it challenging to trace program flow. Additionally, this method can +introduce issues such as exception object slicing and performance overhead due +to the invocation of the copy constructor. + +.. code-block:: c++ + + try { + // Code that may throw an exception + } catch (const std::exception& e) { + throw e; // Bad + } + +To prevent these issues, it is advisable to utilize ``throw;`` statements to +rethrow the original exception object for currently handled exceptions. + +.. code-block:: c++ + + try { + // Code that may throw an exception + } catch (const std::exception& e) { + throw; // Good + } + +However, when empty throw statement is used outside of a catch block, it +will result in a call to ``std::terminate()``, which abruptly terminates the +application. This behavior can lead to abnormal termination of the program and +is often unintended. Such occurrences may indicate errors or oversights in the +exception handling logic, and it is essential to avoid empty throw statements +outside catch blocks to prevent unintended program termination. + +.. code-block:: c++ + + void foo() { + // std::terminate will be called because there is no exception to rethrow + throw; + } + + int main() { + try { + foo(); + } catch(...) { + return 1; + } + return 0; + } + +Above program will be terminated with: + +.. code:: text + + terminate called without an active exception + Aborted (core dumped) + + diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 49747ff896ba5..79e998a7d99c2 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -92,6 +92,7 @@ Clang-Tidy Checks :doc:`bugprone-easily-swappable-parameters <bugprone/easily-swappable-parameters>`, :doc:`bugprone-empty-catch <bugprone/empty-catch>`, :doc:`bugprone-exception-escape <bugprone/exception-escape>`, + :doc:`bugprone-exception-rethrow <bugprone/exception-rethrow>`, :doc:`bugprone-fold-init-type <bugprone/fold-init-type>`, :doc:`bugprone-forward-declaration-namespace <bugprone/forward-declaration-namespace>`, :doc:`bugprone-forwarding-reference-overload <bugprone/forwarding-reference-overload>`, diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-rethrow.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-rethrow.cpp new file mode 100644 index 0000000000000..5e7ba03652393 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-rethrow.cpp @@ -0,0 +1,60 @@ +// RUN: %check_clang_tidy %s bugprone-exception-rethrow %t -- -- -fexceptions + +struct exception {}; + +void correct() { + try { + throw exception(); + } catch(const exception &) { + throw; + } +} + +void correct2() { + try { + throw exception(); + } catch(const exception &e) { + try { + throw exception(); + } catch(...) {} + } +} + +void not_correct() { + try { + throw exception(); + } catch(const exception &e) { + throw e; +// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: throwing a copy of the caught 'exception' exception, remove the argument to throw the original exception object [bugprone-exception-rethrow] + } +} + +void not_correct2() { + try { + throw 5; + } catch(const int &e) { + throw e; +// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: throwing a copy of the caught 'int' exception, remove the argument to throw the original exception object [bugprone-exception-rethrow] + } +} + +void rethrow_not_correct() { + throw; +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: empty 'throw' outside a catch block without an exception can trigger 'std::terminate' [bugprone-exception-rethrow] +} + +void rethrow_not_correct2() { + try { + throw; +// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: empty 'throw' outside a catch block without an exception can trigger 'std::terminate' [bugprone-exception-rethrow] + } catch(...) { + } +} + +void rethrow_correct() { + try { + throw 5; + } catch(...) { + throw; + } +} >From 3d1ae3f17ec41116cb22569a82a7e4c369a355a6 Mon Sep 17 00:00:00 2001 From: Piotr Zegar <pitor.ze...@nokia.com> Date: Sun, 24 Mar 2024 18:43:47 +0000 Subject: [PATCH 2/6] Fix code -> code-block in doc --- .../docs/clang-tidy/checks/bugprone/exception-rethrow.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-rethrow.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-rethrow.rst index 981dfff852d58..68be515297bc5 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-rethrow.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-rethrow.rst @@ -60,7 +60,7 @@ outside catch blocks to prevent unintended program termination. Above program will be terminated with: -.. code:: text +.. code-block:: text terminate called without an active exception Aborted (core dumped) >From 3a6573e01e5532a71bd226f15ab2ed5844a51e17 Mon Sep 17 00:00:00 2001 From: Piotr Zegar <piotr.ze...@nokia.com> Date: Sun, 31 Mar 2024 12:12:43 +0000 Subject: [PATCH 3/6] Fixes for review --- .../bugprone/ExceptionRethrowCheck.cpp | 23 ++++++----- .../bugprone/ExceptionRethrowCheck.h | 4 +- .../checks/bugprone/exception-rethrow.rst | 1 - .../checkers/bugprone/exception-rethrow.cpp | 38 ++++++++++++++++++- 4 files changed, 52 insertions(+), 14 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/ExceptionRethrowCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ExceptionRethrowCheck.cpp index 4855ccc2724a9..3e327606ee562 100644 --- a/clang-tools-extra/clang-tidy/bugprone/ExceptionRethrowCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/ExceptionRethrowCheck.cpp @@ -8,20 +8,26 @@ #include "ExceptionRethrowCheck.h" #include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" using namespace clang::ast_matchers; namespace clang::tidy::bugprone { -namespace { -AST_MATCHER(VarDecl, isExceptionVariable) { return Node.isExceptionVariable(); } -} // namespace - void ExceptionRethrowCheck::registerMatchers(MatchFinder *Finder) { + + auto RefToExceptionVariable = declRefExpr(to(varDecl(isExceptionVariable()))); + auto StdMoveCall = callExpr(argumentCountIs(1), callee(functionDecl(hasName("::std::move"))), hasArgument(0, RefToExceptionVariable)); + auto CopyOrMoveConstruction = cxxConstructExpr(argumentCountIs(1), hasDeclaration(cxxConstructorDecl(anyOf(isCopyConstructor(), isMoveConstructor()))), hasArgument(0, anyOf(RefToExceptionVariable, StdMoveCall))); + auto FunctionCast = cxxFunctionalCastExpr( hasSourceExpression(anyOf(RefToExceptionVariable, StdMoveCall))); + Finder->addMatcher( cxxThrowExpr(unless(isExpansionInSystemHeader()), anyOf(unless(has(expr())), - has(declRefExpr(to(varDecl(isExceptionVariable()))))), + has(RefToExceptionVariable), + has(StdMoveCall), + has(CopyOrMoveConstruction), + has(FunctionCast)), optionally(hasAncestor(cxxCatchStmt().bind("catch")))) .bind("throw"), this); @@ -38,12 +44,11 @@ void ExceptionRethrowCheck::check(const MatchFinder::MatchResult &Result) { return; } - const bool HasCatchAnsestor = + const bool HasCatchAncestor = Result.Nodes.getNodeAs<Stmt>("catch") != nullptr; - if (!HasCatchAnsestor) { + if (!HasCatchAncestor) { diag(MatchedThrow->getThrowLoc(), - "empty 'throw' outside a catch block without an exception can trigger " - "'std::terminate'"); + "empty 'throw' outside a catch block with no operand triggers 'std::terminate()'"); } } diff --git a/clang-tools-extra/clang-tidy/bugprone/ExceptionRethrowCheck.h b/clang-tools-extra/clang-tidy/bugprone/ExceptionRethrowCheck.h index bbb30f779cf25..efe9aad80d13d 100644 --- a/clang-tools-extra/clang-tidy/bugprone/ExceptionRethrowCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/ExceptionRethrowCheck.h @@ -6,8 +6,8 @@ // //===----------------------------------------------------------------------===// -#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_EXCEPTIONRETHROWCHECK_H -#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_EXCEPTIONRETHROWCHECK_H +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_EXCEPTION_RETHROW_CHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_EXCEPTION_RETHROW_CHECK_H #include "../ClangTidyCheck.h" diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-rethrow.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-rethrow.rst index 68be515297bc5..677537094e5f6 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-rethrow.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-rethrow.rst @@ -65,4 +65,3 @@ Above program will be terminated with: terminate called without an active exception Aborted (core dumped) - diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-rethrow.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-rethrow.cpp index 5e7ba03652393..a5732d9b2f44b 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-rethrow.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-rethrow.cpp @@ -2,6 +2,13 @@ struct exception {}; +namespace std { + template <class T> + T&& move(T &x) { + return static_cast<T&&>(x); + } +} + void correct() { try { throw exception(); @@ -30,6 +37,33 @@ void not_correct() { } void not_correct2() { + try { + throw exception(); + } catch(const exception &e) { + throw (e); +// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: throwing a copy of the caught 'exception' exception, remove the argument to throw the original exception object [bugprone-exception-rethrow] + } +} + +void not_correct3() { + try { + throw exception(); + } catch(const exception &e) { + throw exception(e); +// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: throwing a copy of the caught 'exception' exception, remove the argument to throw the original exception object [bugprone-exception-rethrow] + } +} + +void not_correct4() { + try { + throw exception(); + } catch(exception &e) { + throw std::move(e); +// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: throwing a copy of the caught 'exception' exception, remove the argument to throw the original exception object [bugprone-exception-rethrow] + } +} + +void not_correct5() { try { throw 5; } catch(const int &e) { @@ -40,13 +74,13 @@ void not_correct2() { void rethrow_not_correct() { throw; -// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: empty 'throw' outside a catch block without an exception can trigger 'std::terminate' [bugprone-exception-rethrow] +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: empty 'throw' outside a catch block with no operand triggers 'std::terminate()' [bugprone-exception-rethrow] } void rethrow_not_correct2() { try { throw; -// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: empty 'throw' outside a catch block without an exception can trigger 'std::terminate' [bugprone-exception-rethrow] +// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: empty 'throw' outside a catch block with no operand triggers 'std::terminate()' [bugprone-exception-rethrow] } catch(...) { } } >From 17a7099228aa9d59a991ebbc59f3718419c4540b Mon Sep 17 00:00:00 2001 From: Piotr Zegar <piotr.ze...@nokia.com> Date: Sun, 28 Apr 2024 16:46:08 +0000 Subject: [PATCH 4/6] Add support for copy constructors, std::move, and so on --- .../bugprone/ExceptionRethrowCheck.cpp | 47 ++++++++++++------- .../checkers/bugprone/exception-rethrow.cpp | 9 ++++ 2 files changed, 39 insertions(+), 17 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/ExceptionRethrowCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ExceptionRethrowCheck.cpp index 3e327606ee562..55f94306f395e 100644 --- a/clang-tools-extra/clang-tidy/bugprone/ExceptionRethrowCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/ExceptionRethrowCheck.cpp @@ -17,26 +17,39 @@ namespace clang::tidy::bugprone { void ExceptionRethrowCheck::registerMatchers(MatchFinder *Finder) { auto RefToExceptionVariable = declRefExpr(to(varDecl(isExceptionVariable()))); - auto StdMoveCall = callExpr(argumentCountIs(1), callee(functionDecl(hasName("::std::move"))), hasArgument(0, RefToExceptionVariable)); - auto CopyOrMoveConstruction = cxxConstructExpr(argumentCountIs(1), hasDeclaration(cxxConstructorDecl(anyOf(isCopyConstructor(), isMoveConstructor()))), hasArgument(0, anyOf(RefToExceptionVariable, StdMoveCall))); - auto FunctionCast = cxxFunctionalCastExpr( hasSourceExpression(anyOf(RefToExceptionVariable, StdMoveCall))); + auto StdMoveCall = + callExpr(argumentCountIs(1), callee(functionDecl(hasName("::std::move"))), + hasArgument(0, RefToExceptionVariable)); + auto CopyOrMoveConstruction = cxxConstructExpr( + argumentCountIs(1), + traverse(TK_AsIs, hasDeclaration(cxxConstructorDecl( + anyOf(isCopyConstructor(), isMoveConstructor())))), + hasArgument(0, anyOf(RefToExceptionVariable, StdMoveCall))); + auto HasEmptyThrowExprDescendant = + hasDescendant(cxxThrowExpr(equalsBoundNode("empty-throw"))); + + Finder->addMatcher( + cxxThrowExpr( + unless(isExpansionInSystemHeader()), unless(has(expr())), + expr().bind("empty-throw"), + anyOf(unless(hasAncestor(cxxCatchStmt())), + hasAncestor(cxxCatchStmt(anyOf( + hasDescendant(functionDecl(HasEmptyThrowExprDescendant)), + hasDescendant(lambdaExpr(HasEmptyThrowExprDescendant))))))), + this); Finder->addMatcher( cxxThrowExpr(unless(isExpansionInSystemHeader()), - anyOf(unless(has(expr())), - has(RefToExceptionVariable), - has(StdMoveCall), - has(CopyOrMoveConstruction), - has(FunctionCast)), - optionally(hasAncestor(cxxCatchStmt().bind("catch")))) + has(expr(anyOf(RefToExceptionVariable, StdMoveCall, + CopyOrMoveConstruction)))) .bind("throw"), this); } void ExceptionRethrowCheck::check(const MatchFinder::MatchResult &Result) { - const auto *MatchedThrow = Result.Nodes.getNodeAs<CXXThrowExpr>("throw"); - - if (const Expr *ThrownObject = MatchedThrow->getSubExpr()) { + if (const auto *MatchedThrow = + Result.Nodes.getNodeAs<CXXThrowExpr>("throw")) { + const Expr *ThrownObject = MatchedThrow->getSubExpr(); diag(MatchedThrow->getThrowLoc(), "throwing a copy of the caught %0 exception, remove the argument to " "throw the original exception object") @@ -44,11 +57,11 @@ void ExceptionRethrowCheck::check(const MatchFinder::MatchResult &Result) { return; } - const bool HasCatchAncestor = - Result.Nodes.getNodeAs<Stmt>("catch") != nullptr; - if (!HasCatchAncestor) { - diag(MatchedThrow->getThrowLoc(), - "empty 'throw' outside a catch block with no operand triggers 'std::terminate()'"); + if (const auto *MatchedEmptyThrow = + Result.Nodes.getNodeAs<CXXThrowExpr>("empty-throw")) { + diag(MatchedEmptyThrow->getThrowLoc(), + "empty 'throw' outside a catch block with no operand triggers " + "'std::terminate()'"); } } diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-rethrow.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-rethrow.cpp index a5732d9b2f44b..2914d2e7452b8 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-rethrow.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-rethrow.cpp @@ -92,3 +92,12 @@ void rethrow_correct() { throw; } } + +void rethrow_in_lambda() { + try { + throw 5; + } catch(...) { + auto lambda = [] { throw; }; +// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: empty 'throw' outside a catch block with no operand triggers 'std::terminate()' [bugprone-exception-rethrow] + } +} >From 5dd82533b3b013e2731575ee9803bbff4c6d5c08 Mon Sep 17 00:00:00 2001 From: Piotr Zegar <m...@piotrzegar.pl> Date: Tue, 21 May 2024 22:18:43 +0200 Subject: [PATCH 5/6] Update clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-rethrow.rst Co-authored-by: isuckatcs <65320245+isucka...@users.noreply.github.com> --- .../checks/bugprone/exception-rethrow.rst | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-rethrow.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-rethrow.rst index 677537094e5f6..71719c3e6dde5 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-rethrow.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-rethrow.rst @@ -21,9 +21,21 @@ to the invocation of the copy constructor. try { // Code that may throw an exception } catch (const std::exception& e) { - throw e; // Bad + throw e; // Bad, 'e' is copied } +.. code-block:: c++ + class derived_exception : public std::exception { ... }; + + void throwDerived() { throw derived_exception{}; } + + try { + throwDerived(); + } catch (const std::exception& e) { + throw e; // Bad, exception slicing occurs as 'derived_exception' is rethrown as 'exception' (maybe with better wording ?) + } + + To prevent these issues, it is advisable to utilize ``throw;`` statements to rethrow the original exception object for currently handled exceptions. >From ffd2e386d89373f62e9d096ba0eb240af7f98dd9 Mon Sep 17 00:00:00 2001 From: Piotr Zegar <m...@piotrzegar.pl> Date: Tue, 21 May 2024 22:18:52 +0200 Subject: [PATCH 6/6] Update clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-rethrow.rst Co-authored-by: isuckatcs <65320245+isucka...@users.noreply.github.com> --- .../docs/clang-tidy/checks/bugprone/exception-rethrow.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-rethrow.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-rethrow.rst index 71719c3e6dde5..ec38e947dcfba 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-rethrow.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-rethrow.rst @@ -43,7 +43,7 @@ rethrow the original exception object for currently handled exceptions. try { // Code that may throw an exception - } catch (const std::exception& e) { + } catch (const std::exception&) { throw; // Good } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits