llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy Author: Baranov Victor (vbvictor) <details> <summary>Changes</summary> This PR add stacktrace of escaped exception to `bugprone-exception-escape` check. Changes: 1. Modified `ExceptionAnalyzer` and `ExceptionInfo` classes to hold stacktrace of escaped exception in `llvm::SetVector`. `SetVector` is needed to hold relative positions of functions in stack as well as have fast lookup. 2. Added new diagnostics based of `misc-no-recursion` check. Example of new diagnostics: > warning: an exception may be thrown in function 'calls_non_and_throwing' which should not throw exceptions [bugprone-exception-escape] note: example of unhandled exception throw stack, starting from function 'calls_non_and_throwing' note: frame #<!-- -->0: function 'calls_non_and_throwing' note: frame #<!-- -->1: function 'test_basic_throw' throws unhandled exception here 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: - (modified) clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp (+40-7) - (modified) clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp (+45-31) - (modified) clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h (+28-14) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4) - (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-coro.cpp (+124) - (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-rethrow.cpp (+4) - (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-throw.cpp (+9) - (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp (+107) ``````````diff 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] `````````` </details> https://github.com/llvm/llvm-project/pull/134375 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
