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