This is an archive of the discontinued LLVM Phabricator instance.

[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

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.

Eugene.Zelenko set the repository for this revision to rL LLVM.
Eugene.Zelenko added a project: Restricted Project.

How is that compared to https://reviews.llvm.org/D19201 and the clang patch mentioned in this patch?

Docs fixed according to the comments.

How is that compared to https://reviews.llvm.org/D19201 and the clang patch mentioned in this patch?

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.

Check added to the Release Notes.

Ignoring bad_alloc.

aaron.ballman edited edge metadata.Jun 1 2017, 9:59 AM

How is that compared to https://reviews.llvm.org/D19201 and the clang patch mentioned in this patch?

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.

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.

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.

I suppose that the frontend check will not travarse the call-graph just check direct throws. Should we only check indirect throws then?

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.

I suppose that the frontend check will not travarse the call-graph just check direct throws. Should we only check indirect throws then?

The check in D33333 is using a CFG, not just checking direct throws.

The check in D33333 is using a CFG, not just checking direct throws.

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.

The check in D33333 is using a CFG, not just checking direct throws.

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.

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.

jyu2 added a subscriber: jyu2.EditedJul 13 2017, 8:53 AM

The check in D33333 is using a CFG, not just checking direct throws.

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.

The check in D33333 is using a CFG, not just checking direct throws.

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.

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.

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.

Test changed. I made some bad throws reachable, but the frontend check still does detects them.

jyu2 added a comment.Aug 1 2017, 8:35 AM

Test changed. I made some bad throws reachable, but the frontend check still does detects them.

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

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.

I agree that we should not spend too much effort on making warnings from the compiler and tidy disjunct.

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.

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.

JVApen added a subscriber: JVApen.Nov 23 2017, 11:02 PM

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)

We take the conservative approach there: if a function has no throw specifier we handle it as it would have a noexcept specifier.

alexfh requested changes to this revision.Mar 14 2018, 8:17 AM

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.

This revision now requires changes to proceed.Mar 14 2018, 8:17 AM

Escaping exception is certainly a bug, so it should go into bugprone then.

Moved to bugprone.

alexfh added inline comments.Mar 23 2018, 5:43 AM
docs/clang-tidy/checks/misc-exception-escape.rst
1 ↗(On Diff #139583)

This file should be renamed as well.

Eugene.Zelenko added inline comments.Mar 23 2018, 6:55 AM
docs/ReleaseNotes.rst
68 ↗(On Diff #139583)

Please fix link. See other checks as example.

alexfh requested changes to this revision.Mar 23 2018, 7:32 AM
alexfh added inline comments.
clang-tidy/bugprone/ExceptionEscapeCheck.cpp
24–25 ↗(On Diff #139583)
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
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/

28 ↗(On Diff #145155)

FunctionsThatShouldNotThrow?

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.