[PATCH] D59466: [clang-tidy] openmp-exception-escape - a new check

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.


================
Comment at: clang-tidy/openmp/ExceptionEscapeCheck.cpp:53
+  Finder->addMatcher(ompExecutableDirective(
+                         unless(isStandaloneDirective()),
+                         hasStructuredBlock(stmt().bind("structured-block")))
----------------
gribozavr wrote:
> lebedev.ri wrote:
> > gribozavr wrote:
> > > Do we need the `unless`?  `hasStructuredBlock()` just won't match standalone directives.
> > *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.
> > 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.
What i'm saying is, yes, it won't match, but i think this reads better from OpenMP standpoint.


================
Comment at: clang-tidy/openmp/ExceptionEscapeCheck.cpp:32
+  llvm::StringSet<> IgnoredExceptions;
+  StringRef(RawIgnoredExceptions).split(IgnoredExceptionsVec, ",", -1, false);
+  IgnoredExceptions.insert(IgnoredExceptionsVec.begin(),
----------------
aaron.ballman wrote:
> Do you need to trim any of the split strings?
Hm, i guess i might aswell..


================
Comment at: clang-tidy/openmp/ExceptionEscapeCheck.cpp:48
+    return;
+  // Similarly, if C++ Exceptions are not enabled, nothing to do.
+  if (!getLangOpts().CPlusPlus || !getLangOpts().CXXExceptions)
----------------
aaron.ballman wrote:
> Do you have to worry about SEH exceptions in C for this functionality?
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.


================
Comment at: clang-tidy/openmp/ExceptionEscapeCheck.cpp:75-76
+  diag(Directive->getBeginLoc(),
+       "An exception thrown inside of the OpenMP '%0' region is not caught in "
+       "that same region.")
+      << getOpenMPDirectiveName(Directive->getDirectiveKind());
----------------
aaron.ballman wrote:
> Clang-tidy diagnostics are not complete sentences -- please make this horribly ungrammatical. ;-)
Is `s/An/an` sufficient? :)
I'm not sure what i can drop here without loosing context.


================
Comment at: docs/clang-tidy/checks/openmp-exception-escape.rst:10-11
+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
----------------
aaron.ballman wrote:
> Does this mean setjmp/longjmp out of the block is also a dangerous activity? What about gotos?
>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



================
Comment at: docs/clang-tidy/checks/openmp-exception-escape.rst:13
+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.
----------------
aaron.ballman wrote:
> Which is it -- undefined or implementation-defined?
I'm unable to anything specific in the spec, let's call this `Unspecified`.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59466/new/

https://reviews.llvm.org/D59466