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
