Page MenuHomePhabricator

[clang-tidy] openmp-exception-escape - a new check
ClosedPublic

Authored by lebedev.ri on Mar 16 2019, 1:52 PM.

Details

Summary

Finally, we are here!

Analyzes OpenMP Structured Blocks and checks that no exception escapes
out of the Structured Block it was thrown in.

As per the OpenMP specification, structured block is an executable statement,
possibly compound, with a single entry at the top and a single exit at the
bottom. Which means, `throw` may not be used to to 'exit' out of the
structured block. If an exception is not caught in the same structured block
it was thrown in, the behaviour is undefined / implementation defined,
the program will likely terminate.

Diff Detail

Event Timeline

lebedev.ri created this revision.Mar 16 2019, 1:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2019, 1:52 PM

Great work! Thank you! I only have minor comment: did you consider moving the refactoring of ExceptionAnalyzer into a separate (prerequisite) patch?

Great work! Thank you! I only have minor comment: did you consider moving the refactoring of ExceptionAnalyzer into a separate (prerequisite) patch?

Yes, absolutely, that will be separate patch.
I just wanted to finally post the check to show how it finally (*sigh*) comes together in the end.

lebedev.ri added a reviewer: gribozavr.

Rebased, NFC.
Split base ExceptionEscapeCheck refactoring into D59650.

Rebased for D59650 changes, NFC.

gribozavr marked an inline comment as done.Mar 22 2019, 5:47 AM
gribozavr added inline comments.
clang-tidy/openmp/ExceptionEscapeCheck.cpp
54

Do we need the unless? hasStructuredBlock() just won't match standalone directives.

66

soe => some

70

appear to => have been proven to

73

+1

docs/clang-tidy/checks/openmp-exception-escape.rst
22

IgnoredExceptionTypes?

test/clang-tidy/bugprone-exception-escape-openmp.cpp
17

Shouldn't the function be marked with noexcept for bugprone-exception-escape to catch it? It does not know about OpenMP.

Are you suggesting that bugprone-exception-escape should subsume your new OpenMP-specific check?

lebedev.ri marked 6 inline comments as done.

Address some nits.

clang-tidy/openmp/ExceptionEscapeCheck.cpp
54

*need*? no.
But it makes zero sense semantically to look for structured block
without first establishing that it has one.
Sure, we now check that twice (here, and in hasStructuredBlock()), but that is up to optimizer.

The fact that hasStructuredBlock() workarounds the assert in
getStructuredBlock() is only to avoid clang-query crashes,
it is spelled as much in the docs.

docs/clang-tidy/checks/openmp-exception-escape.rst
22

What do you mean?
In the check it's Options.get("IgnoredExceptions", "").
That is identical to the check in bugprone.

test/clang-tidy/bugprone-exception-escape-openmp.cpp
17

Shouldn't the function be marked with noexcept for bugprone-exception-escape to catch it?

Right. I meant noexcept (or did i drop it and forgot to put it back?).
But that changed nothing here, this is still an XFAIL.

It does not know about OpenMP.
Are you suggesting that bugprone-exception-escape should subsume your new OpenMP-specific check?

Only the ; is the structured-block here, so thrower() call *should* be caught by bugprone-exception-escape.

gribozavr accepted this revision.Mar 22 2019, 6:44 AM
gribozavr marked an inline comment as done.
gribozavr added inline comments.
clang-tidy/openmp/ExceptionEscapeCheck.cpp
54

But it makes zero sense semantically to look for structured block without first establishing that it has one.

IDK, in my mind it makes sense. "Does a standalone directive have a structured block? No." is a coherent logical chain.

test/clang-tidy/bugprone-exception-escape-openmp.cpp
17

Ah, I see.

This revision is now accepted and ready to land.Mar 22 2019, 6:44 AM
aaron.ballman added inline comments.Mar 22 2019, 7:14 AM
clang-tidy/openmp/ExceptionEscapeCheck.cpp
33

Do you need to trim any of the split strings?

49

Do you have to worry about SEH exceptions in C for this functionality?

73

Missing a full stop at the end of the comment.

76–77

Clang-tidy diagnostics are not complete sentences -- please make this horribly ungrammatical. ;-)

docs/clang-tidy/checks/openmp-exception-escape.rst
10

, structured block -> , a structured block

11–12

Does this mean setjmp/longjmp out of the block is also a dangerous activity? What about gotos?

14

Which is it -- undefined or implementation-defined?

24

Comma separated -> Comma-separated

lebedev.ri marked 15 inline comments as done.

Addressed some nits.

lebedev.ri added inline comments.Mar 22 2019, 8:58 AM
clang-tidy/openmp/ExceptionEscapeCheck.cpp
33

Hm, i guess i might aswell..

49

I don't have a clue about SEH to be honest.
(This check comes form the bugprone-exception-escape check)
Likely the ExceptionAnalyzer would need to be upgraded first.

54

What i'm saying is, yes, it won't match, but i think this reads better from OpenMP standpoint.

76–77

Is s/An/an sufficient? :)
I'm not sure what i can drop here without loosing context.

docs/clang-tidy/checks/openmp-exception-escape.rst
11–12

From D59214:

https://www.openmp.org/wp-content/uploads/OpenMP-API-Specification-5.0.pdf, page 3:

structured block

For C/C++, an executable statement, possibly compound, with a single entry at the
top and a single exit at the bottom, or an OpenMP construct.

COMMENT: See Section 2.1 on page 38 for restrictions on structured
blocks.
2.1 Directive Format

Some executable directives include a structured block. A structured block:
• may contain infinite loops where the point of exit is never reached;
• may halt due to an IEEE exception;
• may contain calls to exit(), _Exit(), quick_exit(), abort() or functions with a
_Noreturn specifier (in C) or a noreturn attribute (in C/C++);
• may be an expression statement, iteration statement, selection statement, or try block, provided
that the corresponding compound statement obtained by enclosing it in { and } would be a
structured block; and

Restrictions
Restrictions to structured blocks are as follows:
• Entry to a structured block must not be the result of a branch.
• The point of exit cannot be a branch out of the structured block.
C / C++
• The point of entry to a structured block must not be a call to setjmp().
• longjmp() and throw() must not violate the entry/exit criteria.

So yeah, setjmp/longjmp are also suspects.
Maybe not so much goto though https://godbolt.org/z/GZMugf https://godbolt.org/z/WUYcYD

14

I'm unable to anything specific in the spec, let's call this Unspecified.

aaron.ballman added inline comments.Mar 22 2019, 9:04 AM
clang-tidy/openmp/ExceptionEscapeCheck.cpp
49

Hmm, that may be worth looking into, but can be done in a follow-up patch. SEH likely causes problems here for you as well.

76–77

You need to remove the full stop from the end of the sentence as well. How about: exception through inside OpenMP '%0' region is not caught within the region

docs/clang-tidy/checks/openmp-exception-escape.rst
11–12

This might be another future thing for the patch to handle. May be worth adding some FIXME comments to the code detailing the extensions we want to see.

14

Unspecified also has special meaning -- you may want to double-check what the precise behavior is, but I suspect it is undefined behavior to violate these constraints.

lebedev.ri marked 4 inline comments as done.

Last few nits.

docs/clang-tidy/checks/openmp-exception-escape.rst
11–12

I think those might be two new checks, though i don't really expect to look into that.
I have added FIXME here though.
Also, cert-err52-cpp check already exists.

aaron.ballman accepted this revision.Mar 22 2019, 12:12 PM

Aside from a docs nit, LGTM!

docs/clang-tidy/checks/openmp-exception-escape.rst
14

I would remove the bit about the program being likely to terminate -- we have no idea what it will do if it hits UB, so better to just let those words stand on their own.

Aside from a docs nit, LGTM!

Thank you for the review!

This revision was automatically updated to reflect the committed changes.