Page MenuHomePhabricator

[clang-tidy] refactor bugprone-exception-escape analysis into class
ClosedPublic

Authored by JonasToth on Jan 23 2019, 7:14 AM.

Details

Summary

The check bugprone-exception-escape does an AST-based analysis to determine
if a function might throw an exception and warns based on that information.
The analysis part is refactored into a standalone class similiar to
ExprMutAnalyzer that is generally useful.
I intent to use that class in a new check to automatically introduce noexcept
if possible.

bugprone-exception-escape uses the consistent semicolon-separated list for
configuration now as well.

Diff Detail

Repository
rL LLVM

Event Timeline

JonasToth created this revision.Jan 23 2019, 7:14 AM
JonasToth added a project: Restricted Project.Jan 23 2019, 7:16 AM

Two passing-by remarks:

  1. I would *love* for this check to produce less cryptic reports :) It currently does not output any details whatsoever. It's really unhelpful.
  2. It would be great for it to be generalized to not only detect the escaping of exceptions out of functions, but out of "statements" too. Namely, i would love some better handling of openmp directives. I might look into that later.

Two passing-by remarks:

  1. I would *love* for this check to produce less cryptic reports :) It currently does not output any details whatsoever. It's really unhelpful.

I would do that in a second refactoring. As it is now possible to have state within the analysis it should be possible to extract a path that lead to the exception. I think the 'highest' throwing function should be noted as the source.

void indirect_throw() { call_something_throwing(); }
void diagnose() noexcept {
// warning: this function can throw

  indirect_throw();
  // note: this function is not safe to call
}
  1. It would be great for it to be generalized to not only detect the escaping of exceptions out of functions, but out of "statements" too. Namely, i would love some better handling of openmp directives. I might look into that later.

The internals can handle Stmt already and do traverse all statements of a FunctionDecl, so it should be possible to expose that.
Can OpenMP directives throw?

I think making this class and its interface is the first step to both features, incremental improvements are then easier to do.

JonasToth updated this revision to Diff 183099.Jan 23 2019, 7:34 AM
JonasToth removed a subscriber: lebedev.ri.
  • adjust doc to say semicolon separated list instead of comma
  1. It would be great for it to be generalized to not only detect the escaping of exceptions out of functions, but out of "statements" too. Namely, i would love some better handling of openmp directives. I might look into that later.

The internals can handle Stmt already and do traverse all statements of a FunctionDecl, so it should be possible to expose that.
Can OpenMP directives throw?

The other way around, it's the same problem as with exceptions escaping noexcept functions,
but just with narrower scope - a statement - as opposed to whole function

I think making this class and its interface is the first step to both features, incremental improvements are then easier to do.

Yep, don't want to drag anything sideways, just voicing some thoughts.

  • [Fix] make class name correct
lebedev.ri added inline comments.Jan 23 2019, 10:48 AM
clang-tidy/bugprone/ExceptionEscapeCheck.cpp
35 ↗(On Diff #183131)

Does it also accept ',' delimitter?
If not, i'm not sure how good of an idea it is to break existing configs..

clang-tidy/utils/ExceptionAnalyzer.cpp
1 ↗(On Diff #183131)

Standard licence blurb missing.

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

Hm, this may be a breaking change, and the check existed in 7.0 already.

JonasToth marked 3 inline comments as done.Jan 23 2019, 10:57 AM
JonasToth added inline comments.
clang-tidy/bugprone/ExceptionEscapeCheck.cpp
35 ↗(On Diff #183131)

No it doesn't. It is a breaking change but at the same time it is inconsistent and duplicated functionality.
Should be mentioned in release notes, that for sure.

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

See above.

JonasToth marked an inline comment as done.
  • add license to exceptionanalyzer
lebedev.ri added inline comments.Jan 24 2019, 2:04 AM
clang-tidy/bugprone/ExceptionEscapeCheck.cpp
35 ↗(On Diff #183131)

The thing is, if said list was set (as in user-specified to non-empty), it will be impossible
to use that same .clang-tidy config with clang-tidy <=8 and trunk.
I'm personally not affected, but if i was, i wouldn't think this is a great idea..

Can you at least make the breakage less crippling by also renaming the config field?

JonasToth marked 3 inline comments as done.Jan 24 2019, 2:48 AM
JonasToth added inline comments.
clang-tidy/bugprone/ExceptionEscapeCheck.cpp
35 ↗(On Diff #183131)

Oh thats true, i didnt consider the config files. Maybe I just leave it as is.

lebedev.ri added inline comments.Jan 24 2019, 4:23 AM
clang-tidy/bugprone/ExceptionEscapeCheck.cpp
35 ↗(On Diff #183131)

I understand the cleanup and the inconsistency and duplicated functionality,
but maybe regardless of the fix, it should be addressed in a separate review?

To me, modulo the "IgnoredExceptions" change, this seems like a straight-forward refactoring.

clang-tidy/utils/ExceptionAnalyzer.h
25 ↗(On Diff #183134)

What is the test plan for this utility class, BTW?
Ensuring that the main user (bugprone-exception-escape) has sufficiently full test coverage?

JonasToth marked an inline comment as done.
  • revert the option configuration change for comma/semicolon
JonasToth marked 2 inline comments as done.Jan 25 2019, 12:27 AM

Reverted the changes. Maybe the comma thingie will just stay as is ;)

clang-tidy/utils/ExceptionAnalyzer.h
25 ↗(On Diff #183134)

Simplify code first, generate a trace, use it in other context (modernize-patch you have seen already).

JonasToth marked 2 inline comments as done.Jan 25 2019, 12:28 AM
  • revert doc change as well

Hello! I am glad to see that this check gets improved by the community. I also think a "modernize" check which marks functions with noexcept is also useful.

As for the comma or semicolon, I think semicolon would be better. I see that this part is reverted now, but is there any reason for using std::string instead of llvm::StringRef? I always thought this latter is the standard for LLVM code wherever possible.

Hello! I am glad to see that this check gets improved by the community. I also think a "modernize" check which marks functions with noexcept is also useful.

Great to hear!

As for the comma or semicolon, I think semicolon would be better. I see that this part is reverted now, but is there any reason for using std::string instead of llvm::StringRef? I always thought this latter is the standard for LLVM code wherever possible.

Which part to you mean? The original semicolon-splitting?
I think there the issue is, that StringRef is just a handle, but if you split a big string into smaller ones you want the result to be owning its own memory instead of just pointing to a span in the original full string.
Even though that would be correct in some cases it has a subtle dangling-bug if you work further with the result of the split or the underlying string disappears for some reason.
Otherwise StringRef is definitly the goto-class for string operations (that dont change underlying memory).

For now the comma-vs-semicolon thingie will stay as is, because its a breaking change in the configurations and the gain for switching to semicolon is small, whereas the pain to figuring out that changed is big.

@baloghadamsoftware Do you see any problems with the refactoring, especially the new FunctionDecl*-Caching is not exactly a refactoring. If you have any thoughts on that would be great to hear!

lebedev.ri accepted this revision.Jan 30 2019, 8:30 AM

Looked this over once more. To me this looks pretty straight-forward.
The caching seems ok, too. As long as ExceptionAnalyzer does not outlive
it's TU, everything should be fine.

@baloghadamsoftware - thoughts?

clang-tidy/bugprone/ExceptionEscapeCheck.cpp
73–81 ↗(On Diff #183490)

I think you could do early-exit.
Also, if you de-split the string, clang-format might produce better formatting.

This revision is now accepted and ready to land.Jan 30 2019, 8:30 AM

@baloghadamsoftware Do you see any problems with the refactoring, especially the new FunctionDecl*-Caching is not exactly a refactoring. If you have any thoughts on that would be great to hear!

Yes, it looks a good improvement, I expect another huge speedup. (We had a previous huge speedup after I optimized the matcher expression in 7.0.1 because in 7.0.0 it seemed to hang, it was really bad).

clang-tidy/utils/ExceptionAnalyzer.h
23 ↗(On Diff #183490)

std::bad_alloc is hardwired to ignore, so I think this comment should be rephrased.

@baloghadamsoftware Do you see any problems with the refactoring, especially the new FunctionDecl*-Caching is not exactly a refactoring. If you have any thoughts on that would be great to hear!

Yes, it looks a good improvement, I expect another huge speedup. (We had a previous huge speedup after I optimized the matcher expression in 7.0.1 because in 7.0.0 it seemed to hang, it was really bad).

I revisited the reproducer for that bug and there was no bigger performance gain with the new version in bugprone-exception-escape. But it didnt get worse either :)

JonasToth marked an inline comment as done.Jan 31 2019, 2:40 AM
JonasToth updated this revision to Diff 184475.Jan 31 2019, 2:41 AM
  • [Misc] rewrite parts of a comment that were unprecise.
This revision was automatically updated to reflect the committed changes.