[clang-tidy] add stacktrace to exception-escape

Author: vbvictor

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

@@ -78,13 +78,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

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,10 +464,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();
@@ -475,23 +485,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());
+      // Nothing in ThrowInfo because there is no location of 'throw'
+      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;
@@ -505,7 +517,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 +533,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,12 +549,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);
         }
       }
@@ -569,9 +582,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);
@@ -593,7 +607,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);
 
@@ -610,7 +624,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,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);
 

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.