Finds functions which should not throw exceptions: Destructors, move constructors, move assignment operators, the main() function, swap() functions, functions marked with throw() or noexcept and functions given as option to the checker.
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).
| docs/clang-tidy/checks/misc-exception-escape.rst | ||
|---|---|---|
| 7 ↗ | (On Diff #100205) | I think - or * is more correct. |
| 10 ↗ | (On Diff #100205) | Please enclose main() in ``. Same for other functions and keywords. |
| 29 ↗ | (On Diff #100205) | Please add () to WinMain and enclose it in ``. |
| 35 ↗ | (On Diff #100205) | Please use check instead of checker. |
Comment Actions
How is that compared to https://reviews.llvm.org/D19201 and the clang patch mentioned in this patch?
Comment Actions
Actually, this check does much more. It does not only check for noexcept (and throw()) functions, but also for destructors, move constructors, move assignments, the main() function, swap() functions and also functions given as option. A more important difference is that we traverse the whole call-chain and check all the throw statements and try-catch blocks so indirectly throwing functions are also reported and no flase positives are caused by throw and catch in the same try block.
Comment Actions
I think we should try to get as much of this functionality in D33333 as possible; there is a considerable amount of overlap between that functionality and this functionality. This check can then cover only the parts that are not reasonably handled by the frontend check instead of duplicating diagnostics the user already receives.
Comment Actions
I suppose that the frontend check will not travarse the call-graph just check direct throws. Should we only check indirect throws then?
Comment Actions
I tested the latest revision (the fronted patch already included) on my test file. Disregarding of the not so important parameters (EnabledFunctions and IgnoredExceptions) I do not get warnings for any indirect throws (indirect_implicit() and indirect_explicit()). So for me it does not seem to be using the CFG. Furthermore, I do not get warning for throw_and_catch_some() where 1.1 is a double thus catch(int &) should not catch it. The same happens in throw_catch_rethrow_the_rest(), where catch(int &) should not catch 1.1, but catch(...) should catch and rethrow it. This latter may be a bug.
Comment Actions
Jen Yu can clarify, but I believe it was decided to put the implementation in the CFG, but not do a full analysis (leave that for a later implementation). It is not doing 'catch' analysis, and I don't believe they decided to do analysis on the function call itself, since the false-positive rate is massive thanks to the C headers.
Comment Actions
The patch do not do indirect function call check, since that would cause false-positive.
The tests of throw_and_catch_some and throw_catch_rethrow_the_rest no warning emit due the "throw 1.1" is unreachable code after "throw 1", it is not the bug. I am not sure clang has some option to emit unreachable code.
Comment Actions
Test changed. I made some bad throws reachable, but the frontend check still does detects them.
Comment Actions
If block contains two or more throws, that mean compiler can not statically know throw is really throws. So to avoid fails positive, if one of throw can be catch, compiler don't emit warning.
Jennifer
Comment Actions
I think that the acceptable false positive rate of a compiler warning is much lower than that of a Tidy check. So I understand that the fronted patch will not handle the indirect cases (which are the most important in my opinion) or the cases where there are multiple throws in a try block. However, it is nearly impossible to remove the most straightforward cases from the check and also in today's Tidy checks or Static Analyzer checkers there is some overlapping with warnings. I think that maybe we should consider making the error locations and error text to overlap with the warning so an advanced tool (such as CodeChecker) could unify them.
Comment Actions
I agree that we should not spend too much effort on making warnings from the compiler and tidy disjunct.
Comment Actions
I agree that we should not spend too much effort on making warnings from the compiler and tidy disjunct.
+1
There is an effort to treat clangs frontend warnings similar to clang-tidy checks within clang-tidy. This would allow to manage the overlap as well.
Will this check find stuff like this (or is it part of the frontend)
int throwing() {
throw int(42);
}
int not_throwing() noexcept {
throwing();
}It would be nice to have a check that could automatically introduce noexcept into a codebase for cases it safe. I think this path would be a good place for it.
Comment Actions
Will this check find stuff like this (or is it part of the frontend)
int throwing() { throw int(42); } int not_throwing() noexcept { throwing(); }
Yes, see lines 171-193 of the test file.
Comment Actions
Looking at the documentation and the examples, it is unclear to me what the behavior of checking is when you call a function without throw/noexcept if you can't see the implementation. (Other cpp file)
Comment Actions
We take the conservative approach there: if a function has no throw specifier we handle it as it would have a noexcept specifier.
Comment Actions
Is there a more specific module for this check than misc? For example, does it check a rule that happens to appear in a certain coding standard? Otherwise, maybe "bugprone-"? I've moved a number of checks out of misc- and it would be nice to avoid putting more stuff there.
| docs/clang-tidy/checks/misc-exception-escape.rst | ||
|---|---|---|
| 1 ↗ | (On Diff #139583) | This file should be renamed as well. |
| docs/ReleaseNotes.rst | ||
|---|---|---|
| 68 | Please fix link. See other checks as example. | |
| clang-tidy/bugprone/ExceptionEscapeCheck.cpp | ||
|---|---|---|
| 25–26 | Function names should follow llvm conventions http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly | |
| clang-tidy/bugprone/ExceptionEscapeCheck.cpp | ||
|---|---|---|
| 25–26 | If there's no need to pass nullptr to these functions, the arguments can be const references. | |
| 116 | No else after return. | |
| 143 | Don't use getNameAsString when not necessary. if (TD->getDeclName().isIdentifier() && TD->getName() == "bad_alloc") will work as good, but without unnecessary string copies. | |
| clang-tidy/bugprone/ExceptionEscapeCheck.cpp | ||
|---|---|---|
| 25–26 | I agree in general, but I usually get Stmt* and FunctionDecl* from query functions, so it is simpler to pass them as they are instead of dereferencing them. The too many asterisks decrease the readability of the code. Furthermore, St may be nullptr in the second function. | |
| docs/ReleaseNotes.rst | ||
|---|---|---|
| 68 | .html is not necessary. | |
| clang-tidy/bugprone/ExceptionEscapeCheck.cpp | ||
|---|---|---|
| 101 |
http://llvm.org/docs/CodingStandards.html#anonymous-namespaces | |
| 106 | I don't think this is a good use case for a static local variable. If this function needs a state, either pass it from the caller or create a utility class/struct for it. You can leave a non-recursive entry point with the current interface, if you like. For example: const TypeVec throwsException(const FunctionDecl *Func, llvm::SmallSet<const FunctionDecl *, 32> *CallStack) { ... }
const TypeVec throwsException(const FunctionDecl *Func) {
llvm::SmallSet<const FunctionDecl *, 32> CallStack;
return throwsException(Func, &CallStack);
} | |
| 111 | Please use the concrete type here, since it's not obvious from the context. | |
| 120 | Please use the concrete type here. | |
| 121 | Ex.getTypePtrOrNull() / Ex.getTypePtr() would be easier to understand here. | |
| 151 | Please use the concrete type here and below. http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable says
| |
| 167 |
| |
| 177 | I'd put this as close to the remove_if as possible to allow readers keep less context to understand the code. In this case - as the first statement inside the if. | |
Comment Actions
Thank you for your thorough review, I will do the fixes next week, after the Euro LLVM.
Comment Actions
Thank you for the updates. A few more comments.
| clang-tidy/bugprone/ExceptionEscapeCheck.cpp | ||
|---|---|---|
| 18 | You can open namespace clang here instead of this using directive. | |
| 23 | There's llvm::StringSet<> in llvm/ADT/StringSet.h. | |
| 26 | Too many forward declarations for my taste. Can you just move all static functions here and remove unnecessary forward declarations? I guess, one should be enough for all the functions here. | |
| 151 | I don't remember where this comment was, but now I see a few more instances of the same issue. Specifically, ThrownExpr and ThrownType here and CaughtType below. | |
| 179 | iterator being a pointer is an implementation detail of SmallVector. s/auto \*/auto / | |
| docs/ReleaseNotes.rst | ||
| 196 | Resolve the conflicts. | |
| docs/clang-tidy/checks/bugprone-exception-escape.rst | ||
| 29 | FunctionsThatShouldNotThrow? | |
| 30 | Examples should be copy-pastable. I suppose, one can't use WinMain() verbatim as the value of this option. An example could be: main,WinMain or something like that. | |
| 31 | s/vale/value/ | |
| clang-tidy/bugprone/ExceptionEscapeCheck.cpp | ||
|---|---|---|
| 155–156 | Does this actually catch std::bad_alloc or just any exception called bad_alloc? Should this be a fully qualified type, those defined by the implementation? I suspect this isn't as simple to figure out, because an implementation may be using nested namespaces (inline namespace?) to bring in the bad_alloc type in the std namespace. | |
| docs/ReleaseNotes.rst | ||
| 196–199 | You probably want to fix this... | |
| docs/clang-tidy/checks/bugprone-exception-escape.rst | ||
| 29 | My suggestion here would be something like 'FunctionBlacklist' or 'IgnoreFunctions'. Are these exact names, or regular expressions? Should they be regular expressions? How about those that are in namespaces? You might want to explore being able to configure this similar to the Sanitizer blacklist, which supports function name globs. I can imagine this immediately getting really brittle too. | |
| test/clang-tidy/bugprone-exception-escape.cpp | ||
| 179 | How deep does this go? Say we have a call to a function that's extern which doesn't have 'noexcept' nor a dynamic exception specifier -- do we assume that the call to an extern function may throw? Does that warn? What does the warning look like? Should it warn? How about when you call a function through a function pointer? The documentation should cover these cases and/or more explicitly say in the warning that an exception may throw in a noexcept function (rather than just "function <...> throws"). | |
| test/clang-tidy/bugprone-exception-escape.cpp | ||
|---|---|---|
| 179 | We take the most conservative way here. We assume that a function throws if and only if its body has a throw statement or its header has the (old) throw() exception specification. We do not follow function pointers. | |
| test/clang-tidy/bugprone-exception-escape.cpp | ||
|---|---|---|
| 179 | While i realize it may have more noise, it may be nice to have a more pedantic mode (guarded by an option?). | |
| test/clang-tidy/bugprone-exception-escape.cpp | ||
|---|---|---|
| 179 | This could be an enhancement in the future, yes. | |
| docs/clang-tidy/checks/bugprone-exception-escape.rst | ||
|---|---|---|
| 7 | I don't think the check just finds functions that should not throw exceptions. I suppose, it finds throw statements inside them (or maybe it goes slightly deeper - the documentation should explain this in more detail). See also the comment @dberris left below. | |
| 32 | nit: Double period after throw. | |
| 32 | nit: "an empty string" | |
| test/clang-tidy/bugprone-exception-escape.cpp | ||
| 179 | Please address the original comment here. In particular, the warning message should be clearer (that an exception may be thrown in a function that should not throw exceptions). | |
Comment Actions
I'm really excited by this clang-tidy check and I think it's worth having. I do think there's more work needed to bring it up to a level of quality that would be helpful to more users.
| docs/clang-tidy/checks/bugprone-exception-escape.rst | ||
|---|---|---|
| 7 | excpetion -> exception | |
| test/clang-tidy/bugprone-exception-escape.cpp | ||
| 179 | Can we make the warning more accurate here? Something like: warning: call to 'implicit_int_thrower' may throw an exception and propagate through noexcept function 'indirect_implicit' It would be helpful to diagnose the point at which the exception may be thrown from within the function (if it's an operator, a function call, etc.) that doesn't have exceptions handled. If you can highlight not just the line number but the actual expression in which the uncaught exception may propagate, it would make this warning much better. If you think it's worth it (or if it's feasible), having a FixIt hint to wrap a block of statements where exceptions may propagate in a try { ... } catch (...) { ... } block would turn this warning from a good warning, to a great warning -- potentially something that could be automatically applied by a tool as well. | |
| test/clang-tidy/bugprone-exception-escape.cpp | ||
|---|---|---|
| 179 | I think this is a good future improvement. However, I tried to make the first version as small as possible, and then enhance it incrementally. Even the current code is too big for a Clang-Tidy check. | |
Comment Actions
Looks good with one comment.
| test/clang-tidy/bugprone-exception-escape.cpp | ||
|---|---|---|
| 179 | Could you add a relevant FIXME comment next to the diag() call in the check? | |
You can open namespace clang here instead of this using directive.