-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[clang-tidy] Add options to throw unannotated functions in bugprone-exception-escape
#168324
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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,22 +109,35 @@ void ExceptionEscapeCheck::check(const MatchFinder::MatchResult &Result) { | |
| const utils::ExceptionAnalyzer::ExceptionInfo Info = | ||
| Tracer.analyze(MatchedDecl); | ||
|
|
||
| if (Info.getBehaviour() != utils::ExceptionAnalyzer::State::Throwing) | ||
| const auto Behaviour = Info.getBehaviour(); | ||
| const bool IsThrowing = | ||
| Behaviour == utils::ExceptionAnalyzer::State::Throwing; | ||
| const bool IsUnknown = Behaviour == utils::ExceptionAnalyzer::State::Unknown; | ||
|
|
||
| const bool ReportUnknown = | ||
| IsUnknown && | ||
| ((KnownUnannotatedAsThrowing && Info.hasUnknownFromKnownUnannotated()) || | ||
| (UnknownAsThrowing && Info.hasUnknownFromMissingDefinition())); | ||
|
|
||
| if (!(IsThrowing || ReportUnknown)) | ||
| return; | ||
|
|
||
| diag(MatchedDecl->getLocation(), "an exception may be thrown in function " | ||
| "%0 which should not throw exceptions") | ||
| diag(MatchedDecl->getLocation(), "an exception may be thrown in function %0 " | ||
| "which should not throw exceptions") | ||
| << MatchedDecl; | ||
|
|
||
| if (Info.getExceptions().empty()) | ||
| return; | ||
|
|
||
| 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", | ||
| "frame #0: unhandled exception of type %0 may be thrown in function " | ||
| "%1 here", | ||
|
Comment on lines
+139
to
+140
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same seems unrelated |
||
| DiagnosticIDs::Note) | ||
| << QualType(ThrowType, 0U) << Stack.back().first; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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}; } | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand there are valid reasons for this function to return If this change doesn't match your preference, I'm totally fine reverting it. Thanks in advance. |
||
|
|
||
| /// 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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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`. | ||
|
Comment on lines
+75
to
+85
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think instead of 2 options we should make 1 option with 3 different levels: Because i don't see a good usecase when we want to set |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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(); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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(); | ||
| } |
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.
Did clang-format changed it? Seems unrelated