PiotrZSL updated this revision to Diff 513691. PiotrZSL added a comment. Ping + Rebase
Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145865/new/ https://reviews.llvm.org/D145865 Files: clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-escape.rst clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-throw.cpp clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp =================================================================== --- clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp +++ clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp @@ -1,10 +1,9 @@ -// RUN: %check_clang_tidy -std=c++11,c++14 %s bugprone-exception-escape %t -- \ +// RUN: %check_clang_tidy -std=c++11-or-later %s bugprone-exception-escape %t -- \ // RUN: -config="{CheckOptions: [ \ // RUN: {key: bugprone-exception-escape.IgnoredExceptions, value: 'ignored1,ignored2'}, \ // RUN: {key: bugprone-exception-escape.FunctionsThatShouldNotThrow, value: 'enabled1,enabled2,enabled3'} \ // RUN: ]}" \ // RUN: -- -fexceptions -// FIXME: Fix the checker to work in C++17 or later mode. struct throwing_destructor { ~throwing_destructor() { @@ -32,11 +31,6 @@ throw 1; } -void throwing_throw_nothing() throw() { - // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throwing_throw_nothing' which should not throw exceptions - throw 1; -} - void throw_and_catch() noexcept { // CHECK-MESSAGES-NOT: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_and_catch' which should not throw exceptions try { @@ -157,7 +151,7 @@ } // FIXME: In this case 'a' is convertible to the handler and should be caught -// but in reality it's thrown. Note that clang doesn't report a warning for +// but in reality it's thrown. Note that clang doesn't report a warning for // this either. void throw_catch_multi_ptr_5() noexcept { try { @@ -254,7 +248,7 @@ void throw_derived_catch_base_ptr_c() noexcept { try { derived d; - throw &d; + throw &d; } catch(const base *) { } } @@ -264,7 +258,7 @@ try { derived d; const derived *p = &d; - throw p; + throw p; } catch(base *) { } } @@ -287,7 +281,7 @@ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derived_catch_base_private' which should not throw exceptions try { B b; - throw b; + throw b; } catch(A) { } } @@ -296,7 +290,7 @@ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derived_catch_base_private_ptr' which should not throw exceptions try { B b; - throw &b; + throw &b; } catch(A *) { } } @@ -305,7 +299,7 @@ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derived_catch_base_protected' which should not throw exceptions try { C c; - throw c; + throw c; } catch(A) { } } @@ -314,7 +308,7 @@ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derived_catch_base_protected_ptr' which should not throw exceptions try { C c; - throw &c; + throw &c; } catch(A *) { } } @@ -323,7 +317,7 @@ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derived_catch_base_ambiguous' which should not throw exceptions try { E e; - throw e; + throw e; } catch(A) { } } @@ -332,7 +326,7 @@ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derived_catch_base_ambiguous_ptr' which should not throw exceptions try { E e; - throw e; + throw e; } catch(A) { } } @@ -401,7 +395,7 @@ } namespace b { - inline int foo() { return 0; }; + int foo() { return 0; }; void throw_inline_catch_regular() noexcept { try { @@ -412,12 +406,12 @@ } namespace c { - inline int foo() noexcept { return 0; }; + int foo() noexcept { return 0; }; void throw_noexcept_catch_regular() noexcept { try { throw &foo; - } catch(int (*)()) { + } catch(int (*)() noexcept) { } } } @@ -557,7 +551,9 @@ throw 1; } -void explicit_int_thrower() throw(int); +void explicit_thrower() noexcept(false) { + throw 1; +} void indirect_implicit() noexcept { // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'indirect_implicit' which should not throw exceptions @@ -566,7 +562,7 @@ void indirect_explicit() noexcept { // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'indirect_explicit' which should not throw exceptions - explicit_int_thrower(); + explicit_thrower(); } void indirect_catch() noexcept { @@ -655,7 +651,6 @@ } int indirectly_recursive(int n) noexcept; - // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: an exception may be thrown in function 'indirectly_recursive' which should not throw exceptions int recursion_helper(int n) { indirectly_recursive(n); @@ -677,15 +672,6 @@ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: an exception may be thrown in function 'sub_throws' which should not throw exceptions }; -struct super_throws_again { - super_throws_again() throw(int); -}; - -struct sub_throws_again : super_throws_again { - sub_throws_again() noexcept : super_throws_again() {} - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: an exception may be thrown in function 'sub_throws_again' which should not throw exceptions -}; - struct init_member_throws { super_throws s; @@ -716,3 +702,75 @@ throw 1; return 0; } + +namespace PR55143 { namespace PR40583 { + +struct test_explicit_throw { + test_explicit_throw() noexcept(false) { throw 42; } + test_explicit_throw(const test_explicit_throw&) noexcept(false) { throw 42; } + test_explicit_throw(test_explicit_throw&&) noexcept(false) { throw 42; } + test_explicit_throw& operator=(const test_explicit_throw&) noexcept(false) { throw 42; } + test_explicit_throw& operator=(test_explicit_throw&&) noexcept(false) { throw 42; } + ~test_explicit_throw() noexcept(false) { throw 42; } + + void swap(test_explicit_throw&) noexcept(false) { throw 42; } +}; + + +struct test_implicit_throw { + test_implicit_throw() { throw 42; } + test_implicit_throw(const test_implicit_throw&) { throw 42; } + test_implicit_throw(test_implicit_throw&&) { throw 42; } + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: an exception may be thrown in function 'test_implicit_throw' which should not throw exceptions + test_implicit_throw& operator=(const test_implicit_throw&) { throw 42; } + test_implicit_throw& operator=(test_implicit_throw&&) { throw 42; } + // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: an exception may be thrown in function 'operator=' which should not throw exceptions + ~test_implicit_throw() { throw 42; } + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: an exception may be thrown in function '~test_implicit_throw' which should not throw exceptions + + void swap(test_explicit_throw&) { throw 42; } + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: an exception may be thrown in function 'swap' which should not throw exceptions +}; + +}} + +namespace PR43667 { namespace PR51596 { // PR56411, PR54956, PR54668, PR49151 +// The following functions all incorrectly throw exceptions, *but* calling them +// should not yield a warning because they are marked as noexcept (or similar). + +void test_basic_noexcept() noexcept { throw 42; } +// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'test_basic_noexcept' which should not throw exceptions +void test_noexcept_true() noexcept(true) { throw 42; } +// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'test_noexcept_true' which should not throw exceptions +template <typename T> +void test_dependent_noexcept() noexcept(T::should_throw) { throw 42; } +// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'test_dependent_noexcept<PR43667::PR51596::ShouldThrow<true>>' which should not throw exceptions + +template <bool B> +struct ShouldThrow { + static const bool should_throw = B; +}; + +void only_calls_non_throwing() noexcept { + test_basic_noexcept(); + test_noexcept_true(); + test_dependent_noexcept<ShouldThrow<true>>(); +} + +void calls_non_and_throwing() noexcept { +// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'calls_non_and_throwing' which should not throw exceptions + test_basic_noexcept(); + test_noexcept_true(); + test_dependent_noexcept<ShouldThrow<true>>(); + test_dependent_noexcept<ShouldThrow<false>>(); +} + +}} + +namespace PR46282 { + void foo(); + + void bar() noexcept { + foo(); + } +} Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-throw.cpp =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-throw.cpp @@ -0,0 +1,84 @@ +// RUN: %check_clang_tidy -std=c++11,c++14 %s bugprone-exception-escape %t -- -- -fexceptions + +void throwing_throw_nothing() throw() { +// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throwing_throw_nothing' which should not throw exceptions + throw 1; +} + +void explicit_int_thrower() throw(int); + +void implicit_int_thrower() { + throw 5; +} + +void indirect_implicit() throw() { +// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'indirect_implicit' which should not throw exceptions + implicit_int_thrower(); +} + +void indirect_explicit() throw() { +// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'indirect_explicit' which should not throw exceptions + explicit_int_thrower(); +} + +struct super_throws { + super_throws() throw(int) { throw 42; } +}; + +struct sub_throws : super_throws { + sub_throws() throw() : super_throws() {} +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: an exception may be thrown in function 'sub_throws' which should not throw exceptions +}; + +namespace PR55143 { namespace PR40583 { + +struct test_explicit_throw { + test_explicit_throw() throw(int) { throw 42; } + test_explicit_throw(const test_explicit_throw&) throw(int) { throw 42; } + test_explicit_throw(test_explicit_throw&&) throw(int) { throw 42; } + test_explicit_throw& operator=(const test_explicit_throw&) throw(int) { throw 42; } + test_explicit_throw& operator=(test_explicit_throw&&) throw(int) { throw 42; } + ~test_explicit_throw() throw(int) { throw 42; } +}; + +struct test_implicit_throw { + test_implicit_throw() { throw 42; } + test_implicit_throw(const test_implicit_throw&) { throw 42; } + test_implicit_throw(test_implicit_throw&&) { throw 42; } + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: an exception may be thrown in function 'test_implicit_throw' which should not throw exceptions [bugprone-exception-escape] + test_implicit_throw& operator=(const test_implicit_throw&) { throw 42; } + test_implicit_throw& operator=(test_implicit_throw&&) { throw 42; } + // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: an exception may be thrown in function 'operator=' which should not throw exceptions [bugprone-exception-escape] + ~test_implicit_throw() { throw 42; } + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: an exception may be thrown in function '~test_implicit_throw' which should not throw exceptions [bugprone-exception-escape] +}; + +}} + +namespace PR43667 { namespace PR51596 { // PR56411, PR54956, PR54668, PR49151 +// The following functions all incorrectly throw exceptions, *but* calling them +// should not yield a warning because they are marked as noexcept (or similar). + +void test_basic_no_throw() throw() { throw 42; } +// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'test_basic_no_throw' which should not throw exceptions +void test_basic_throw() throw(int) { throw 42; } + +void only_calls_non_throwing() throw() { + test_basic_no_throw(); +} + +void calls_non_and_throwing() throw() { +// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'calls_non_and_throwing' which should not throw exceptions + test_basic_no_throw(); + test_basic_throw(); +} + +}} + +namespace PR46282 { + void foo(); + + void bar() throw() { + foo(); + } +} Index: clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-escape.rst =================================================================== --- clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-escape.rst +++ clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-escape.rst @@ -22,6 +22,12 @@ operations are also used to create move operations. A throwing ``main()`` function also results in unexpected termination. +Functions declared explicitly with ``noexcept(false)`` or ``throw(exception)`` +will be excluded from the analysis, as even though it is not recommended for +functions like ``swap``, ``main``, move constructors and assignment operators, +and destructors, it is a clear indication of the developer's intention and +should be respected.. + WARNING! This check may be expensive on large source files. Options Index: clang-tools-extra/docs/ReleaseNotes.rst =================================================================== --- clang-tools-extra/docs/ReleaseNotes.rst +++ clang-tools-extra/docs/ReleaseNotes.rst @@ -179,6 +179,12 @@ <clang-tidy/checks/bugprone/dynamic-static-initializers>` check. Global options of the same name should be used instead. +- Improved :doc:`bugprone-exception-escape + <clang-tidy/checks/bugprone/exception-escape>` by excluding explicitly + declared throwing functions from analysis, unless specified in the check option, + and respecting the ``noexcept`` specifier of called functions to handle + exceptions better. + - Improved :doc:`bugprone-fold-init-type <clang-tidy/checks/bugprone/fold-init-type>` to handle iterators that do not define `value_type` type aliases. Index: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp =================================================================== --- clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp +++ clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp @@ -47,6 +47,27 @@ // FIXME: This could be ported to clang later. namespace { +bool cannotThrow(const FunctionDecl *Func) { + const auto *FunProto = Func->getType()->getAs<FunctionProtoType>(); + if (!FunProto) + return false; + + switch (FunProto->canThrow()) { + case CT_Cannot: + return true; + case CT_Dependent: { + const Expr *NoexceptExpr = FunProto->getNoexceptExpr(); + bool Result; + return NoexceptExpr && !NoexceptExpr->isValueDependent() && + NoexceptExpr->EvaluateAsBooleanCondition( + Result, Func->getASTContext(), true) && + Result; + } + default: + return false; + }; +} + bool isUnambiguousPublicBaseClass(const Type *DerivedType, const Type *BaseType) { const auto *DerivedClass = @@ -157,6 +178,9 @@ return false; if (To->isFunctionPointerType()) { + // FIXME: Two function pointers can differ in 'noexcept', but they still + // should be considered to be same, now this triggers false-positive because + // Type* don't match if (From->isFunctionPointerType()) return To->getPointeeType() == From->getPointeeType(); @@ -285,6 +309,8 @@ if (From->isIncompleteArrayType() && !To->isIncompleteArrayType()) return false; + if (From->isMemberPointerType() || To->isMemberPointerType()) + return false; } else { return false; } @@ -421,7 +447,8 @@ ExceptionAnalyzer::ExceptionInfo ExceptionAnalyzer::throwsException( const FunctionDecl *Func, llvm::SmallSet<const FunctionDecl *, 32> &CallStack) { - if (CallStack.count(Func)) + if (!Func || CallStack.count(Func) || + (!CallStack.empty() && cannotThrow(Func))) return ExceptionInfo::createNonThrowing(); if (const Stmt *Body = Func->getBody()) { @@ -461,14 +488,11 @@ if (const auto *Throw = dyn_cast<CXXThrowExpr>(St)) { if (const auto *ThrownExpr = Throw->getSubExpr()) { - const auto *ThrownType = - ThrownExpr->getType()->getUnqualifiedDesugaredType(); - if (ThrownType->isReferenceType()) - ThrownType = ThrownType->castAs<ReferenceType>() - ->getPointeeType() - ->getUnqualifiedDesugaredType(); - Results.registerException( - ThrownExpr->getType()->getUnqualifiedDesugaredType()); + const Type *ThrownType = ThrownExpr->getType() + .getCanonicalType() + .getNonReferenceType() + ->getUnqualifiedDesugaredType(); + Results.registerException(ThrownType); } else // A rethrow of a caught exception happens which makes it possible // to throw all exception that are caught in the 'catch' clause of @@ -487,14 +511,10 @@ Results.merge(Rethrown); Uncaught.clear(); } else { - const auto *CaughtType = - Catch->getCaughtType()->getUnqualifiedDesugaredType(); - if (CaughtType->isReferenceType()) { - CaughtType = CaughtType->castAs<ReferenceType>() - ->getPointeeType() - ->getUnqualifiedDesugaredType(); - } - + const Type *CaughtType = Catch->getCaughtType() + .getCanonicalType() + .getNonReferenceType() + ->getUnqualifiedDesugaredType(); // If the caught exception will catch multiple previously potential // thrown types (because it's sensitive to inheritance) the throwing // situation changes. First of all filter the exception types and Index: clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp =================================================================== --- clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp +++ clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp @@ -15,15 +15,26 @@ using namespace clang::ast_matchers; -namespace clang { +namespace clang::tidy::bugprone { namespace { AST_MATCHER_P(FunctionDecl, isEnabled, llvm::StringSet<>, FunctionsThatShouldNotThrow) { return FunctionsThatShouldNotThrow.count(Node.getNameAsString()) > 0; } + +AST_MATCHER(FunctionDecl, isExplicitThrow) { + switch (Node.getExceptionSpecType()) { + case EST_Dynamic: + case EST_MSAny: + case EST_NoexceptFalse: + return Node.getExceptionSpecSourceRange().isValid(); + default: + return false; + } +} + } // namespace -namespace tidy::bugprone { ExceptionEscapeCheck::ExceptionEscapeCheck(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), RawFunctionsThatShouldNotThrow(Options.get( @@ -52,11 +63,14 @@ void ExceptionEscapeCheck::registerMatchers(MatchFinder *Finder) { Finder->addMatcher( - functionDecl(anyOf(isNoThrow(), cxxDestructorDecl(), - cxxConstructorDecl(isMoveConstructor()), - cxxMethodDecl(isMoveAssignmentOperator()), - hasName("main"), hasName("swap"), - isEnabled(FunctionsThatShouldNotThrow))) + functionDecl( + isDefinition(), + anyOf(isNoThrow(), + allOf(anyOf(isMain(), hasName("swap"), cxxDestructorDecl(), + cxxConstructorDecl(isMoveConstructor()), + cxxMethodDecl(isMoveAssignmentOperator())), + unless(isExplicitThrow())), + isEnabled(FunctionsThatShouldNotThrow))) .bind("thrower"), this); } @@ -64,17 +78,25 @@ void ExceptionEscapeCheck::check(const MatchFinder::MatchResult &Result) { const auto *MatchedDecl = Result.Nodes.getNodeAs<FunctionDecl>("thrower"); - if (!MatchedDecl) + const utils::ExceptionAnalyzer::ExceptionInfo AnalyzeResult = + Tracer.analyze(MatchedDecl); + if (!(AnalyzeResult.getBehaviour() == + utils::ExceptionAnalyzer::State::Throwing)) return; - if (Tracer.analyze(MatchedDecl).getBehaviour() == - utils::ExceptionAnalyzer::State::Throwing) - // FIXME: We should provide more information about the exact location where - // the exception is thrown, maybe the full path the exception escapes - diag(MatchedDecl->getLocation(), "an exception may be thrown in function " - "%0 which should not throw exceptions") - << MatchedDecl; + // FIXME: We should provide more information about the exact location where + // the exception is thrown, maybe the full path the exception escapes + diag(MatchedDecl->getLocation(), "an exception may be thrown in function " + "%0 which should not throw exceptions") + << MatchedDecl; + + for (const Type *ExceptionType : AnalyzeResult.getExceptionTypes()) + diag(MatchedDecl->getLocation(), "may throw %0", DiagnosticIDs::Note) + << QualType(ExceptionType, 0U); + + if (AnalyzeResult.containsUnknownElements()) + diag(MatchedDecl->getLocation(), "may throw unknown exceptions", + DiagnosticIDs::Note); } -} // namespace tidy::bugprone -} // namespace clang +} // namespace clang::tidy::bugprone
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits