Page MenuHomePhabricator

[clang-tidy] Exception Escape Checker
ClosedPublic

Authored by baloghadamsoftware on May 24 2017, 11:04 PM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
alexfh added inline comments.Mar 23 2018, 7:32 AM
clang-tidy/bugprone/ExceptionEscapeCheck.cpp
24–25 ↗(On Diff #139583)

If there's no need to pass nullptr to these functions, the arguments can be const references.

115 ↗(On Diff #139583)

No else after return.

142 ↗(On Diff #139583)

Don't use getNameAsString when not necessary. if (TD->getDeclName().isIdentifier() && TD->getName() == "bad_alloc") will work as good, but without unnecessary string copies.

This revision now requires changes to proceed.Mar 23 2018, 7:32 AM

Updated according to the comments.

baloghadamsoftware marked 4 inline comments as done.Mar 28 2018, 8:33 AM
baloghadamsoftware added inline comments.
clang-tidy/bugprone/ExceptionEscapeCheck.cpp
24–25 ↗(On Diff #139583)

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 ↗(On Diff #140079)

.html is not necessary.

,html removed from release notes.

baloghadamsoftware marked an inline comment as done.Mar 28 2018, 12:15 PM
alexfh requested changes to this revision.Apr 5 2018, 5:59 AM
alexfh added inline comments.
clang-tidy/bugprone/ExceptionEscapeCheck.cpp
100 ↗(On Diff #140117)

make anonymous namespaces as small as possible, and only use them for class declarations

http://llvm.org/docs/CodingStandards.html#anonymous-namespaces

105 ↗(On Diff #140117)

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);
}
110 ↗(On Diff #140117)

Please use the concrete type here, since it's not obvious from the context.

119 ↗(On Diff #140117)

Please use the concrete type here.

120 ↗(On Diff #140117)

Ex.getTypePtrOrNull() / Ex.getTypePtr() would be easier to understand here.

150 ↗(On Diff #140117)

Please use the concrete type here and below. http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable says

Don’t “almost always” use auto, but do use auto with initializers like cast<Foo>(...) or other places where the type is already obvious from the context.

166 ↗(On Diff #140117)
  1. iterator being a pointer is an implementation detail of SmallVector
  2. use llvm::remove_if, it has a range based interface
176 ↗(On Diff #140117)

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.

This revision now requires changes to proceed.Apr 5 2018, 5:59 AM

Thank you for your thorough review, I will do the fixes next week, after the Euro LLVM.

Updated according to the comments.

baloghadamsoftware marked 8 inline comments as done.Apr 26 2018, 5:56 AM
alexfh requested changes to this revision.May 3 2018, 9:14 AM

It looks like you've missed some comments or uploaded a wrong patch.

clang-tidy/bugprone/ExceptionEscapeCheck.cpp
105 ↗(On Diff #140117)

Still not addressed?

110 ↗(On Diff #140117)

Still not addressed?

119 ↗(On Diff #140117)

Still not addressed?

120 ↗(On Diff #140117)

Still not addressed?

This revision now requires changes to proceed.May 3 2018, 9:14 AM

It looks like you've missed some comments or uploaded a wrong patch.

The latter. Now I retried to upload the correct patch.

How about tests for functions with conditional noexcept?

docs/clang-tidy/checks/bugprone-exception-escape.rst
8–9 ↗(On Diff #145155)

Are copy constructors and assignment operators not supposed to throw? Or did you mean *move* constructors?

28 ↗(On Diff #145155)

EnabledFunctions but they *should not* throw?

baloghadamsoftware marked an inline comment as done.May 4 2018, 4:35 AM
baloghadamsoftware added inline comments.
docs/clang-tidy/checks/bugprone-exception-escape.rst
8–9 ↗(On Diff #145155)

You are right. In the description below it is correct, but here there was a typo.

28 ↗(On Diff #145155)

Maybe we should come up with a better name for this option. I just took this from our company internal check.

baloghadamsoftware marked an inline comment as done.

New test added.

alexfh requested changes to this revision.May 4 2018, 7:10 AM

Thank you for the updates. A few more comments.

clang-tidy/bugprone/ExceptionEscapeCheck.cpp
17 ↗(On Diff #145174)

You can open namespace clang here instead of this using directive.

22 ↗(On Diff #145174)

There's llvm::StringSet<> in llvm/ADT/StringSet.h.

25 ↗(On Diff #145174)

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.

178 ↗(On Diff #145174)

iterator being a pointer is an implementation detail of SmallVector. s/auto \*/auto /

150 ↗(On Diff #140117)

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.

docs/ReleaseNotes.rst
230 ↗(On Diff #145174)

Resolve the conflicts.

docs/clang-tidy/checks/bugprone-exception-escape.rst
28 ↗(On Diff #145155)

FunctionsThatShouldNotThrow?

29 ↗(On Diff #145174)

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.

30 ↗(On Diff #145174)

s/vale/value/

This revision now requires changes to proceed.May 4 2018, 7:10 AM
dberris added inline comments.May 4 2018, 7:14 AM
clang-tidy/bugprone/ExceptionEscapeCheck.cpp
154–155 ↗(On Diff #145174)

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
230–233 ↗(On Diff #145174)

You probably want to fix this...

docs/clang-tidy/checks/bugprone-exception-escape.rst
28 ↗(On Diff #145155)

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
178 ↗(On Diff #145174)

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").

Updated according to the comments.

baloghadamsoftware marked 10 inline comments as done.May 7 2018, 6:07 AM
baloghadamsoftware added inline comments.
test/clang-tidy/bugprone-exception-escape.cpp
178 ↗(On Diff #145174)

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.

lebedev.ri added inline comments.
test/clang-tidy/bugprone-exception-escape.cpp
178 ↗(On Diff #145174)

While i realize it may have more noise, it may be nice to have a more pedantic mode (guarded by an option?).
E.g. noexcept propagation, much like const on methods propagation.
Or at least a test that shows that it does not happen, unless i simply did not notice it :)

test/clang-tidy/bugprone-exception-escape.cpp
178 ↗(On Diff #145174)

This could be an enhancement in the future, yes.

alexfh requested changes to this revision.Jun 6 2018, 8:12 AM
alexfh added inline comments.
docs/clang-tidy/checks/bugprone-exception-escape.rst
6 ↗(On Diff #145452)

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.

31 ↗(On Diff #145452)

nit: Double period after throw.

31 ↗(On Diff #145452)

nit: "an empty string"
Same below.

test/clang-tidy/bugprone-exception-escape.cpp
178 ↗(On Diff #145174)

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).

This revision now requires changes to proceed.Jun 6 2018, 8:12 AM

New warning message, more detailed docs.

baloghadamsoftware marked 5 inline comments as done.Jun 18 2018, 1:18 AM

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
6 ↗(On Diff #151666)

excpetion -> exception

test/clang-tidy/bugprone-exception-escape.cpp
178 ↗(On Diff #151666)

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
178 ↗(On Diff #151666)

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.

baloghadamsoftware marked an inline comment as done.Jun 19 2018, 2:28 AM

Any news regarding this?

alexfh accepted this revision.Jul 11 2018, 8:57 AM

Looks good with one comment.

test/clang-tidy/bugprone-exception-escape.cpp
178 ↗(On Diff #151666)

Could you add a relevant FIXME comment next to the diag() call in the check?

This revision is now accepted and ready to land.Jul 11 2018, 8:57 AM
This revision was automatically updated to reflect the committed changes.