fwolff created this revision.
fwolff added reviewers: aaron.ballman, JonasToth, lebedev.ri.
fwolff added a project: clang-tools-extra.
Herald added subscribers: carlosgalvezp, xazax.hun.
fwolff requested review of this revision.
Herald added a subscriber: cfe-commits.
Fixes PR#52254 <https://bugs.llvm.org/show_bug.cgi?id=52254>. The
`ExceptionAnalyzer` currently recursively checks called functions, but this
does not make sense if the called function is marked as `noexcept`. If it does
throw, there should be a warning //for that function,// but not for every
caller. In particular, this can lead to false positives if the called
`noexcept` function calls other, potentially throwing, functions, in a way that
ensures they will never actually throw.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D115118
Files:
clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.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
@@ -288,6 +288,29 @@
return recursion_helper(n);
}
+// The following functions all incorrectly throw exceptions, *but* calling them
+// should not yield a warning because they are marked as noexcept (or similar).
+void est_dynamic_none() throw() { throw 42; }
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in
function 'est_dynamic_none' which should not throw exceptions
+void est_basic_noexcept() noexcept { throw 42; }
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in
function 'est_basic_noexcept' which should not throw exceptions
+void est_noexcept_true() noexcept(true) { throw 42; }
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in
function 'est_noexcept_true' which should not throw exceptions
+template <typename T>
+void est_dependent_noexcept() noexcept(T::should_throw) { throw 42; }
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in
function 'est_dependent_noexcept<ShouldThrow<true>>' which should not throw
exceptions
+template <bool B>
+struct ShouldThrow {
+ static const bool should_throw = B;
+};
+
+void only_calls_non_throwing() noexcept {
+ est_dynamic_none();
+ est_basic_noexcept();
+ est_noexcept_true();
+ est_dependent_noexcept<ShouldThrow<true>>();
+}
+
int main() {
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: an exception may be thrown in
function 'main' which should not throw exceptions
throw 1;
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
@@ -192,8 +192,27 @@
Results.merge(Uncaught);
} else if (const auto *Call = dyn_cast<CallExpr>(St)) {
if (const FunctionDecl *Func = Call->getDirectCallee()) {
- ExceptionInfo Excs = throwsException(Func, CallStack);
- Results.merge(Excs);
+ bool MightThrow;
+
+ switch (Func->getExceptionSpecType()) {
+ case EST_DynamicNone:
+ case EST_NoThrow:
+ case EST_BasicNoexcept:
+ case EST_DependentNoexcept: // Let's be optimistic here (necessary e.g.
+ // for variant assignment; see PR#52254)
+ case EST_NoexceptTrue:
+ MightThrow = false;
+ break;
+
+ default:
+ MightThrow = true;
+ break;
+ }
+
+ if (MightThrow) {
+ ExceptionInfo Excs = throwsException(Func, CallStack);
+ Results.merge(Excs);
+ }
}
} else {
for (const Stmt *Child : St->children()) {
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
@@ -288,6 +288,29 @@
return recursion_helper(n);
}
+// The following functions all incorrectly throw exceptions, *but* calling them
+// should not yield a warning because they are marked as noexcept (or similar).
+void est_dynamic_none() throw() { throw 42; }
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'est_dynamic_none' which should not throw exceptions
+void est_basic_noexcept() noexcept { throw 42; }
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'est_basic_noexcept' which should not throw exceptions
+void est_noexcept_true() noexcept(true) { throw 42; }
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'est_noexcept_true' which should not throw exceptions
+template <typename T>
+void est_dependent_noexcept() noexcept(T::should_throw) { throw 42; }
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'est_dependent_noexcept<ShouldThrow<true>>' which should not throw exceptions
+template <bool B>
+struct ShouldThrow {
+ static const bool should_throw = B;
+};
+
+void only_calls_non_throwing() noexcept {
+ est_dynamic_none();
+ est_basic_noexcept();
+ est_noexcept_true();
+ est_dependent_noexcept<ShouldThrow<true>>();
+}
+
int main() {
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: an exception may be thrown in function 'main' which should not throw exceptions
throw 1;
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
@@ -192,8 +192,27 @@
Results.merge(Uncaught);
} else if (const auto *Call = dyn_cast<CallExpr>(St)) {
if (const FunctionDecl *Func = Call->getDirectCallee()) {
- ExceptionInfo Excs = throwsException(Func, CallStack);
- Results.merge(Excs);
+ bool MightThrow;
+
+ switch (Func->getExceptionSpecType()) {
+ case EST_DynamicNone:
+ case EST_NoThrow:
+ case EST_BasicNoexcept:
+ case EST_DependentNoexcept: // Let's be optimistic here (necessary e.g.
+ // for variant assignment; see PR#52254)
+ case EST_NoexceptTrue:
+ MightThrow = false;
+ break;
+
+ default:
+ MightThrow = true;
+ break;
+ }
+
+ if (MightThrow) {
+ ExceptionInfo Excs = throwsException(Func, CallStack);
+ Results.merge(Excs);
+ }
}
} else {
for (const Stmt *Child : St->children()) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
