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

[lebedev.ri]

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.

[baloghadamsoftware]

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

[lebedev.ri]

In D59466#1434841, @baloghadamsoftware wrote:

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]

Rebased, NFC.
Split base ExceptionEscapeCheck refactoring into D59650.

[lebedev.ri]

Rebased for D59650 changes, NFC.

[gribozavr]

clang-tidy/openmp/ExceptionEscapeCheck.cpp

53

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

65

soe => some

69

appear to => have been proven to

72

+1

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

21

IgnoredExceptionTypes?

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

16

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]

Address some nits.

clang-tidy/openmp/ExceptionEscapeCheck.cpp

53

*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

21

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

16

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]

clang-tidy/openmp/ExceptionEscapeCheck.cpp

53

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

16

Ah, I see.

[aaron.ballman]

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]

Addressed some nits.

[lebedev.ri]

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.

53

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]

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]

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, [[ https://clang.llvm.org/extra/clang-tidy/checks/cert-err52-cpp.html | cert-err52-cpp ]] check already exists.

[aaron.ballman]

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.

[lebedev.ri]

In D59466#1439880, @aaron.ballman wrote:

Aside from a docs nit, LGTM!

Thank you for the review!

clang-tidy/openmp/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyOpenMPModule
+  ExceptionEscapeCheck.cpp
   OpenMPTidyModule.cpp
   UseDefaultNoneCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangTidy
+  clangTidyUtils
   )

clang-tidy/openmp/ExceptionEscapeCheck.h

+//===--- ExceptionEscapeCheck.h - clang-tidy --------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OPENMP_EXCEPTIONESCAPECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OPENMP_EXCEPTIONESCAPECHECK_H
+
+#include "../ClangTidy.h"
+#include "../utils/ExceptionAnalyzer.h"
+
+namespace clang {
+namespace tidy {
+namespace openmp {
+
+/// Analyzes OpenMP Structured Blocks and checks that no exception escapes
+/// out of the Structured Block it was thrown in.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/openmp-exception-escape.html
+class ExceptionEscapeCheck : public ClangTidyCheck {
+public:
+  ExceptionEscapeCheck(StringRef Name, ClangTidyContext *Context);
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  std::string RawIgnoredExceptions;
+
+  utils::ExceptionAnalyzer Tracer;
+};
+
+} // namespace openmp
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OPENMP_EXCEPTIONESCAPECHECK_H

clang-tidy/openmp/ExceptionEscapeCheck.cpp

+//===--- ExceptionEscapeCheck.cpp - clang-tidy ----------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "ExceptionEscapeCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/OpenMPClause.h"
+#include "clang/AST/Stmt.h"
+#include "clang/AST/StmtOpenMP.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchersMacros.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace openmp {
+
+ExceptionEscapeCheck::ExceptionEscapeCheck(StringRef Name,
+                                           ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      RawIgnoredExceptions(Options.get("IgnoredExceptions", "")) {
+  llvm::SmallVector<StringRef, 8> FunctionsThatShouldNotThrowVec,
+      IgnoredExceptionsVec;
+
+  llvm::StringSet<> IgnoredExceptions;
+  StringRef(RawIgnoredExceptions).split(IgnoredExceptionsVec, ",", -1, false);
+  IgnoredExceptions.insert(IgnoredExceptionsVec.begin(),
+                           IgnoredExceptionsVec.end());
+  Tracer.ignoreExceptions(std::move(IgnoredExceptions));
+  Tracer.ignoreBadAlloc(true);
+}
+
+void ExceptionEscapeCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "IgnoredExceptions", RawIgnoredExceptions);
+}
+
+void ExceptionEscapeCheck::registerMatchers(MatchFinder *Finder) {
+  // Don't register the check if OpenMP is not enabled; the OpenMP pragmas are
+  // completely ignored then, so no OpenMP entires will be present in the AST.
+  if (!getLangOpts().OpenMP)
+    return;
+  // Similarly, if C++ Exceptions are not enabled, nothing to do.
+  if (!getLangOpts().CPlusPlus || !getLangOpts().CXXExceptions)
+    return;
+
+  Finder->addMatcher(ompExecutableDirective(
+                         unless(isStandaloneDirective()),
+                         hasStructuredBlock(stmt().bind("structured-block")))
+                         .bind("directive"),
+                     this);
+}
+
+void ExceptionEscapeCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Directive =
+      Result.Nodes.getNodeAs<OMPExecutableDirective>("directive");
+  assert(Directive && "Expected to match some OpenMP Executable directive.");
+  const auto *StructuredBlock =
+      Result.Nodes.getNodeAs<Stmt>("structured-block");
+  assert(StructuredBlock && "Expected to get soe OpenMP Structured Block.");
+
+  if (Tracer.analyze(StructuredBlock).getBehaviour() !=
+      utils::ExceptionAnalyzer::State::Throwing)
+    return; // No exceptions appear to escape out of the structured block.
+
+  // FIXME: We should provide more information about the exact location where
+  // the exception is thrown, maybe the full path the exception escapes
+
+  diag(Directive->getBeginLoc(),
+       "An exception thrown inside of the OpenMP '%0' region is not caught in "
+       "that same region.")
+      << getOpenMPDirectiveName(Directive->getDirectiveKind());
+}
+
+} // namespace openmp
+} // namespace tidy
+} // namespace clang

clang-tidy/openmp/OpenMPTidyModule.cpp

 //===--- OpenMPTidyModule.cpp - clang-tidy--------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "ExceptionEscapeCheck.h"
 #include "UseDefaultNoneCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace openmp {
 
 /// This module is for OpenMP-specific checks.
 class OpenMPModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<ExceptionEscapeCheck>(
+        "openmp-exception-escape");
     CheckFactories.registerCheck<UseDefaultNoneCheck>(
         "openmp-use-default-none");
   }
 };
 
 // Register the OpenMPTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<OpenMPModule>
     X("openmp-module", "Adds OpenMP-specific checks.");

clang-tidy/utils/ExceptionAnalyzer.h

public:
   ExceptionAnalyzer() = default;
 
   void ignoreBadAlloc(bool ShallIgnore) { IgnoreBadAlloc = ShallIgnore; }
   void ignoreExceptions(llvm::StringSet<> ExceptionNames) {
     IgnoredExceptions = std::move(ExceptionNames);
   }
 
   ExceptionInfo analyze(const FunctionDecl *Func);
+  ExceptionInfo analyze(const Stmt *Stmt);
 
 private:
   ExceptionInfo
   throwsException(const FunctionDecl *Func,
                   llvm::SmallSet<const FunctionDecl *, 32> &CallStack);
   ExceptionInfo
   throwsException(const Stmt *St, const ExceptionInfo::Throwables &Caught,
                   llvm::SmallSet<const FunctionDecl *, 32> &CallStack);

clang-tidy/utils/ExceptionAnalyzer.cpp

if (FunctionCache.count(Func) == 0) {
     // because it is best to keep as much information as possible.
     // The results here might be relevant to different analysis passes
     // with different needs as well.
     FunctionCache.insert(std::make_pair(Func, ExceptionList));
   } else
     ExceptionList = FunctionCache[Func];
 }
 
+template <>
+void ExceptionAnalyzer::analyze(const Stmt *Stmt,
+                                ExceptionInfo &ExceptionList) {
+  llvm::SmallSet<const FunctionDecl *, 32> CallStack;
+  ExceptionList = throwsException(Stmt, ExceptionInfo::Throwables(), CallStack);
+}
+
 template <typename T>
 ExceptionAnalyzer::ExceptionInfo
 ExceptionAnalyzer::analyzeBoilerplate(const T *Node) {
   ExceptionInfo ExceptionList;
 
   analyze<T>(Node, ExceptionList);
 
   if (ExceptionList.getBehaviour() == State::NotThrowing ||
       ExceptionList.getBehaviour() == State::Unknown)
     return ExceptionList;
 
   // Remove all ignored exceptions from the list of exceptions that can be
   // thrown.
   ExceptionList.filterIgnoredExceptions(IgnoredExceptions, IgnoreBadAlloc);
 
   return ExceptionList;
 }
 
 ExceptionAnalyzer::ExceptionInfo
 ExceptionAnalyzer::analyze(const FunctionDecl *Func) {
   return analyzeBoilerplate(Func);
 }
 
+ExceptionAnalyzer::ExceptionInfo
+ExceptionAnalyzer::analyze(const Stmt *Stmt) {
+  return analyzeBoilerplate(Stmt);
+}
+
 } // namespace utils
 } // namespace tidy
 
 } // namespace clang

docs/ReleaseNotes.rst

 - The `Acronyms` and `IncludeDefaultAcronyms` options for the
   :doc:`objc-property-declaration <clang-tidy/checks/objc-property-declaration>`
   check have been removed.
 
 - The :doc:`modernize-use-override
   <clang-tidy/checks/modernize-use-override>` now supports `OverrideSpelling`
   and `FinalSpelling` options.
 
+- New :doc:`openmp-exception-escape
+  <clang-tidy/checks/openmp-exception-escape>` check.
+
+  Analyzes OpenMP Structured Blocks and checks that no exception escapes
+  out of the Structured Block it was thrown in.
+
 - New :doc:`openmp-use-default-none
   <clang-tidy/checks/openmp-use-default-none>` check.
 
   Finds OpenMP directives that are allowed to contain a ``default`` clause,
   but either don't specify it or the clause is specified but with the kind
   other than ``none``, and suggests to use the ``default(none)`` clause.
 
 Improvements to include-fixer

docs/clang-tidy/checks/list.rst

.. toctree::
    modernize-use-uncaught-exceptions
    modernize-use-using
    mpi-buffer-deref
    mpi-type-mismatch
    objc-avoid-nserror-init
    objc-avoid-spinlock
    objc-forbidden-subclassing
    objc-property-declaration
+   openmp-exception-escape
    openmp-use-default-none
    performance-faster-string-find
    performance-for-range-copy
    performance-implicit-conversion-in-loop
    performance-inefficient-algorithm
    performance-inefficient-string-concatenation
    performance-inefficient-vector-operation
    performance-move-const-arg

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

+.. title:: clang-tidy - openmp-exception-escape
+
+openmp-exception-escape
+=======================
+
+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.
+
+WARNING! This check may be expensive on large source files.
+
+Options
+-------
+
+.. option:: IgnoredExceptions
+
+   Comma separated list containing type names which are not counted as thrown
+   exceptions in the check. Default value is an empty string.

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

+// RUN: %check_clang_tidy %s bugprone-exception-escape %t -- -extra-arg=-fopenmp -extra-arg=-fexceptions --
+
+int thrower() {
+  throw 42;
+}
+
+void ok_parallel() {
+#pragma omp parallel
+  thrower();
+}
+
+void bad_for_header_XFAIL(const int a) {
+#pragma omp for
+  for (int i = 0; i < thrower(); i++)
+    ;
+  // FIXME: this really should be caught by bugprone-exception-escape.
+  // https://bugs.llvm.org/show_bug.cgi?id=41102
+}
+
+void ok_forloop(const int a) {
+#pragma omp for
+  for (int i = 0; i < a; i++)
+    thrower();
+}
+
+void some_exception_just_so_that_check_clang_tidy_shuts_up() noexcept {
+  thrower();
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:6: warning: an exception may be thrown in function 'some_exception_just_so_that_check_clang_tidy_shuts_up' which should not throw exceptions

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

+// RUN: %check_clang_tidy %s openmp-exception-escape %t -- -extra-arg=-fopenmp -extra-arg=-fexceptions -config="{CheckOptions: [{key: openmp-exception-escape.IgnoredExceptions, value: 'ignored'}]}" --
+
+int thrower() {
+  throw 42;
+}
+
+class ignored {};
+namespace std {
+class bad_alloc {};
+} // namespace std
+
+void parallel() {
+#pragma omp parallel
+  thrower();
+  // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: An exception thrown inside of the OpenMP 'parallel' region is not caught in that same region.
+}
+
+void ignore() {
+#pragma omp parallel
+  throw ignored();
+}
+
+void standalone_directive() {
+#pragma omp taskwait
+  throw ignored(); // not structured block
+}
+
+void ignore_alloc() {
+#pragma omp parallel
+  throw std::bad_alloc();
+}
+
+void parallel_caught() {
+#pragma omp parallel
+  {
+    try {
+      thrower();
+    } catch (...) {
+    }
+  }
+}
+
+void for_header(const int a) {
+  // Only the body of the loop counts.
+#pragma omp for
+  for (int i = 0; i < thrower(); i++)
+    ;
+}
+
+void forloop(const int a) {
+#pragma omp for
+  for (int i = 0; i < a; i++)
+    thrower();
+  // CHECK-MESSAGES: :[[@LINE-3]]:9: warning: An exception thrown inside of the OpenMP 'for' region is not caught in that same region.
+}
+
+void parallel_forloop(const int a) {
+#pragma omp parallel
+  {
+#pragma omp for
+    for (int i = 0; i < a; i++)
+      thrower();
+    thrower();
+    // CHECK-MESSAGES: :[[@LINE-6]]:9: warning: An exception thrown inside of the OpenMP 'parallel' region is not caught in that same region.
+    // CHECK-MESSAGES: :[[@LINE-5]]:9: warning: An exception thrown inside of the OpenMP 'for' region is not caught in that same region.
+  }
+}
+
+void parallel_forloop_caught(const int a) {
+#pragma omp parallel
+  {
+#pragma omp for
+    for (int i = 0; i < a; i++) {
+      try {
+        thrower();
+      } catch (...) {
+      }
+    }
+    thrower();
+    // CHECK-MESSAGES: :[[@LINE-10]]:9: warning: An exception thrown inside of the OpenMP 'parallel' region is not caught in that same region.
+  }
+}
+
+void parallel_caught_forloop(const int a) {
+#pragma omp parallel
+  {
+#pragma omp for
+    for (int i = 0; i < a; i++)
+      thrower();
+    try {
+      thrower();
+    } catch (...) {
+    }
+    // CHECK-MESSAGES: :[[@LINE-7]]:9: warning: An exception thrown inside of the OpenMP 'for' region is not caught in that same region.
+  }
+}
+
+void parallel_outercaught_forloop(const int a) {
+#pragma omp parallel
+  {
+    try {
+#pragma omp for
+      for (int i = 0; i < a; i++)
+        thrower();
+      thrower();
+    } catch (...) {
+    }
+    // CHECK-MESSAGES: :[[@LINE-6]]:9: warning: An exception thrown inside of the OpenMP 'for' region is not caught in that same region.
+  }
+}
+
+void parallel_outercaught_forloop_caught(const int a) {
+#pragma omp parallel
+  {
+    try {
+#pragma omp for
+      for (int i = 0; i < a; i++) {
+        try {
+          thrower();
+        } catch (...) {
+        }
+      }
+    } catch (...) {
+    }
+  }
+}