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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/openmp/ExceptionEscapeCheck.cpp:48
+    return;
+  // Similarly, if C++ Exceptions are not enabled, nothing to do.
+  if (!getLangOpts().CPlusPlus || !getLangOpts().CXXExceptions)
----------------
lebedev.ri wrote:
> 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.
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.


================
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());
----------------
lebedev.ri wrote:
> 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.
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`


================
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
----------------
lebedev.ri wrote:
> 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
> 
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.


================
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.
----------------
lebedev.ri wrote:
> aaron.ballman wrote:
> > Which is it -- undefined or implementation-defined?
> I'm unable to anything specific in the spec, let's call this `Unspecified`.
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.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59466