[clang-tidy] Improve bugprone-exception-escape: add stacktrace of escaped exception (#134375)

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::MapVector`. `llvm::MapVector`
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.

Fixes llvm/llvm-project#87422.

Author: vbvictor

clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp

@@ -78,13 +78,45 @@ 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 auto &[ThrowType, ThrowInfo] = *Info.getExceptions().begin();
+
+  if (ThrowInfo.Loc.isInvalid())
+    return;
+
+  const utils::ExceptionAnalyzer::CallStack &Stack = ThrowInfo.Stack;
+  diag(ThrowInfo.Loc,
+       "frame #0: unhandled exception of type %0 may be thrown in function %1 "
+       "here",
+       DiagnosticIDs::Note)
+      << QualType(ThrowType, 0U) << Stack.back().first;
+
+  size_t FrameNo = 1;
+  for (auto CurrIt = ++Stack.rbegin(), PrevIt = Stack.rbegin();
+       CurrIt != Stack.rend(); ++CurrIt, ++PrevIt) {
+    const FunctionDecl *CurrFunction = CurrIt->first;
+    const FunctionDecl *PrevFunction = PrevIt->first;
+    const SourceLocation PrevLocation = PrevIt->second;
+    if (PrevLocation.isValid()) {
+      diag(PrevLocation, "frame #%0: function %1 calls function %2 here",
+           DiagnosticIDs::Note)
+          << FrameNo << CurrFunction << PrevFunction;
+    } else {
+      diag(CurrFunction->getLocation(),
+           "frame #%0: function %1 calls function %2", DiagnosticIDs::Note)
+          << FrameNo << CurrFunction << PrevFunction;
+    }
+    ++FrameNo;
+  }
 }
 
 } // namespace clang::tidy::bugprone

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(
@@ -356,10 +356,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();
 
@@ -409,11 +411,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 &
@@ -422,7 +431,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 &&
@@ -454,16 +464,15 @@ 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) {
+    CallStack &CallStack, SourceLocation CallLoc) {
   if (!Func || CallStack.contains(Func) ||
       (!CallStack.empty() && !canThrow(Func)))
     return ExceptionInfo::createNonThrowing();
 
   if (const Stmt *Body = Func->getBody()) {
-    CallStack.insert(Func);
+    CallStack.insert({Func, CallLoc});
     ExceptionInfo Result = throwsException(Body, Caught, CallStack);
 
     // For a constructor, we also have to check the initializers.
@@ -481,17 +490,23 @@ ExceptionAnalyzer::ExceptionInfo ExceptionAnalyzer::throwsException(
 
   auto Result = ExceptionInfo::createUnknown();
   if (const auto *FPT = Func->getType()->getAs<FunctionProtoType>()) {
-    for (const QualType &Ex : FPT->exceptions())
-      Result.registerException(Ex.getTypePtr());
+    for (const QualType &Ex : FPT->exceptions()) {
+      CallStack.insert({Func, CallLoc});
+      Result.registerException(
+          Ex.getTypePtr(),
+          {Func->getExceptionSpecSourceRange().getBegin(), CallStack});
+      CallStack.erase(Func);
+    }
   }
   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;
@@ -505,7 +520,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
@@ -520,7 +536,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 {
@@ -536,25 +552,26 @@ 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);
         }
       }
     }
     Results.merge(Uncaught);
   } else if (const auto *Call = dyn_cast<CallExpr>(St)) {
     if (const FunctionDecl *Func = Call->getDirectCallee()) {
-      ExceptionInfo Excs = throwsException(Func, Caught, CallStack);
+      ExceptionInfo Excs =
+          throwsException(Func, Caught, CallStack, Call->getBeginLoc());
       Results.merge(Excs);
     }
   } else if (const auto *Construct = dyn_cast<CXXConstructExpr>(St)) {
-    ExceptionInfo Excs =
-        throwsException(Construct->getConstructor(), Caught, CallStack);
+    ExceptionInfo Excs = throwsException(Construct->getConstructor(), Caught,
+                                         CallStack, Construct->getBeginLoc());
     Results.merge(Excs);
   } else if (const auto *DefaultInit = dyn_cast<CXXDefaultInitExpr>(St)) {
     ExceptionInfo Excs =
@@ -569,11 +586,12 @@ 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()) {
-        ExceptionInfo DestructorExcs =
-            throwsException(ThrowableRec->getDestructor(), Caught, CallStack);
+                                  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, SourceLocation{});
         Results.merge(DestructorExcs);
       }
     }
@@ -593,9 +611,9 @@ 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;
-    ExceptionList =
-        throwsException(Func, ExceptionInfo::Throwables(), CallStack);
+    CallStack CallStack;
+    ExceptionList = throwsException(Func, ExceptionInfo::Throwables(),
+                                    CallStack, Func->getLocation());
 
     // Cache the result of the analysis. This is done prior to filtering
     // because it is best to keep as much information as possible.
@@ -610,7 +628,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);
 }
 

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,10 @@ class ExceptionAnalyzer {
                  ///< definition.
   };
 
+  /// We use a MapVector to preserve the order of the functions in the call
+  /// stack as well as have fast lookup.
+  using CallStack = llvm::MapVector<const FunctionDecl *, SourceLocation>;
+
   /// 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 +42,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 +69,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 +87,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 +101,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 +140,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, SourceLocation CallLoc);
+  ExceptionInfo throwsException(const Stmt *St,
+                                const ExceptionInfo::Throwables &Caught,
+                                CallStack &CallStack);
   ExceptionInfo analyzeImpl(const FunctionDecl *Func);
   ExceptionInfo analyzeImpl(const Stmt *Stmt);
 

clang-tools-extra/docs/ReleaseNotes.rst

@@ -187,6 +187,10 @@ Changes in existing checks
   false positives on deleted constructors that cannot be used to construct
   objects, even if they have public or protected access.
 
+- Improved :doc:`bugprone-exception-escape
+  <clang-tidy/checks/bugprone/exception-escape>` check to print stack trace
+  of a potentially escaped exception.
+
 - Added an option to :doc:`bugprone-multi-level-implicit-pointer-conversion
   <clang-tidy/checks/bugprone/multi-level-implicit-pointer-conversion>` to
   choose whether to enable the check in C code or not.