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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
