-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[clang-tidy] Improve bugprone-exception-escape: add stacktrace of escaped exception
#134375
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[clang-tidy] Improve bugprone-exception-escape: add stacktrace of escaped exception
#134375
Conversation
|
@llvm/pr-subscribers-clang-tidy Author: Baranov Victor (vbvictor) ChangesThis PR add stacktrace of escaped exception to
Example of new diagnostics: > warning: an exception may be thrown in function 'calls_non_and_throwing' which should not throw exceptions [bugprone-exception-escape] More example can be found it tests. Patch is 67.93 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/134375.diff 8 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
index 7e9551532b72f..0113da6ec1ac1 100644
--- a/clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
@@ -80,13 +80,46 @@ void ExceptionEscapeCheck::check(const MatchFinder::MatchResult &Result) {
if (!MatchedDecl)
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;
+ const utils::ExceptionAnalyzer::ExceptionInfo Info =
+ Tracer.analyze(MatchedDecl);
+
+ if (Info.getBehaviour() != utils::ExceptionAnalyzer::State::Throwing) {
+ return;
+ }
+
+ diag(MatchedDecl->getLocation(), "an exception may be thrown in function "
+ "%0 which should not throw exceptions")
+ << MatchedDecl;
+
+ const utils::ExceptionAnalyzer::ExceptionInfo::ThrowInfo ThrowInfo =
+ Info.getExceptions().begin()->getSecond();
+
+ if (ThrowInfo.Loc.isInvalid()) {
+ return;
+ }
+
+ // FIXME: We should provide exact position of functions calls, not only call
+ // stack of thrown exception.
+ const utils::ExceptionAnalyzer::CallStack &Stack = ThrowInfo.Stack;
+ diag(Stack.front()->getLocation(),
+ "example of unhandled exception throw stack, starting from function %0",
+ DiagnosticIDs::Note)
+ << Stack.front();
+
+ size_t FrameNo = 0;
+ for (const FunctionDecl *CallNode : Stack) {
+ if (FrameNo != Stack.size() - 1) {
+ diag(CallNode->getLocation(), "frame #%0: function %1",
+ DiagnosticIDs::Note)
+ << FrameNo << CallNode;
+ } else {
+ diag(ThrowInfo.Loc,
+ "frame #%0: function %1 throws unhandled exception here",
+ DiagnosticIDs::Note)
+ << FrameNo << CallNode;
+ }
+ ++FrameNo;
+ }
}
} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp b/clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
index e28ee7d9c70f7..42f04b07d88f8 100644
--- a/clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
+++ b/clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
@@ -11,10 +11,10 @@
namespace clang::tidy::utils {
void ExceptionAnalyzer::ExceptionInfo::registerException(
- const Type *ExceptionType) {
+ const Type *ExceptionType, const ThrowInfo &ThrowInfo) {
assert(ExceptionType != nullptr && "Only valid types are accepted");
Behaviour = State::Throwing;
- ThrownExceptions.insert(ExceptionType);
+ ThrownExceptions.insert({ExceptionType, ThrowInfo});
}
void ExceptionAnalyzer::ExceptionInfo::registerExceptions(
@@ -354,10 +354,12 @@ static bool canThrow(const FunctionDecl *Func) {
};
}
-bool ExceptionAnalyzer::ExceptionInfo::filterByCatch(
- const Type *HandlerTy, const ASTContext &Context) {
+ExceptionAnalyzer::ExceptionInfo::Throwables
+ExceptionAnalyzer::ExceptionInfo::filterByCatch(const Type *HandlerTy,
+ const ASTContext &Context) {
llvm::SmallVector<const Type *, 8> TypesToDelete;
- for (const Type *ExceptionTy : ThrownExceptions) {
+ for (const auto &ThrownException : ThrownExceptions) {
+ const Type *ExceptionTy = ThrownException.getFirst();
CanQualType ExceptionCanTy = ExceptionTy->getCanonicalTypeUnqualified();
CanQualType HandlerCanTy = HandlerTy->getCanonicalTypeUnqualified();
@@ -407,11 +409,18 @@ bool ExceptionAnalyzer::ExceptionInfo::filterByCatch(
}
}
- for (const Type *T : TypesToDelete)
- ThrownExceptions.erase(T);
+ Throwables DeletedExceptions;
+
+ for (const Type *TypeToDelete : TypesToDelete) {
+ const auto DeleteIt = ThrownExceptions.find(TypeToDelete);
+ if (DeleteIt != ThrownExceptions.end()) {
+ DeletedExceptions.insert(*DeleteIt);
+ ThrownExceptions.erase(DeleteIt);
+ }
+ }
reevaluateBehaviour();
- return !TypesToDelete.empty();
+ return DeletedExceptions;
}
ExceptionAnalyzer::ExceptionInfo &
@@ -420,7 +429,8 @@ ExceptionAnalyzer::ExceptionInfo::filterIgnoredExceptions(
llvm::SmallVector<const Type *, 8> TypesToDelete;
// Note: Using a 'SmallSet' with 'llvm::remove_if()' is not possible.
// Therefore this slightly hacky implementation is required.
- for (const Type *T : ThrownExceptions) {
+ for (const auto &ThrownException : ThrownExceptions) {
+ const Type *T = ThrownException.getFirst();
if (const auto *TD = T->getAsTagDecl()) {
if (TD->getDeclName().isIdentifier()) {
if ((IgnoreBadAlloc &&
@@ -452,10 +462,10 @@ void ExceptionAnalyzer::ExceptionInfo::reevaluateBehaviour() {
else
Behaviour = State::Throwing;
}
-
-ExceptionAnalyzer::ExceptionInfo ExceptionAnalyzer::throwsException(
- const FunctionDecl *Func, const ExceptionInfo::Throwables &Caught,
- llvm::SmallSet<const FunctionDecl *, 32> &CallStack) {
+ExceptionAnalyzer::ExceptionInfo
+ExceptionAnalyzer::throwsException(const FunctionDecl *Func,
+ const ExceptionInfo::Throwables &Caught,
+ CallStack &CallStack) {
if (!Func || CallStack.contains(Func) ||
(!CallStack.empty() && !canThrow(Func)))
return ExceptionInfo::createNonThrowing();
@@ -473,23 +483,25 @@ ExceptionAnalyzer::ExceptionInfo ExceptionAnalyzer::throwsException(
}
}
- CallStack.erase(Func);
+ CallStack.remove(Func);
return Result;
}
auto Result = ExceptionInfo::createUnknown();
if (const auto *FPT = Func->getType()->getAs<FunctionProtoType>()) {
for (const QualType &Ex : FPT->exceptions())
- Result.registerException(Ex.getTypePtr());
+ // FIXME add something to ThrowInfo
+ Result.registerException(Ex.getTypePtr(), {});
}
return Result;
}
/// Analyzes a single statement on it's throwing behaviour. This is in principle
/// possible except some 'Unknown' functions are called.
-ExceptionAnalyzer::ExceptionInfo ExceptionAnalyzer::throwsException(
- const Stmt *St, const ExceptionInfo::Throwables &Caught,
- llvm::SmallSet<const FunctionDecl *, 32> &CallStack) {
+ExceptionAnalyzer::ExceptionInfo
+ExceptionAnalyzer::throwsException(const Stmt *St,
+ const ExceptionInfo::Throwables &Caught,
+ CallStack &CallStack) {
auto Results = ExceptionInfo::createNonThrowing();
if (!St)
return Results;
@@ -503,7 +515,8 @@ ExceptionAnalyzer::ExceptionInfo ExceptionAnalyzer::throwsException(
->getPointeeType()
->getUnqualifiedDesugaredType();
Results.registerException(
- ThrownExpr->getType()->getUnqualifiedDesugaredType());
+ ThrownExpr->getType()->getUnqualifiedDesugaredType(),
+ {Throw->getBeginLoc(), CallStack});
} else
// A rethrow of a caught exception happens which makes it possible
// to throw all exception that are caught in the 'catch' clause of
@@ -518,7 +531,7 @@ ExceptionAnalyzer::ExceptionInfo ExceptionAnalyzer::throwsException(
// Everything is caught through 'catch(...)'.
if (!Catch->getExceptionDecl()) {
ExceptionInfo Rethrown = throwsException(
- Catch->getHandlerBlock(), Uncaught.getExceptionTypes(), CallStack);
+ Catch->getHandlerBlock(), Uncaught.getExceptions(), CallStack);
Results.merge(Rethrown);
Uncaught.clear();
} else {
@@ -534,12 +547,12 @@ ExceptionAnalyzer::ExceptionInfo ExceptionAnalyzer::throwsException(
// thrown types (because it's sensitive to inheritance) the throwing
// situation changes. First of all filter the exception types and
// analyze if the baseclass-exception is rethrown.
- if (Uncaught.filterByCatch(
- CaughtType, Catch->getExceptionDecl()->getASTContext())) {
- ExceptionInfo::Throwables CaughtExceptions;
- CaughtExceptions.insert(CaughtType);
- ExceptionInfo Rethrown = throwsException(Catch->getHandlerBlock(),
- CaughtExceptions, CallStack);
+ const ExceptionInfo::Throwables FilteredExceptions =
+ Uncaught.filterByCatch(CaughtType,
+ Catch->getExceptionDecl()->getASTContext());
+ if (!FilteredExceptions.empty()) {
+ ExceptionInfo Rethrown = throwsException(
+ Catch->getHandlerBlock(), FilteredExceptions, CallStack);
Results.merge(Rethrown);
}
}
@@ -567,9 +580,10 @@ ExceptionAnalyzer::ExceptionInfo ExceptionAnalyzer::throwsException(
}
ExceptionInfo Excs = throwsException(Coro->getBody(), Caught, CallStack);
Results.merge(throwsException(Coro->getExceptionHandler(),
- Excs.getExceptionTypes(), CallStack));
- for (const Type *Throwable : Excs.getExceptionTypes()) {
- if (const auto ThrowableRec = Throwable->getAsCXXRecordDecl()) {
+ Excs.getExceptions(), CallStack));
+ for (const auto &Exception : Excs.getExceptions()) {
+ const Type *ExcType = Exception.getFirst();
+ if (const CXXRecordDecl *ThrowableRec = ExcType->getAsCXXRecordDecl()) {
ExceptionInfo DestructorExcs =
throwsException(ThrowableRec->getDestructor(), Caught, CallStack);
Results.merge(DestructorExcs);
@@ -591,7 +605,7 @@ ExceptionAnalyzer::analyzeImpl(const FunctionDecl *Func) {
// Check if the function has already been analyzed and reuse that result.
const auto CacheEntry = FunctionCache.find(Func);
if (CacheEntry == FunctionCache.end()) {
- llvm::SmallSet<const FunctionDecl *, 32> CallStack;
+ CallStack CallStack;
ExceptionList =
throwsException(Func, ExceptionInfo::Throwables(), CallStack);
@@ -608,7 +622,7 @@ ExceptionAnalyzer::analyzeImpl(const FunctionDecl *Func) {
ExceptionAnalyzer::ExceptionInfo
ExceptionAnalyzer::analyzeImpl(const Stmt *Stmt) {
- llvm::SmallSet<const FunctionDecl *, 32> CallStack;
+ CallStack CallStack;
return throwsException(Stmt, ExceptionInfo::Throwables(), CallStack);
}
diff --git a/clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h b/clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h
index 6c2d693d64b50..0fea44fc8a622 100644
--- a/clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h
+++ b/clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h
@@ -12,6 +12,7 @@
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "llvm/ADT/SmallSet.h"
+#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringSet.h"
namespace clang::tidy::utils {
@@ -28,6 +29,12 @@ class ExceptionAnalyzer {
///< definition.
};
+ /// We use a SetVector to preserve the order of the functions in the call
+ /// stack as well as have fast lookup.
+ using CallStack = llvm::SetVector<const FunctionDecl *,
+ llvm::SmallVector<const FunctionDecl *, 32>,
+ llvm::DenseSet<const FunctionDecl *>, 32>;
+
/// Bundle the gathered information about an entity like a function regarding
/// it's exception behaviour. The 'NonThrowing'-state can be considered as the
/// neutral element in terms of information propagation.
@@ -37,7 +44,15 @@ class ExceptionAnalyzer {
/// exception at runtime.
class ExceptionInfo {
public:
- using Throwables = llvm::SmallSet<const Type *, 2>;
+ /// Holds information about where an exception is thrown.
+ /// First element in the call stack is analyzed function.
+ struct ThrowInfo {
+ SourceLocation Loc;
+ CallStack Stack;
+ };
+
+ using Throwables = llvm::SmallDenseMap<const Type *, ThrowInfo, 2>;
+
static ExceptionInfo createUnknown() { return {State::Unknown}; }
static ExceptionInfo createNonThrowing() { return {State::Throwing}; }
@@ -56,7 +71,8 @@ class ExceptionAnalyzer {
/// Register a single exception type as recognized potential exception to be
/// thrown.
- void registerException(const Type *ExceptionType);
+ void registerException(const Type *ExceptionType,
+ const ThrowInfo &ThrowInfo);
/// Registers a `SmallVector` of exception types as recognized potential
/// exceptions to be thrown.
@@ -73,8 +89,8 @@ class ExceptionAnalyzer {
/// This method is useful in case 'catch' clauses are analyzed as it is
/// possible to catch multiple exception types by one 'catch' if they
/// are a subclass of the 'catch'ed exception type.
- /// Returns 'true' if some exceptions were filtered, otherwise 'false'.
- bool filterByCatch(const Type *HandlerTy, const ASTContext &Context);
+ /// Returns filtered exceptions.
+ Throwables filterByCatch(const Type *HandlerTy, const ASTContext &Context);
/// Filter the set of thrown exception type against a set of ignored
/// types that shall not be considered in the exception analysis.
@@ -87,9 +103,9 @@ class ExceptionAnalyzer {
/// neutral.
void clear();
- /// References the set of known exception types that can escape from the
+ /// References the set of known exceptions that can escape from the
/// corresponding entity.
- const Throwables &getExceptionTypes() const { return ThrownExceptions; }
+ const Throwables &getExceptions() const { return ThrownExceptions; }
/// Signal if the there is any 'Unknown' element within the scope of
/// the related entity. This might be relevant if the entity is 'Throwing'
@@ -126,14 +142,12 @@ class ExceptionAnalyzer {
ExceptionInfo analyze(const Stmt *Stmt);
private:
- ExceptionInfo
- throwsException(const FunctionDecl *Func,
- const ExceptionInfo::Throwables &Caught,
- llvm::SmallSet<const FunctionDecl *, 32> &CallStack);
- ExceptionInfo
- throwsException(const Stmt *St, const ExceptionInfo::Throwables &Caught,
- llvm::SmallSet<const FunctionDecl *, 32> &CallStack);
-
+ ExceptionInfo throwsException(const FunctionDecl *Func,
+ const ExceptionInfo::Throwables &Caught,
+ CallStack &CallStack);
+ ExceptionInfo throwsException(const Stmt *St,
+ const ExceptionInfo::Throwables &Caught,
+ CallStack &CallStack);
ExceptionInfo analyzeImpl(const FunctionDecl *Func);
ExceptionInfo analyzeImpl(const Stmt *Stmt);
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 6cb8d572d3a78..36d06fce7232e 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -121,6 +121,10 @@ New check aliases
Changes in existing checks
^^^^^^^^^^^^^^^^^^^^^^^^^^
+- Improved :doc:`bugprone-exception-escape
+ <clang-tidy/checks/bugprone/exception-escape>` check to print stack trace
+ of a potentially escaped exception.
+
- Improved :doc:`bugprone-optional-value-conversion
<clang-tidy/checks/bugprone/optional-value-conversion>` check to detect
conversion in argument of ``std::make_optional``.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-coro.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-coro.cpp
index aff13d19fd209..829ec30353b94 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-coro.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-coro.cpp
@@ -221,6 +221,9 @@ Task<int> c_ShouldDiag(const int a, const int b) noexcept {
co_return a / b;
}
+// CHECK-MESSAGES: :[[@LINE-7]]:11: note: example of unhandled exception throw stack, starting from function 'c_ShouldDiag'
+// CHECK-MESSAGES: :[[@LINE-8]]:11: note: frame #0: function 'c_ShouldDiag'
+// CHECK-MESSAGES: :186:5: note: frame #1: function '~Evil' throws unhandled exception here
Task<int, true> d_ShouldNotDiag(const int a, const int b) {
co_return a / b;
@@ -230,6 +233,10 @@ Task<int, true> d_ShouldDiag(const int a, const int b) noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: an exception may be thrown in function 'd_ShouldDiag' which should not throw exceptions
co_return a / b;
}
+// CHECK-MESSAGES: :[[@LINE-4]]:17: note: example of unhandled exception throw stack, starting from function 'd_ShouldDiag'
+// CHECK-MESSAGES: :[[@LINE-5]]:17: note: frame #0: function 'd_ShouldDiag'
+// CHECK-MESSAGES: :104:8: note: frame #1: function 'get_return_object'
+// CHECK-MESSAGES: :54:7: note: frame #2: function 'Task' throws unhandled exception here
Task<int, false, true> e_ShouldNotDiag(const int a, const int b) {
co_return a / b;
@@ -239,6 +246,9 @@ Task<int, false, true> e_ShouldDiag(const int a, const int b) noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: an exception may be thrown in function 'e_ShouldDiag' which should not throw exceptions
co_return a / b;
}
+// CHECK-MESSAGES: :[[@LINE-4]]:24: note: example of unhandled exception throw stack, starting from function 'e_ShouldDiag'
+// CHECK-MESSAGES: :[[@LINE-5]]:24: note: frame #0: function 'e_ShouldDiag'
+// CHECK-MESSAGES: :100:7: note: frame #1: function 'Promise' throws unhandled exception here
Task<int, false, false, true> f_ShouldNotDiag(const int a, const int b) {
co_return a / b;
@@ -248,6 +258,9 @@ Task<int, false, false, true> f_ShouldDiag(const int a, const int b) noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: an exception may be thrown in function 'f_ShouldDiag' which should not throw exceptions
co_return a / b;
}
+// CHECK-MESSAGES: :[[@LINE-4]]:31: note: example of unhandled exception throw stack, starting from function 'f_ShouldDiag'
+// CHECK-MESSAGES: :[[@LINE-5]]:31: note: frame #0: function 'f_ShouldDiag'
+// CHECK-MESSAGES: :114:7: note: frame #1: function 'initial_suspend' throws unhandled exception here
Task<int, false, false, false, true> g_ShouldNotDiag(const int a, const int b) {
co_return a / b;
@@ -258,6 +271,9 @@ Task<int, false, false, false, true> g_ShouldDiag(const int a,
// CHECK-MESSAGES: :[[@LINE-2]]:38: warning: an exception may be thrown in function 'g_ShouldDiag' which should not throw exceptions
co_return a / b;
}
+// CHECK-MESSAGES: :[[@LINE-5]]:38: note: example of unhandled exception throw stack, starting from function 'g_ShouldDiag'
+// CHECK-MESSAGES: :[[@LINE-6]]:38: note: frame #0: function 'g_ShouldDiag'
+// CHECK-MESSAGES: :106:7: note: frame #1: function 'get_return_object' throws unhandled exception here
Task<int, false, false, false, false, true> h_ShouldNotDiag(const int a,
const int b) {
@@ -269,6 +285,9 @@ Task<int, false, false, false, false, true> h_ShouldDiag(const int a,
// CHECK-MESSAGES: :[[@LINE-2]]:45: warning: an exception may be thrown in function 'h_ShouldDiag' which should not throw exceptions
co_return a / b;
}
+// CHECK-MESSAGES: :[[@LINE-5]]:45: note: example of unhandled exception throw stack, starting from function 'h_ShouldDiag'
+// CHECK-MESSAGES: :[[@LINE-6]]:45: note: frame #0: function 'h_ShouldDiag'
+// CHECK-MESSAGES: :133:7: note: frame #1: function 'unhandled_exception' throws unhandled exception here
Task<int, false, false, false, false, false, true>
i_ShouldNotDiag(const int a, const int b) {
@@ -296,6 +315,8 @@ j_ShouldDiag(const int a, const int b) noexcept {
co_return a / b;
}
+// CHECK-MESSAGES: :[[@LINE-7]]:1: note: example of unhandled exception throw stack, starting from function 'j_ShouldDiag'
+// CHECK-MESSAGES: :[[@LINE-5]]:5: note: frame #0: function 'j_ShouldDiag' throws unhandled exception here
} // namespace coreturn
@@ -329,6 +350,9 @@ Task<int> c_ShouldDiag(const int a, const int b) noexcept {
co_yield a / b;
}
+// CHECK-MESSAGES: :[[@LINE-7]]:11: note: example of unhandled exception throw stack, starting from function 'c...
[truncated]
|
|
@llvm/pr-subscribers-clang-tools-extra Author: Baranov Victor (vbvictor) ChangesThis PR add stacktrace of escaped exception to
Example of new diagnostics: > warning: an exception may be thrown in function 'calls_non_and_throwing' which should not throw exceptions [bugprone-exception-escape] More example can be found it tests. Patch is 67.93 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/134375.diff 8 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
index 7e9551532b72f..0113da6ec1ac1 100644
--- a/clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
@@ -80,13 +80,46 @@ void ExceptionEscapeCheck::check(const MatchFinder::MatchResult &Result) {
if (!MatchedDecl)
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;
+ const utils::ExceptionAnalyzer::ExceptionInfo Info =
+ Tracer.analyze(MatchedDecl);
+
+ if (Info.getBehaviour() != utils::ExceptionAnalyzer::State::Throwing) {
+ return;
+ }
+
+ diag(MatchedDecl->getLocation(), "an exception may be thrown in function "
+ "%0 which should not throw exceptions")
+ << MatchedDecl;
+
+ const utils::ExceptionAnalyzer::ExceptionInfo::ThrowInfo ThrowInfo =
+ Info.getExceptions().begin()->getSecond();
+
+ if (ThrowInfo.Loc.isInvalid()) {
+ return;
+ }
+
+ // FIXME: We should provide exact position of functions calls, not only call
+ // stack of thrown exception.
+ const utils::ExceptionAnalyzer::CallStack &Stack = ThrowInfo.Stack;
+ diag(Stack.front()->getLocation(),
+ "example of unhandled exception throw stack, starting from function %0",
+ DiagnosticIDs::Note)
+ << Stack.front();
+
+ size_t FrameNo = 0;
+ for (const FunctionDecl *CallNode : Stack) {
+ if (FrameNo != Stack.size() - 1) {
+ diag(CallNode->getLocation(), "frame #%0: function %1",
+ DiagnosticIDs::Note)
+ << FrameNo << CallNode;
+ } else {
+ diag(ThrowInfo.Loc,
+ "frame #%0: function %1 throws unhandled exception here",
+ DiagnosticIDs::Note)
+ << FrameNo << CallNode;
+ }
+ ++FrameNo;
+ }
}
} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp b/clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
index e28ee7d9c70f7..42f04b07d88f8 100644
--- a/clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
+++ b/clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
@@ -11,10 +11,10 @@
namespace clang::tidy::utils {
void ExceptionAnalyzer::ExceptionInfo::registerException(
- const Type *ExceptionType) {
+ const Type *ExceptionType, const ThrowInfo &ThrowInfo) {
assert(ExceptionType != nullptr && "Only valid types are accepted");
Behaviour = State::Throwing;
- ThrownExceptions.insert(ExceptionType);
+ ThrownExceptions.insert({ExceptionType, ThrowInfo});
}
void ExceptionAnalyzer::ExceptionInfo::registerExceptions(
@@ -354,10 +354,12 @@ static bool canThrow(const FunctionDecl *Func) {
};
}
-bool ExceptionAnalyzer::ExceptionInfo::filterByCatch(
- const Type *HandlerTy, const ASTContext &Context) {
+ExceptionAnalyzer::ExceptionInfo::Throwables
+ExceptionAnalyzer::ExceptionInfo::filterByCatch(const Type *HandlerTy,
+ const ASTContext &Context) {
llvm::SmallVector<const Type *, 8> TypesToDelete;
- for (const Type *ExceptionTy : ThrownExceptions) {
+ for (const auto &ThrownException : ThrownExceptions) {
+ const Type *ExceptionTy = ThrownException.getFirst();
CanQualType ExceptionCanTy = ExceptionTy->getCanonicalTypeUnqualified();
CanQualType HandlerCanTy = HandlerTy->getCanonicalTypeUnqualified();
@@ -407,11 +409,18 @@ bool ExceptionAnalyzer::ExceptionInfo::filterByCatch(
}
}
- for (const Type *T : TypesToDelete)
- ThrownExceptions.erase(T);
+ Throwables DeletedExceptions;
+
+ for (const Type *TypeToDelete : TypesToDelete) {
+ const auto DeleteIt = ThrownExceptions.find(TypeToDelete);
+ if (DeleteIt != ThrownExceptions.end()) {
+ DeletedExceptions.insert(*DeleteIt);
+ ThrownExceptions.erase(DeleteIt);
+ }
+ }
reevaluateBehaviour();
- return !TypesToDelete.empty();
+ return DeletedExceptions;
}
ExceptionAnalyzer::ExceptionInfo &
@@ -420,7 +429,8 @@ ExceptionAnalyzer::ExceptionInfo::filterIgnoredExceptions(
llvm::SmallVector<const Type *, 8> TypesToDelete;
// Note: Using a 'SmallSet' with 'llvm::remove_if()' is not possible.
// Therefore this slightly hacky implementation is required.
- for (const Type *T : ThrownExceptions) {
+ for (const auto &ThrownException : ThrownExceptions) {
+ const Type *T = ThrownException.getFirst();
if (const auto *TD = T->getAsTagDecl()) {
if (TD->getDeclName().isIdentifier()) {
if ((IgnoreBadAlloc &&
@@ -452,10 +462,10 @@ void ExceptionAnalyzer::ExceptionInfo::reevaluateBehaviour() {
else
Behaviour = State::Throwing;
}
-
-ExceptionAnalyzer::ExceptionInfo ExceptionAnalyzer::throwsException(
- const FunctionDecl *Func, const ExceptionInfo::Throwables &Caught,
- llvm::SmallSet<const FunctionDecl *, 32> &CallStack) {
+ExceptionAnalyzer::ExceptionInfo
+ExceptionAnalyzer::throwsException(const FunctionDecl *Func,
+ const ExceptionInfo::Throwables &Caught,
+ CallStack &CallStack) {
if (!Func || CallStack.contains(Func) ||
(!CallStack.empty() && !canThrow(Func)))
return ExceptionInfo::createNonThrowing();
@@ -473,23 +483,25 @@ ExceptionAnalyzer::ExceptionInfo ExceptionAnalyzer::throwsException(
}
}
- CallStack.erase(Func);
+ CallStack.remove(Func);
return Result;
}
auto Result = ExceptionInfo::createUnknown();
if (const auto *FPT = Func->getType()->getAs<FunctionProtoType>()) {
for (const QualType &Ex : FPT->exceptions())
- Result.registerException(Ex.getTypePtr());
+ // FIXME add something to ThrowInfo
+ Result.registerException(Ex.getTypePtr(), {});
}
return Result;
}
/// Analyzes a single statement on it's throwing behaviour. This is in principle
/// possible except some 'Unknown' functions are called.
-ExceptionAnalyzer::ExceptionInfo ExceptionAnalyzer::throwsException(
- const Stmt *St, const ExceptionInfo::Throwables &Caught,
- llvm::SmallSet<const FunctionDecl *, 32> &CallStack) {
+ExceptionAnalyzer::ExceptionInfo
+ExceptionAnalyzer::throwsException(const Stmt *St,
+ const ExceptionInfo::Throwables &Caught,
+ CallStack &CallStack) {
auto Results = ExceptionInfo::createNonThrowing();
if (!St)
return Results;
@@ -503,7 +515,8 @@ ExceptionAnalyzer::ExceptionInfo ExceptionAnalyzer::throwsException(
->getPointeeType()
->getUnqualifiedDesugaredType();
Results.registerException(
- ThrownExpr->getType()->getUnqualifiedDesugaredType());
+ ThrownExpr->getType()->getUnqualifiedDesugaredType(),
+ {Throw->getBeginLoc(), CallStack});
} else
// A rethrow of a caught exception happens which makes it possible
// to throw all exception that are caught in the 'catch' clause of
@@ -518,7 +531,7 @@ ExceptionAnalyzer::ExceptionInfo ExceptionAnalyzer::throwsException(
// Everything is caught through 'catch(...)'.
if (!Catch->getExceptionDecl()) {
ExceptionInfo Rethrown = throwsException(
- Catch->getHandlerBlock(), Uncaught.getExceptionTypes(), CallStack);
+ Catch->getHandlerBlock(), Uncaught.getExceptions(), CallStack);
Results.merge(Rethrown);
Uncaught.clear();
} else {
@@ -534,12 +547,12 @@ ExceptionAnalyzer::ExceptionInfo ExceptionAnalyzer::throwsException(
// thrown types (because it's sensitive to inheritance) the throwing
// situation changes. First of all filter the exception types and
// analyze if the baseclass-exception is rethrown.
- if (Uncaught.filterByCatch(
- CaughtType, Catch->getExceptionDecl()->getASTContext())) {
- ExceptionInfo::Throwables CaughtExceptions;
- CaughtExceptions.insert(CaughtType);
- ExceptionInfo Rethrown = throwsException(Catch->getHandlerBlock(),
- CaughtExceptions, CallStack);
+ const ExceptionInfo::Throwables FilteredExceptions =
+ Uncaught.filterByCatch(CaughtType,
+ Catch->getExceptionDecl()->getASTContext());
+ if (!FilteredExceptions.empty()) {
+ ExceptionInfo Rethrown = throwsException(
+ Catch->getHandlerBlock(), FilteredExceptions, CallStack);
Results.merge(Rethrown);
}
}
@@ -567,9 +580,10 @@ ExceptionAnalyzer::ExceptionInfo ExceptionAnalyzer::throwsException(
}
ExceptionInfo Excs = throwsException(Coro->getBody(), Caught, CallStack);
Results.merge(throwsException(Coro->getExceptionHandler(),
- Excs.getExceptionTypes(), CallStack));
- for (const Type *Throwable : Excs.getExceptionTypes()) {
- if (const auto ThrowableRec = Throwable->getAsCXXRecordDecl()) {
+ Excs.getExceptions(), CallStack));
+ for (const auto &Exception : Excs.getExceptions()) {
+ const Type *ExcType = Exception.getFirst();
+ if (const CXXRecordDecl *ThrowableRec = ExcType->getAsCXXRecordDecl()) {
ExceptionInfo DestructorExcs =
throwsException(ThrowableRec->getDestructor(), Caught, CallStack);
Results.merge(DestructorExcs);
@@ -591,7 +605,7 @@ ExceptionAnalyzer::analyzeImpl(const FunctionDecl *Func) {
// Check if the function has already been analyzed and reuse that result.
const auto CacheEntry = FunctionCache.find(Func);
if (CacheEntry == FunctionCache.end()) {
- llvm::SmallSet<const FunctionDecl *, 32> CallStack;
+ CallStack CallStack;
ExceptionList =
throwsException(Func, ExceptionInfo::Throwables(), CallStack);
@@ -608,7 +622,7 @@ ExceptionAnalyzer::analyzeImpl(const FunctionDecl *Func) {
ExceptionAnalyzer::ExceptionInfo
ExceptionAnalyzer::analyzeImpl(const Stmt *Stmt) {
- llvm::SmallSet<const FunctionDecl *, 32> CallStack;
+ CallStack CallStack;
return throwsException(Stmt, ExceptionInfo::Throwables(), CallStack);
}
diff --git a/clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h b/clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h
index 6c2d693d64b50..0fea44fc8a622 100644
--- a/clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h
+++ b/clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h
@@ -12,6 +12,7 @@
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "llvm/ADT/SmallSet.h"
+#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringSet.h"
namespace clang::tidy::utils {
@@ -28,6 +29,12 @@ class ExceptionAnalyzer {
///< definition.
};
+ /// We use a SetVector to preserve the order of the functions in the call
+ /// stack as well as have fast lookup.
+ using CallStack = llvm::SetVector<const FunctionDecl *,
+ llvm::SmallVector<const FunctionDecl *, 32>,
+ llvm::DenseSet<const FunctionDecl *>, 32>;
+
/// Bundle the gathered information about an entity like a function regarding
/// it's exception behaviour. The 'NonThrowing'-state can be considered as the
/// neutral element in terms of information propagation.
@@ -37,7 +44,15 @@ class ExceptionAnalyzer {
/// exception at runtime.
class ExceptionInfo {
public:
- using Throwables = llvm::SmallSet<const Type *, 2>;
+ /// Holds information about where an exception is thrown.
+ /// First element in the call stack is analyzed function.
+ struct ThrowInfo {
+ SourceLocation Loc;
+ CallStack Stack;
+ };
+
+ using Throwables = llvm::SmallDenseMap<const Type *, ThrowInfo, 2>;
+
static ExceptionInfo createUnknown() { return {State::Unknown}; }
static ExceptionInfo createNonThrowing() { return {State::Throwing}; }
@@ -56,7 +71,8 @@ class ExceptionAnalyzer {
/// Register a single exception type as recognized potential exception to be
/// thrown.
- void registerException(const Type *ExceptionType);
+ void registerException(const Type *ExceptionType,
+ const ThrowInfo &ThrowInfo);
/// Registers a `SmallVector` of exception types as recognized potential
/// exceptions to be thrown.
@@ -73,8 +89,8 @@ class ExceptionAnalyzer {
/// This method is useful in case 'catch' clauses are analyzed as it is
/// possible to catch multiple exception types by one 'catch' if they
/// are a subclass of the 'catch'ed exception type.
- /// Returns 'true' if some exceptions were filtered, otherwise 'false'.
- bool filterByCatch(const Type *HandlerTy, const ASTContext &Context);
+ /// Returns filtered exceptions.
+ Throwables filterByCatch(const Type *HandlerTy, const ASTContext &Context);
/// Filter the set of thrown exception type against a set of ignored
/// types that shall not be considered in the exception analysis.
@@ -87,9 +103,9 @@ class ExceptionAnalyzer {
/// neutral.
void clear();
- /// References the set of known exception types that can escape from the
+ /// References the set of known exceptions that can escape from the
/// corresponding entity.
- const Throwables &getExceptionTypes() const { return ThrownExceptions; }
+ const Throwables &getExceptions() const { return ThrownExceptions; }
/// Signal if the there is any 'Unknown' element within the scope of
/// the related entity. This might be relevant if the entity is 'Throwing'
@@ -126,14 +142,12 @@ class ExceptionAnalyzer {
ExceptionInfo analyze(const Stmt *Stmt);
private:
- ExceptionInfo
- throwsException(const FunctionDecl *Func,
- const ExceptionInfo::Throwables &Caught,
- llvm::SmallSet<const FunctionDecl *, 32> &CallStack);
- ExceptionInfo
- throwsException(const Stmt *St, const ExceptionInfo::Throwables &Caught,
- llvm::SmallSet<const FunctionDecl *, 32> &CallStack);
-
+ ExceptionInfo throwsException(const FunctionDecl *Func,
+ const ExceptionInfo::Throwables &Caught,
+ CallStack &CallStack);
+ ExceptionInfo throwsException(const Stmt *St,
+ const ExceptionInfo::Throwables &Caught,
+ CallStack &CallStack);
ExceptionInfo analyzeImpl(const FunctionDecl *Func);
ExceptionInfo analyzeImpl(const Stmt *Stmt);
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 6cb8d572d3a78..36d06fce7232e 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -121,6 +121,10 @@ New check aliases
Changes in existing checks
^^^^^^^^^^^^^^^^^^^^^^^^^^
+- Improved :doc:`bugprone-exception-escape
+ <clang-tidy/checks/bugprone/exception-escape>` check to print stack trace
+ of a potentially escaped exception.
+
- Improved :doc:`bugprone-optional-value-conversion
<clang-tidy/checks/bugprone/optional-value-conversion>` check to detect
conversion in argument of ``std::make_optional``.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-coro.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-coro.cpp
index aff13d19fd209..829ec30353b94 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-coro.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-coro.cpp
@@ -221,6 +221,9 @@ Task<int> c_ShouldDiag(const int a, const int b) noexcept {
co_return a / b;
}
+// CHECK-MESSAGES: :[[@LINE-7]]:11: note: example of unhandled exception throw stack, starting from function 'c_ShouldDiag'
+// CHECK-MESSAGES: :[[@LINE-8]]:11: note: frame #0: function 'c_ShouldDiag'
+// CHECK-MESSAGES: :186:5: note: frame #1: function '~Evil' throws unhandled exception here
Task<int, true> d_ShouldNotDiag(const int a, const int b) {
co_return a / b;
@@ -230,6 +233,10 @@ Task<int, true> d_ShouldDiag(const int a, const int b) noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: an exception may be thrown in function 'd_ShouldDiag' which should not throw exceptions
co_return a / b;
}
+// CHECK-MESSAGES: :[[@LINE-4]]:17: note: example of unhandled exception throw stack, starting from function 'd_ShouldDiag'
+// CHECK-MESSAGES: :[[@LINE-5]]:17: note: frame #0: function 'd_ShouldDiag'
+// CHECK-MESSAGES: :104:8: note: frame #1: function 'get_return_object'
+// CHECK-MESSAGES: :54:7: note: frame #2: function 'Task' throws unhandled exception here
Task<int, false, true> e_ShouldNotDiag(const int a, const int b) {
co_return a / b;
@@ -239,6 +246,9 @@ Task<int, false, true> e_ShouldDiag(const int a, const int b) noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: an exception may be thrown in function 'e_ShouldDiag' which should not throw exceptions
co_return a / b;
}
+// CHECK-MESSAGES: :[[@LINE-4]]:24: note: example of unhandled exception throw stack, starting from function 'e_ShouldDiag'
+// CHECK-MESSAGES: :[[@LINE-5]]:24: note: frame #0: function 'e_ShouldDiag'
+// CHECK-MESSAGES: :100:7: note: frame #1: function 'Promise' throws unhandled exception here
Task<int, false, false, true> f_ShouldNotDiag(const int a, const int b) {
co_return a / b;
@@ -248,6 +258,9 @@ Task<int, false, false, true> f_ShouldDiag(const int a, const int b) noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:31: warning: an exception may be thrown in function 'f_ShouldDiag' which should not throw exceptions
co_return a / b;
}
+// CHECK-MESSAGES: :[[@LINE-4]]:31: note: example of unhandled exception throw stack, starting from function 'f_ShouldDiag'
+// CHECK-MESSAGES: :[[@LINE-5]]:31: note: frame #0: function 'f_ShouldDiag'
+// CHECK-MESSAGES: :114:7: note: frame #1: function 'initial_suspend' throws unhandled exception here
Task<int, false, false, false, true> g_ShouldNotDiag(const int a, const int b) {
co_return a / b;
@@ -258,6 +271,9 @@ Task<int, false, false, false, true> g_ShouldDiag(const int a,
// CHECK-MESSAGES: :[[@LINE-2]]:38: warning: an exception may be thrown in function 'g_ShouldDiag' which should not throw exceptions
co_return a / b;
}
+// CHECK-MESSAGES: :[[@LINE-5]]:38: note: example of unhandled exception throw stack, starting from function 'g_ShouldDiag'
+// CHECK-MESSAGES: :[[@LINE-6]]:38: note: frame #0: function 'g_ShouldDiag'
+// CHECK-MESSAGES: :106:7: note: frame #1: function 'get_return_object' throws unhandled exception here
Task<int, false, false, false, false, true> h_ShouldNotDiag(const int a,
const int b) {
@@ -269,6 +285,9 @@ Task<int, false, false, false, false, true> h_ShouldDiag(const int a,
// CHECK-MESSAGES: :[[@LINE-2]]:45: warning: an exception may be thrown in function 'h_ShouldDiag' which should not throw exceptions
co_return a / b;
}
+// CHECK-MESSAGES: :[[@LINE-5]]:45: note: example of unhandled exception throw stack, starting from function 'h_ShouldDiag'
+// CHECK-MESSAGES: :[[@LINE-6]]:45: note: frame #0: function 'h_ShouldDiag'
+// CHECK-MESSAGES: :133:7: note: frame #1: function 'unhandled_exception' throws unhandled exception here
Task<int, false, false, false, false, false, true>
i_ShouldNotDiag(const int a, const int b) {
@@ -296,6 +315,8 @@ j_ShouldDiag(const int a, const int b) noexcept {
co_return a / b;
}
+// CHECK-MESSAGES: :[[@LINE-7]]:1: note: example of unhandled exception throw stack, starting from function 'j_ShouldDiag'
+// CHECK-MESSAGES: :[[@LINE-5]]:5: note: frame #0: function 'j_ShouldDiag' throws unhandled exception here
} // namespace coreturn
@@ -329,6 +350,9 @@ Task<int> c_ShouldDiag(const int a, const int b) noexcept {
co_yield a / b;
}
+// CHECK-MESSAGES: :[[@LINE-7]]:11: note: example of unhandled exception throw stack, starting from function 'c...
[truncated]
|
63c7ec8 to
9865b1a
Compare
|
Ping, appreciate it if someone will take a look at these changes. |
PiotrZSL
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about unknown exceptions ?
Take a look into https://reviews.llvm.org/D153298, mainly into comments and approach if everything is covered in your change.
cb2a4c2 to
96a8d47
Compare
|
Added handling of unknown exceptions, now we diag on place where e.g. |
96a8d47 to
bd9db71
Compare
|
Ping |
1 similar comment
|
Ping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add some separate tests for this diagnostic? I am not sure existed test case is covered all scenarios in this PR
| // CHECK-MESSAGES: :[[@LINE-2]]:45: warning: an exception may be thrown in function 'h_ShouldDiag' which should not throw exceptions | ||
| co_yield a / b; | ||
| } | ||
| // CHECK-MESSAGES: :133:7: note: frame #0: unhandled exception may be thrown in function 'unhandled_exception' here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I do not like hard code line number in FileCheck, but I don't know how to resolve this issue. @EugeneZelenko Is there any better solution to avoid it?
895fa78 to
905a31b
Compare
I've added tests with:
If there are any more tests that I should, please tell me. |
|
Ping @HerrCai0907 if you wish to re-review tests |
|
Ping if anyone want to take a look, I'm planning to merge it next week |
823a345 to
a71a485
Compare
|
Ran this check on |
a71a485 to
2e14a63
Compare
This PR add stacktrace of escaped exception to
bugprone-exception-escapecheck.Changes:
ExceptionAnalyzerandExceptionInfoclasses to hold stacktrace of escaped exception inllvm::MapVector.llvm::MapVectoris needed to hold relative positions of functions in stack as well as have fast lookup.misc-no-recursioncheck.Example of new diagnostics:
More example can be found it tests.
Performance downgrade:
Run tests on poco library, which is known for using exceptions. 679 files total.
Before:
timecommandsummed-up
--enable-check-profilecommandAfter:
timecommandsummed-up
--enable-check-profilecommandFixes #87422