Page MenuHomePhabricator

[clang-tidy] Add new check 'bugprone-unhandled-exception-at-new'.
ClosedPublic

Authored by balazske on Feb 22 2021, 7:57 AM.

Diff Detail

Event Timeline

balazske created this revision.Feb 22 2021, 7:57 AM
balazske requested review of this revision.Feb 22 2021, 7:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 22 2021, 7:57 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Eugene.Zelenko added inline comments.
clang-tools-extra/docs/ReleaseNotes.rst
73

Please rebase from trunk.

79

Please indent.

Eugene.Zelenko added a project: Restricted Project.
njames93 added inline comments.Feb 23 2021, 8:37 AM
clang-tools-extra/clang-tidy/bugprone/UnhandledExceptionAtNewCheck.cpp
24–25

This can leak bound nodes If it doesn't match.

clang-tools-extra/clang-tidy/bugprone/UnhandledExceptionAtNewCheck.h
29

I'd suggest this check is only ran when exceptions are enabled.

balazske updated this revision to Diff 326642.Feb 26 2021, 3:20 AM

Rebase, fixes according to review comments.

balazske marked 3 inline comments as done.Feb 26 2021, 3:21 AM
aaron.ballman added inline comments.Wed, Mar 24, 11:41 AM
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-exception-at-new.rst
18
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-exception-at-new.cpp
21

It would be useful to also have a test with a function-try-block to ensure those are handled properly.

144

You have tests for placement new with nothrow_t, but I think other forms of placement new are also very interesting to test as those typically don't throw.

Additionally, perhaps tests where the allocation functions have been replaced by the user (such as a class allocation function)?

balazske added inline comments.Fri, Mar 26, 9:31 AM
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-exception-at-new.cpp
144

I realized that the case of user-defined constructor or allocation function allows to throw any kind of exception. So the check should be improved to handle this case: Not rely on the syntax of new expression, instead check if the called allocation function or the called constructor may throw, and if yes, check what exceptions are possible. What is your opinion, is it a better approach?
(And a function to collect all possible exceptions called from a function is needed, ExceptionAnalyzer seems usable.)

balazske added inline comments.Tue, Mar 30, 3:47 AM
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-exception-at-new.cpp
144

It looks like that the user is free to define custom operator new and any constructor called that may throw any exception. Even in the "nothrow" case it is possible to use a constructor that may throw? If we would analyze every possible throwable exception that may come out from a new-expression, the checker would end up almost in a general checker that checks for uncaught exceptions. At least it is easy to extend.

balazske updated this revision to Diff 334156.Tue, Mar 30, 7:03 AM

Use mayThrow, improve test (user and class-specific cases).

aaron.ballman added inline comments.Wed, Mar 31, 5:47 AM
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-exception-at-new.cpp
144

Not rely on the syntax of new expression, instead check if the called allocation function or the called constructor may throw, and if yes, check what exceptions are possible. What is your opinion, is it a better approach?

I think that's a better approach.

144

Even in the "nothrow" case it is possible to use a constructor that may throw?

Certainly -- the nothrow syntax is specifying that the allocation cannot throw (it either returns a valid pointer to the allocated memory or null), not that the constructor cannot throw if the allocation succeeds.

Is the check intended to care about *allocations* that fail or just exceptions in general around new expressions? If it's allocations that fail, then I wouldn't worry about the ctor's exception specification. If exceptions in general, then I agree that we're talking about a general checker for all uncaught exceptions in some ways.

balazske updated this revision to Diff 335505.Tue, Apr 6, 7:14 AM

Removed check of possible exceptions from constructor call.

Rename the check to "unhandled-bad_alloc" (or similar")? And the error message to " missing exception handler 'std::bad_alloc' "?

balazske updated this revision to Diff 335767.Wed, Apr 7, 2:47 AM

Fix a crash, update documentation.

Ping.
The check now handles only check of allocation failure at new.

aaron.ballman added inline comments.Mon, Apr 12, 8:31 AM
clang-tools-extra/clang-tidy/bugprone/UnhandledExceptionAtNewCheck.cpp
29–31

I think this should move above the call to InnertMatcher.matches() so that the inner matcher doesn't have to worry quite as much about getting null types.

clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-exception-at-new.rst
8

This isn't quite accurate -- if the exception handler is missing or doesn't handle either std::bad_alloc or std::exception.

18
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-exception-at-new.cpp
21

Another interesting test case would be when the user subclasses std::bad_alloc and throws errors of the subclass type from the allocation function which are in/correctly caught.

21

Still missing this test -- you should add one like:

void func() try {
  int *i = new int;
} catch (const std::bad_alloc &) {
}
balazske updated this revision to Diff 337078.Tue, Apr 13, 2:42 AM
balazske marked 8 inline comments as done.

Rebase, changed documentation, small fix in the code, more tests added.

clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-exception-at-new.rst
8

I restructure the text, but do not want to mention std::exception because it is a base class (handling of std::exception means handling of std::bad_alloc), and then why not mention generic exception handler (catch(...)). I think this text is for introduction only, should not be totally precise.

balazske added inline comments.Tue, Apr 13, 3:49 AM
clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-exception-at-new.cpp
21

In this case the check must find every (visible) subclass of std::bad_alloc check if there is a catch block for every case. Even then a custom allocation function may throw other classes (not derived from std::bad_alloc) or classes (may be inherited from std::bad_alloc but not visible in the TU) that are not checked. So this is still a partial solution, probably not better than the rule that a handler for bad_alloc (or more generic) should exist.

aaron.ballman accepted this revision.Tue, Apr 13, 1:04 PM

LGTM!

clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-exception-at-new.rst
8

The restructured text is good -- I just wanted the information *somewhere* so that users weren't surprised by the behavior.

clang-tools-extra/test/clang-tidy/checkers/bugprone-unhandled-exception-at-new.cpp
21

So this is still a partial solution, probably not better than the rule that a handler for bad_alloc (or more generic) should exist.

Okay, that's fair -- we can always address it in follow-up if it turns out to be an issue in practice.

This revision is now accepted and ready to land.Tue, Apr 13, 1:04 PM
balazske updated this revision to Diff 337344.Tue, Apr 13, 11:33 PM

Add another test.

This revision was landed with ongoing or failed builds.Wed, Apr 14, 12:24 AM
This revision was automatically updated to reflect the committed changes.
dyung added a subscriber: dyung.Wed, Apr 14, 12:36 AM

This appears to be failing on the PS4 linux bot likely due to the PS4 target has exceptions disabled by default. Can you take a look?

https://lab.llvm.org/buildbot/#/builders/139/builds/2441