llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: mitchell (zeyi2) <details> <summary>Changes</summary> Closes https://github.com/llvm/llvm-project/issues/164795 --- Full diff: https://github.com/llvm/llvm-project/pull/168324.diff 8 Files Affected: - (modified) clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp (+49-31) - (modified) clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.h (+3) - (modified) clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp (+13) - (modified) clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h (+26-1) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+5-1) - (modified) clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-escape.rst (+12) - (added) clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-known-unannotated-option.cpp (+25) - (added) clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-unannotated-option.cpp (+25) ``````````diff diff --git a/clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp index b7de8395ffa05..b2415b59b135d 100644 --- a/clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp @@ -42,7 +42,10 @@ ExceptionEscapeCheck::ExceptionEscapeCheck(StringRef Name, CheckDestructors(Options.get("CheckDestructors", true)), CheckMoveMemberFunctions(Options.get("CheckMoveMemberFunctions", true)), CheckMain(Options.get("CheckMain", true)), - CheckNothrowFunctions(Options.get("CheckNothrowFunctions", true)) { + CheckNothrowFunctions(Options.get("CheckNothrowFunctions", true)), + KnownUnannotatedAsThrowing( + Options.get("KnownUnannotatedAsThrowing", false)), + UnknownAsThrowing(Options.get("UnknownAsThrowing", false)) { llvm::SmallVector<StringRef, 8> FunctionsThatShouldNotThrowVec, IgnoredExceptionsVec, CheckedSwapFunctionsVec; RawFunctionsThatShouldNotThrow.split(FunctionsThatShouldNotThrowVec, ",", -1, @@ -57,6 +60,7 @@ ExceptionEscapeCheck::ExceptionEscapeCheck(StringRef Name, IgnoredExceptions.insert_range(IgnoredExceptionsVec); Tracer.ignoreExceptions(std::move(IgnoredExceptions)); Tracer.ignoreBadAlloc(true); + Tracer.assumeUnannotatedFunctionsThrow(KnownUnannotatedAsThrowing); } void ExceptionEscapeCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { @@ -68,6 +72,8 @@ void ExceptionEscapeCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "CheckMoveMemberFunctions", CheckMoveMemberFunctions); Options.store(Opts, "CheckMain", CheckMain); Options.store(Opts, "CheckNothrowFunctions", CheckNothrowFunctions); + Options.store(Opts, "KnownUnannotatedAsThrowing", KnownUnannotatedAsThrowing); + Options.store(Opts, "UnknownAsThrowing", UnknownAsThrowing); } void ExceptionEscapeCheck::registerMatchers(MatchFinder *Finder) { @@ -103,41 +109,53 @@ void ExceptionEscapeCheck::check(const MatchFinder::MatchResult &Result) { 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 Behaviour = Info.getBehaviour(); + const bool IsThrowing = + Behaviour == utils::ExceptionAnalyzer::State::Throwing; + const bool IsUnknown = Behaviour == utils::ExceptionAnalyzer::State::Unknown; - const auto &[ThrowType, ThrowInfo] = *Info.getExceptions().begin(); + const bool ReportUnknown = + IsUnknown && + ((KnownUnannotatedAsThrowing && Info.hasUnknownFromKnownUnannotated()) || + (UnknownAsThrowing && Info.hasUnknownFromMissingDefinition())); - if (ThrowInfo.Loc.isInvalid()) + if (!(IsThrowing || ReportUnknown)) 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; + diag(MatchedDecl->getLocation(), "an exception may be thrown in function %0 " + "which should not throw exceptions") + << MatchedDecl; + + if (!Info.getExceptions().empty()) { + 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; } - ++FrameNo; } } diff --git a/clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.h b/clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.h index c3bf4a4335273..ba65640435368 100644 --- a/clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.h @@ -42,6 +42,9 @@ class ExceptionEscapeCheck : public ClangTidyCheck { const bool CheckMain; const bool CheckNothrowFunctions; + const bool KnownUnannotatedAsThrowing; + const bool UnknownAsThrowing; + llvm::StringSet<> FunctionsThatShouldNotThrow; llvm::StringSet<> CheckedSwapFunctions; utils::ExceptionAnalyzer Tracer; diff --git a/clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp b/clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp index c774f54b1da5a..221f7711786de 100644 --- a/clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp +++ b/clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp @@ -39,6 +39,10 @@ ExceptionAnalyzer::ExceptionInfo &ExceptionAnalyzer::ExceptionInfo::merge( Behaviour = State::Unknown; ContainsUnknown = ContainsUnknown || Other.ContainsUnknown; + UnknownFromMissingDefinition = + UnknownFromMissingDefinition || Other.UnknownFromMissingDefinition; + UnknownFromKnownUnannotated = + UnknownFromKnownUnannotated || Other.UnknownFromKnownUnannotated; ThrownExceptions.insert_range(Other.ThrownExceptions); return *this; } @@ -484,10 +488,19 @@ ExceptionAnalyzer::ExceptionInfo ExceptionAnalyzer::throwsException( } CallStack.erase(Func); + // Optionally treat unannotated functions as potentially throwing if they + // are not explicitly non-throwing and no throw was discovered. + if (AssumeUnannotatedThrowing && + Result.getBehaviour() == State::NotThrowing && canThrow(Func)) { + auto Unknown = ExceptionInfo::createUnknown(); + Unknown.markUnknownFromKnownUnannotated(); + return Unknown; + } return Result; } auto Result = ExceptionInfo::createUnknown(); + Result.markUnknownFromMissingDefinition(); if (const auto *FPT = Func->getType()->getAs<FunctionProtoType>()) { for (const QualType &Ex : FPT->exceptions()) { CallStack.insert({Func, CallLoc}); diff --git a/clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h b/clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h index 1a277c8a6d3b2..c0356b71383fb 100644 --- a/clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h +++ b/clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h @@ -52,7 +52,7 @@ class ExceptionAnalyzer { using Throwables = llvm::SmallDenseMap<const Type *, ThrowInfo, 2>; static ExceptionInfo createUnknown() { return {State::Unknown}; } - static ExceptionInfo createNonThrowing() { return {State::Throwing}; } + static ExceptionInfo createNonThrowing() { return {State::NotThrowing}; } /// By default the exception situation is unknown and must be /// clarified step-wise. @@ -67,6 +67,22 @@ class ExceptionAnalyzer { State getBehaviour() const { return Behaviour; } + /// Unknown cause tracking. + void markUnknownFromMissingDefinition() { + UnknownFromMissingDefinition = true; + ContainsUnknown = true; + } + void markUnknownFromKnownUnannotated() { + UnknownFromKnownUnannotated = true; + ContainsUnknown = true; + } + bool hasUnknownFromMissingDefinition() const { + return UnknownFromMissingDefinition; + } + bool hasUnknownFromKnownUnannotated() const { + return UnknownFromKnownUnannotated; + } + /// Register a single exception type as recognized potential exception to be /// thrown. void registerException(const Type *ExceptionType, @@ -124,12 +140,20 @@ class ExceptionAnalyzer { /// after filtering. bool ContainsUnknown; + bool UnknownFromMissingDefinition = false; + bool UnknownFromKnownUnannotated = false; + /// 'ThrownException' is empty if the 'Behaviour' is either 'NotThrowing' or /// 'Unknown'. Throwables ThrownExceptions; }; ExceptionAnalyzer() = default; + /// When enabled, treat any function that is not explicitly non-throwing + /// as potentially throwing, even if its body analysis finds no throw. + void assumeUnannotatedFunctionsThrow(bool Enable) { + AssumeUnannotatedThrowing = Enable; + } void ignoreBadAlloc(bool ShallIgnore) { IgnoreBadAlloc = ShallIgnore; } void ignoreExceptions(llvm::StringSet<> ExceptionNames) { @@ -154,6 +178,7 @@ class ExceptionAnalyzer { bool IgnoreBadAlloc = true; llvm::StringSet<> IgnoredExceptions; llvm::DenseMap<const FunctionDecl *, ExceptionInfo> FunctionCache{32U}; + bool AssumeUnannotatedThrowing = false; }; } // namespace clang::tidy::utils diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index b982216297919..d671efa8b388f 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -328,7 +328,11 @@ Changes in existing checks where the check wouldn't diagnose throws in arguments to functions or constructors. Added fine-grained configuration via options `CheckDestructors`, `CheckMoveMemberFunctions`, `CheckMain`, - `CheckedSwapFunctions`, and `CheckNothrowFunctions`. + `CheckedSwapFunctions`, and `CheckNothrowFunctions`; and added + ``KnownUnannotatedAsThrowing`` and ``UnknownAsThrowing`` to support + reporting for unannotated functions, enabling reporting when no explicit + ``throw`` is seen and allowing separate tuning for known and unknown + implementations. - Improved :doc:`bugprone-infinite-loop <clang-tidy/checks/bugprone/infinite-loop>` check by adding detection for diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-escape.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-escape.rst index 7d724a4581715..66e8acaa242cf 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-escape.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/exception-escape.rst @@ -71,3 +71,15 @@ Options Comma separated list containing type names which are not counted as thrown exceptions in the check. Default value is an empty string. + +.. option:: KnownUnannotatedAsThrowing + + When `true`, treat calls to functions with visible definitions that are not + explicitly declared as non-throwing (i.e. lack ``noexcept`` or ``throw()``) + as potentially throwing, even if their bodies are visible and no explicit + throw is found. Default value is `false`. + +.. option:: UnknownAsThrowing + + When `true`, treat calls to functions without visible definitions as + potentially throwing. Default value is `false`. diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-known-unannotated-option.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-known-unannotated-option.cpp new file mode 100644 index 0000000000000..7f72870f43ea9 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-known-unannotated-option.cpp @@ -0,0 +1,25 @@ +// RUN: %check_clang_tidy -std=c++11-or-later %s bugprone-exception-escape %t -- \ +// RUN: -config='{"CheckOptions": { \ +// RUN: "bugprone-exception-escape.KnownUnannotatedAsThrowing": true \ +// RUN: }}' -- -fexceptions + +void unannotated_no_throw_body() {} + +void calls_unannotated() noexcept { + // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'calls_unannotated' which should not throw exceptions + unannotated_no_throw_body(); +} + +void extern_declared(); + +void calls_unknown() noexcept { + // CHECK-MESSAGES-NOT: warning: + extern_declared(); +} + +void definitely_nothrow() noexcept {} + +void calls_nothrow() noexcept { + // CHECK-MESSAGES-NOT: warning: + definitely_nothrow(); +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-unannotated-option.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-unannotated-option.cpp new file mode 100644 index 0000000000000..c92eaa75c6adf --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-unannotated-option.cpp @@ -0,0 +1,25 @@ +// RUN: %check_clang_tidy -std=c++11-or-later %s bugprone-exception-escape %t -- \ +// RUN: -config='{"CheckOptions": { \ +// RUN: "bugprone-exception-escape.UnknownAsThrowing": true \ +// RUN: }}' -- -fexceptions + +void unannotated_no_throw_body() {} + +void calls_unannotated() noexcept { + // CHECK-MESSAGES-NOT: warning: + unannotated_no_throw_body(); +} + +void extern_declared(); + +void calls_unknown() noexcept { + // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'calls_unknown' which should not throw exceptions + extern_declared(); +} + +void definitely_nothrow() noexcept {} + +void calls_nothrow() noexcept { + // CHECK-MESSAGES-NOT: warning: + definitely_nothrow(); +} `````````` </details> https://github.com/llvm/llvm-project/pull/168324 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
