[clang-tidy] signal handler must be plain old function check

[bruntib]

Summary

Based on CERT-MSC54-CPP

[Eugene.Zelenko]

clang-tidy/cert/CERTTidyModule.cpp

64 ↗

(On Diff #101191)

checker -> check.

docs/ReleaseNotes.rst

73 ↗

(On Diff #101191)

Probably C++ should be mentioned for clarity.

docs/clang-tidy/checks/cert-msc54-cpp.rst

7 ↗

(On Diff #101191)

'extern C' -> `extern "C"`.

21 ↗

(On Diff #101191)

Unnecessary empty line.

32 ↗

(On Diff #101191)

Unnecessary empty line.

34 ↗

(On Diff #101191)

Please replace CPP with C++ as in similar checks documentation.

[aaron.ballman]

This check is an interesting one. The rules around what is signal safe are changing for C++17 to be a bit more lenient than what the rules are for C++14. CERT's rule is written against C++14, and so the current behavior matches the rule wording. However, the *intent* of the rule is to ensure that only signal-safe functionality is used from a signal handler, and so from that perspective, I can imagine a user compiling for C++17 to want the relaxed rules to still comply with CERT's wording. What do you think?

clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp

22 ↗

(On Diff #101191)

Since this is only needed once and is quite succinct, I'd just lower it into its usage.

47 ↗

(On Diff #101191)

Same here.

52 ↗

(On Diff #101191)

And here.

83 ↗

(On Diff #101191)

Don't use auto for this one, since the type is neither complex nor spelled out in the initialization.

84 ↗

(On Diff #101191)

Can use isDefined() instead of getDefinition(). Then you can store off the FunctionDecl * for the definition and use it below in the call to hasCxxRepr().

102–103 ↗

(On Diff #101191)

These functions can be marked const.

141 ↗

(On Diff #101191)

'extern \"C\"'' instead of 'external C'. I'd probably reword it to: "signal handlers must be 'extern \"C\""

149 ↗

(On Diff #101191)

representations -> constructs

(same below)

docs/clang-tidy/checks/cert-msc54-cpp.rst

24 ↗

(On Diff #101191)

Space between // and warning; indent the comment.

29 ↗

(On Diff #101191)

Indent the code.

[NorenaLeonetti]

In D33825#772688, @aaron.ballman wrote:

This check is an interesting one. The rules around what is signal safe are changing for C++17 to be a bit more lenient than what the rules are for C++14. CERT's rule is written against C++14, and so the current behavior matches the rule wording. However, the *intent* of the rule is to ensure that only signal-safe functionality is used from a signal handler, and so from that perspective, I can imagine a user compiling for C++17 to want the relaxed rules to still comply with CERT's wording. What do you think?

I think your concerns are right. I've checked the upcoming changes in the C++17 standard draft and I'll add some logic to the code to have those relaxed rules at least partly fulfilled.
For now, I uploaded the changes required for this check.

[jklaehn]

docs/ReleaseNotes.rst

62 ↗

(On Diff #113651)

You missed a conflict marker here (and below in line 149).

[aaron.ballman]

Aside from a few small nits, this looks reasonable. Have you run it over any large code bases that use signals to test the quality of the check?

docs/clang-tidy/checks/cert-msc54-cpp.rst

23 ↗

(On Diff #113652)

The warning text should match the actual warning.

test/clang-tidy/cert-signal-handler-must-be-plain-old-function.cpp

56–57 ↗

(On Diff #113652)

Can you move these to be closer to where the actual note appears? It makes it easier to ensure that the diagnostic appears on the proper line.

[bruntib]

I'll rebase this patch and continue its development.

[bruntib]

I rebased the patch so it compiles with master version of LLVM/Clang. I did no other change, so I would like if this patch would be committed on behalf of @NorenaLeonetti if the patch is acceptable.
I would kindly ask the reviewers to give some comments if any additional modification is needed. I run this checker on DuckDB project and this checker gave two reports on functions which shouldn't be used as signal handler:

duckdb-0.2.0/third_party/sqlsmith/sqlsmith.cc:38:17: do not use C++ constructs in signal handlers [cert-msc54-cpp]
https://github.com/cwida/duckdb/blob/master/third_party/sqlsmith/sqlsmith.cc#L38

duckdb-0.2.0/tools/shell/shell.c:8828:13: signal handlers must be 'extern "C"' [cert-msc54-cpp]
https://github.com/cwida/duckdb/blob/master/tools/shell/shell.c#L8828

I haven't found other C++ projects which use signals. However, this checker reports on the non-compliant code fragments of the corresponding SEI-CERT rule: https://wiki.sei.cmu.edu/confluence/display/cplusplus/MSC54-CPP.+A+signal+handler+must+be+a+plain+old+function. The two findings above also seem to be true positive.

[bruntib]

Update lincense.

[jfb]

Please consider these changes, and whether this is relevant as implemented: http://wg21.link/p0270

clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp

23

"C++ construct" isn't particularly useful. Here it needs to call out throwing exceptions.

58

Here too, I have no idea what this means given just this message.

74

This is also very confusing.

[bruntib]

Test file has been extended with the second line of the "note" diagnostic message which designates the location of the suspicious "C++ construct". The full clang-tidy output looks like this:

llvm-project/clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp:20:17: warning: do not use C++ constructs in signal handlers [cert-msc54-cpp]
extern "C" void cpp_signal_handler(int sig) {

^

llvm-project/clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp:23:3: note: C++ construct used here

throw "error message";
^

[bruntib]

clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp

23

Thanks for the review!
I think the clang-tidy message is understandable, because the next line after "note: C++ construct used here" displays the exact source location where this C++ construct is used. I included this line in the test file so it is visible here. The full message looks like this:

llvm-project/clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp:20:17: warning: do not use C++ constructs in signal handlers [cert-msc54-cpp]
extern "C" void cpp_signal_handler(int sig) {

^

llvm-project/clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp:23:3: note: C++ construct used here

throw "error message";
^

Do you think it's enough, or should the note text be different for all C++ constructs?

[bruntib]

Note texts are now describing what kind of C++ constructs were used. The warning message also explains better the reason of the issue: "signal handlers must be plain old functions, C++-only constructs are not allowed"

[jfb]

MSC54-CPP refers to POF, which as I pointed out above isn't relevant anymore. I'd much rather have a diagnostic which honors the state of things after http://wg21.link/p0270.

Additionally, lots of what MSC54-CPP intends is implementation-defined. We shouldn't diagnose for things that clang has defined as signal safe, at least not by default.

From the paper, I'd like to see tests for these:

• a call to any standard library function, except for plain lock-free atomic atomic operations and functions explicitly identified as signal-safe. [Note: This implicitly excludes the use of new and delete expressions that rely on a library-provided memory allocator. – end note]; -- here I'd like to explicitly see the list of signal-safe functions, and examples of ones that are not
• an access to an object with thread storage duration;
• a dynamic_cast expression;
• throwing of an exception;
• control entering a try-block or function-try-block; or
• initialization of a variable with static storage duration requiring dynamic initialization (3.6.3 [basic.start.dynamic], 6.7 [stmt.decl])). [ Note: Such initialization might occur because it is the first odr-use (3.2 [basic.def.odr]) of that variable. -- end note ]
• waiting for the completion of the initialization of a variable with static storage duration (6.7 [stmt.decl]).

clang-tools-extra/clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp

84

Most of the above are fine per p0270, and most don't have a test at the moment.

clang-tools-extra/docs/clang-tidy/checks/cert-msc54-cpp.rst

15

'extern', not 'external'

23

Update the example too.

clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp

74

I don't think this should diagnose, it is signal safe for clang's implementation, and allowed by p0270.

91

_Thread_local isn't signal safe per p0270.

114

The atomic isn't used here, and atomic initialization isn't atomic, so the test isn't sufficient.

[aaron.ballman]

In D33825#2246369, @jfb wrote:

MSC54-CPP refers to POF, which as I pointed out above isn't relevant anymore. I'd much rather have a diagnostic which honors the state of things after http://wg21.link/p0270.

I agree, and I've commented on that rule to remind the folks at CERT that the rule has largely gone stale.

Additionally, lots of what MSC54-CPP intends is implementation-defined. We shouldn't diagnose for things that clang has defined as signal safe, at least not by default.

Also agreed.

clang-tools-extra/clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp

42

I'm not super keen on this approach because as new C++ constructs are added, this will continually need to be updated, which is a maintenance burden I think we should try to avoid. I would rather use some generic wording or tie the wording automatically to something provided by the Stmt class.

44

Diagnostics in clang-tidy should start with a lowercase letter

91

Similar concerns here.

clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp

 #include "DontModifyStdNamespaceCheck.h"
 #include "FloatLoopCounter.h"
 #include "LimitedRandomnessCheck.h"
 #include "MutatingCopyCheck.h"
 #include "NonTrivialTypesLibcMemoryCallsCheck.h"
 #include "PostfixOperatorCheck.h"
 #include "ProperlySeededRandomGeneratorCheck.h"
 #include "SetLongJmpCheck.h"
+#include "SignalHandlerMustBePlainOldFunctionCheck.h"
 #include "StaticObjectExceptionCheck.h"
 #include "StrToNumCheck.h"
 #include "ThrownExceptionTypeCheck.h"
 #include "VariadicFunctionDefCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace cert {
CheckFactories.registerCheck<misc::ThrowByValueCatchByReferenceCheck>(
         "cert-err61-cpp");
     // MEM
     CheckFactories.registerCheck<DefaultOperatorNewAlignmentCheck>(
         "cert-mem57-cpp");
     // MSC
     CheckFactories.registerCheck<LimitedRandomnessCheck>("cert-msc50-cpp");
     CheckFactories.registerCheck<ProperlySeededRandomGeneratorCheck>(
         "cert-msc51-cpp");
+    CheckFactories.registerCheck<SignalHandlerMustBePlainOldFunctionCheck>(
+        "cert-msc54-cpp");
     // OOP
     CheckFactories.registerCheck<performance::MoveConstructorInitCheck>(
         "cert-oop11-cpp");
     CheckFactories.registerCheck<bugprone::UnhandledSelfAssignmentCheck>(
         "cert-oop54-cpp");
     CheckFactories.registerCheck<NonTrivialTypesLibcMemoryCallsCheck>(
         "cert-oop57-cpp");
     CheckFactories.registerCheck<MutatingCopyCheck>(

clang-tools-extra/clang-tidy/cert/CMakeLists.txt

add_clang_library(clangTidyCERTModule
   DontModifyStdNamespaceCheck.cpp
   FloatLoopCounter.cpp
   LimitedRandomnessCheck.cpp
   MutatingCopyCheck.cpp
   NonTrivialTypesLibcMemoryCallsCheck.cpp
   PostfixOperatorCheck.cpp
   ProperlySeededRandomGeneratorCheck.cpp
   SetLongJmpCheck.cpp
+  SignalHandlerMustBePlainOldFunctionCheck.cpp
   StaticObjectExceptionCheck.cpp
   StrToNumCheck.cpp
   ThrownExceptionTypeCheck.cpp
   VariadicFunctionDefCheck.cpp
 
   LINK_LIBS
   clangTidy
   clangTidyBugproneModule

clang-tools-extra/clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.h

+//===--- CERTTidyModule.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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_SIGNAL_HANDLER_MUST_BE_PLAIN_OLD_FUNCTION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_SIGNAL_HANDLER_MUST_BE_PLAIN_OLD_FUNCTION_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace cert {
+
+/// A signal handler must be a plain old function.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cert-msc54-cpp.html
+class SignalHandlerMustBePlainOldFunctionCheck : public ClangTidyCheck {
+public:
+  SignalHandlerMustBePlainOldFunctionCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace cert
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_SIGNAL_HANDLER_MUST_BE_PLAIN_OLD_FUNCTION_H

clang-tools-extra/clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp

+//===--- CERTTidyModule.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 "SignalHandlerMustBePlainOldFunctionCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include <queue>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace cert {
+
+namespace {
+internal::Matcher<Decl> hasCxxStmt() {
+  return hasDescendant(
+      stmt(anyOf(cxxBindTemporaryExpr(), cxxBoolLiteral(), cxxCatchStmt(),
+                 cxxConstCastExpr(), cxxConstructExpr(), cxxDefaultArgExpr(),
+                 cxxDeleteExpr(), cxxDynamicCastExpr(), cxxForRangeStmt(),
+                 cxxFunctionalCastExpr(), cxxMemberCallExpr(), cxxNewExpr(),
+                 cxxNullPtrLiteralExpr(), cxxOperatorCallExpr(),
+                 cxxReinterpretCastExpr(), cxxStaticCastExpr(),
+                 cxxTemporaryObjectExpr(), cxxThisExpr(), cxxThrowExpr(),
+                 cxxTryStmt(), cxxUnresolvedConstructExpr()))
+          .bind("cxx_stmt"));
+}
+
+internal::Matcher<Decl> hasCxxDecl() {
+  return hasDescendant(
+      decl(anyOf(cxxConstructorDecl(), cxxConversionDecl(), cxxDestructorDecl(),
+                 cxxMethodDecl(),
+                 cxxRecordDecl(unless(anyOf(isStruct(), isUnion())))))
+          .bind("cxx_decl"));
+}
+
+const char *cxxStmtText(const Stmt *stmt) {
+  if (llvm::isa<CXXBindTemporaryExpr>(stmt))
+    return "Binding temporary C++ expression here";
+  else if (llvm::isa<CXXBoolLiteralExpr>(stmt))
+    return "C++ boolean literal used here";
+  else if (llvm::isa<CXXCatchStmt>(stmt))
+    return "catch statement used here";
+  else if (llvm::isa<CXXConstCastExpr>(stmt))
+    return "const_cast statement used here";
+  else if (llvm::isa<CXXConstructExpr>(stmt))
+    return "Constructor called here";
+  else if (llvm::isa<CXXDefaultArgExpr>(stmt))
+    return "Default function argument used here";
+  else if (llvm::isa<CXXDeleteExpr>(stmt))
+    return "delete statement used here";
+  else if (llvm::isa<CXXDynamicCastExpr>(stmt))
+    return "dynamic_cast statement used here";
+  else if (llvm::isa<CXXForRangeStmt>(stmt))
+    return "for range loop used here";
+  else if (llvm::isa<CXXFunctionalCastExpr>(stmt))
+    return "C++-style cast expression used here";
+  else if (llvm::isa<CXXMemberCallExpr>(stmt))
+    return "Member function called here";
+  else if (llvm::isa<CXXNewExpr>(stmt))
+    return "new statement used here";
+  else if (llvm::isa<CXXNullPtrLiteralExpr>(stmt))
+    return "nullptr used here";
+  else if (llvm::isa<CXXOperatorCallExpr>(stmt))
+    return "C++ operator used here";
+  else if (llvm::isa<CXXReinterpretCastExpr>(stmt))
+    return "reinterpret_cast statement used here";
+  else if (llvm::isa<CXXStaticCastExpr>(stmt))
+    return "static_cast statement used here";
+  else if (llvm::isa<CXXTemporaryObjectExpr>(stmt))
+    return "Temporary object created here";
+  else if (llvm::isa<CXXThisExpr>(stmt))
+    return "\"this\" object used here";
+  else if (llvm::isa<CXXThrowExpr>(stmt))
+    return "throw statement used here";
+  else if (llvm::isa<CXXTryStmt>(stmt))
+    return "try statement used here";
+  else if (llvm::isa<CXXUnresolvedConstructExpr>(stmt))
+    return "Construction of an unresolved type here";
+  else
+    // Shouldn't reach this point.
+    return "C++ construct used here";
+}
+
+const char *cxxDeclText(const Decl *decl) {
+  if (llvm::isa<CXXConstructorDecl>(decl))
+    return "Constructor declared here";
+  else if (llvm::isa<CXXConversionDecl>(decl))
+    return "Conversion operator declared here";
+  else if (llvm::isa<CXXDestructorDecl>(decl))
+    return "Destructor declared here";
+  else if (llvm::isa<CXXMethodDecl>(decl))
+    return "Method declared here";
+  else if (llvm::isa<CXXRecordDecl>(decl))
+    return "C++ record declared here";
+  else
+    // Shouldn't reach this point.
+    return "C++ construct used here";
+}
+
+class CallGraphCheck {
+  std::queue<const FunctionDecl *> UncheckedCalls;
+  llvm::DenseSet<const FunctionDecl *> CheckedCalls;
+  SourceLocation CxxRepresentation;
+  SourceLocation FunctionCall;
+  ASTContext *Context;
+  const char *CxxReprNote;
+
+  bool hasCxxRepr(const FunctionDecl *Func) {
+    auto MatchesCxxStmt = match(hasCxxStmt(), *Func, *Context);
+    if (!MatchesCxxStmt.empty()) {
+      const Stmt *stmt = MatchesCxxStmt[0].getNodeAs<Stmt>("cxx_stmt");
+      CxxRepresentation = stmt->getBeginLoc();
+      CxxReprNote = cxxStmtText(stmt);
+      return true;
+    }
+
+    auto MatchesCxxDecl = match(hasCxxDecl(), *Func, *Context);
+    if (!MatchesCxxDecl.empty()) {
+      const Decl *decl = MatchesCxxDecl[0].getNodeAs<Decl>("cxx_decl");
+      CxxRepresentation = decl->getBeginLoc();
+      CxxReprNote = cxxDeclText(decl);
+      return true;
+    }
+    return false;
+  }
+
+  bool callExprContainsCxxRepr(SmallVectorImpl<BoundNodes> &Calls) {
+    for (const auto &Match : Calls) {
+      const auto *Call = Match.getNodeAs<CallExpr>("call");
+      const FunctionDecl *Func = Call->getDirectCallee();
+      if (!Func || !Func->isDefined())
+        continue;
+      auto MatchesCxxMethod = match(decl(cxxMethodDecl()), *Func, *Context);
+      if (hasCxxRepr(Func->getDefinition()) || !MatchesCxxMethod.empty()) {
+        FunctionCall = Call->getBeginLoc();
+        return true;
+      }
+      if (!CheckedCalls.count(Func))
+        UncheckedCalls.push(Func);
+    }
+    return false;
+  }
+
+public:
+  CallGraphCheck(const FunctionDecl *SignalHandler, ASTContext *ResultContext)
+      : Context(ResultContext) {
+    UncheckedCalls.push(SignalHandler);
+  }
+  const SourceLocation callLoc() { return FunctionCall; }
+  const SourceLocation cxxRepLoc() { return CxxRepresentation; }
+  const char *cxxRepNoteText() { return CxxReprNote; }
+
+  bool checkAllCall() {
+    while (!UncheckedCalls.empty()) {
+      CheckedCalls.insert(UncheckedCalls.front());
+      auto Matches = match(decl(forEachDescendant(callExpr().bind("call"))),
+                           *UncheckedCalls.front()->getDefinition(), *Context);
+      UncheckedCalls.pop();
+      if (callExprContainsCxxRepr(Matches))
+        return true;
+    }
+    return false;
+  }
+};
+} // namespace
+
+void SignalHandlerMustBePlainOldFunctionCheck::registerMatchers(
+    MatchFinder *Finder) {
+  auto SignalHas = [](const internal::Matcher<Decl> &SignalHandler) {
+    return callExpr(hasDeclaration(functionDecl(hasName("signal"))),
+                    hasArgument(1, declRefExpr(hasDeclaration(SignalHandler))
+                                       .bind("signal_argument")));
+  };
+  auto SignalHandler = [](const internal::Matcher<Decl> &HasCxxRepr) {
+    return functionDecl(isExternC(), HasCxxRepr)
+        .bind("signal_handler_with_cxx_repr");
+  };
+  Finder->addMatcher(
+      SignalHas(functionDecl(unless(isExternC())).bind("signal_handler")),
+      this);
+  Finder->addMatcher(SignalHas(SignalHandler(hasCxxStmt())), this);
+  Finder->addMatcher(SignalHas(SignalHandler(hasCxxDecl())), this);
+  Finder->addMatcher(SignalHas(functionDecl(hasDescendant(callExpr()))
+                                   .bind("signal_handler_with_call_expr")),
+                     this);
+}
+
+void SignalHandlerMustBePlainOldFunctionCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  if (const auto *SignalHandler =
+          Result.Nodes.getNodeAs<FunctionDecl>("signal_handler")) {
+    diag(SignalHandler->getLocation(),
+         "signal handlers must be 'extern \"C\"'");
+    diag(Result.Nodes.getNodeAs<DeclRefExpr>("signal_argument")->getLocation(),
+         "given as a signal parameter here", DiagnosticIDs::Note);
+  }
+
+  if (const auto *SingalHandler = Result.Nodes.getNodeAs<FunctionDecl>(
+          "signal_handler_with_cxx_repr")) {
+    diag(SingalHandler->getLocation(),
+         "signal handlers must be plain old functions, "
+         "C++-only constructs are not allowed");
+    if (const auto *CxxStmt = Result.Nodes.getNodeAs<Stmt>("cxx_stmt"))
+      diag(CxxStmt->getBeginLoc(), cxxStmtText(CxxStmt), DiagnosticIDs::Note);
+    else {
+      const auto *CxxDecl = Result.Nodes.getNodeAs<Decl>("cxx_decl");
+      diag(CxxDecl->getBeginLoc(), cxxDeclText(CxxDecl), DiagnosticIDs::Note);
+    }
+  }
+
+  if (const auto *SignalHandler = Result.Nodes.getNodeAs<FunctionDecl>(
+          "signal_handler_with_call_expr")) {
+    CallGraphCheck CallChain(SignalHandler, Result.Context);
+    if (CallChain.checkAllCall()) {
+      diag(SignalHandler->getLocation(), "do not call functions with C++ "
+                                         "constructs in signal "
+                                         "handlers");
+      diag(CallChain.callLoc(), "function called here", DiagnosticIDs::Note);
+      if (CallChain.cxxRepLoc().isValid())
+        diag(CallChain.cxxRepLoc(), CallChain.cxxRepNoteText(),
+             DiagnosticIDs::Note);
+    }
+  }
+}
+
+} // namespace cert
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 - Improved :doc:`readability-identifier-naming
   <clang-tidy/checks/readability-identifier-naming>` check.
 
   Added an option `GetConfigPerFile` to support including files which use
   different naming styles.
 
+New checks
+^^^^^^^^^^
+
+- New `cert-msc54-cpp
+  <http://clang.llvm.org/extra/clang-tidy/checks/cert-msc54-cpp.html>`_ check
+
+  Checks if a signal handler is an 'extern \"C\" function without C++ constructs.
+
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to clang-include-fixer
 -----------------------------------
 

clang-tools-extra/docs/clang-tidy/checks/cert-msc54-cpp.rst

+.. title:: clang-tidy - cert-msc54-cpp
+
+cert-msc54-cpp
+==============
+
+This check will give a warning if a signal handler is not defined
+as an `extern "C"` function or if the declaration of the function
+contains C++ representation.
+
+Here's an example:
+
+ .. code-block:: c++
+
+    static void sig_handler(int sig) {}
+    // warning: use 'external C' prefix for signal handlers
+
+    void install_signal_handler() {
+      if (SIG_ERR == std::signal(SIGTERM, sig_handler))
+        return;
+    }
+
+    extern "C" void cpp_signal_handler(int sig) {
+      // warning: do not use C++ constructs in signal handlers
+      throw "error message";
+    }
+
+    void install_cpp_signal_handler() {
+      if (SIG_ERR == std::signal(SIGTERM, cpp_signal_handler))
+        return;
+    }
+
+This check corresponds to the CERT C++ Coding Standard rule
+`MSC54-CPP. A signal handler must be a plain old function
+<https://www.securecoding.cert.org/confluence/display/cplusplus/MSC54-CPP.+A+signal+handler+must+be+a+plain+old+function>`_.

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `cert-err34-c <cert-err34-c.html>`_,
    `cert-err52-cpp <cert-err52-cpp.html>`_,
    `cert-err58-cpp <cert-err58-cpp.html>`_,
    `cert-err60-cpp <cert-err60-cpp.html>`_,
    `cert-flp30-c <cert-flp30-c.html>`_,
    `cert-mem57-cpp <cert-mem57-cpp.html>`_,
    `cert-msc50-cpp <cert-msc50-cpp.html>`_,
    `cert-msc51-cpp <cert-msc51-cpp.html>`_,
+   `cert-msc54-cpp <cert-msc54-cpp.html>`_,
    `cert-oop57-cpp <cert-oop57-cpp.html>`_,
    `cert-oop58-cpp <cert-oop58-cpp.html>`_,
    `clang-analyzer-core.DynamicTypePropagation <clang-analyzer-core.DynamicTypePropagation.html>`_,
    `clang-analyzer-core.uninitialized.CapturedBlockVariable <clang-analyzer-core.uninitialized.CapturedBlockVariable.html>`_,
    `clang-analyzer-cplusplus.InnerPointer <clang-analyzer-cplusplus.InnerPointer.html>`_,
    `clang-analyzer-nullability.NullableReturnedFromNonnull <clang-analyzer-nullability.NullableReturnedFromNonnull.html>`_,
    `clang-analyzer-optin.osx.OSObjectCStyleCast <clang-analyzer-optin.osx.OSObjectCStyleCast.html>`_,
    `clang-analyzer-optin.performance.GCDAntipattern <clang-analyzer-optin.performance.GCDAntipattern.html>`_,

clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp

+// RUN: %check_clang_tidy %s cert-msc54-cpp %t -- -- -std=c++11
+
+namespace std {
+extern "C" using signalhandler = void(int);
+int signal(int sig, signalhandler *func);
+}
+
+#define SIG_ERR -1
+#define SIGTERM 15
+
+static void sig_handler(int sig) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: signal handlers must be 'extern "C"' [cert-msc54-cpp]
+// CHECK-MESSAGES: :[[@LINE+3]]:39: note: given as a signal parameter here
+
+void install_signal_handler() {
+  if (SIG_ERR == std::signal(SIGTERM, sig_handler))
+    return;
+}
+
+extern "C" void cpp_signal_handler(int sig) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: signal handlers must be plain old functions, C++-only constructs are not allowed [cert-msc54-cpp]
+  // CHECK-MESSAGES: :[[@LINE+1]]:3: note: throw statement used here
+  throw "error message";
+}
+
+void install_cpp_signal_handler() {
+  if (SIG_ERR == std::signal(SIGTERM, cpp_signal_handler))
+    return;
+}
+
+void indirect_recursion();
+
+void cpp_like(){
+  try {
+    cpp_signal_handler(SIG_ERR);
+  } catch(...) {
+    // handle error
+  }
+}
+
+void no_body();
+
+void recursive_function() {
+  indirect_recursion();
+  cpp_like();
+  no_body();
+  recursive_function();
+}
+
+void indirect_recursion() {
+  recursive_function();
+}
+
+extern "C" void signal_handler_with_cpp_function_call(int sig) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: do not call functions with C++ constructs in signal handlers [cert-msc54-cpp]
+  // CHECK-MESSAGES: :[[@LINE-11]]:3: note: function called here
+  // CHECK-MESSAGES: :[[@LINE-23]]:3: note: try statement used here
+  recursive_function();
+}
+
+void install_signal_handler_with_cpp_function_call() {
+  if (SIG_ERR == std::signal(SIGTERM, signal_handler_with_cpp_function_call))
+    return;
+}
+
+class TestClass {
+public:
+  static void static_function() {}
+};
+
+extern "C" void signal_handler_with_cpp_static_method_call(int sig) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: do not call functions with C++ constructs in signal handlers [cert-msc54-cpp]
+  // CHECK-MESSAGES: :[[@LINE+2]]:3: note: function called here
+  // CHECK-MESSAGES: TestClass::static_function();
+  TestClass::static_function();
+}
+
+void install_signal_handler_with_cpp_static_method_call() {
+  if (SIG_ERR == std::signal(SIGTERM, signal_handler_with_cpp_static_method_call))
+    return;
+}
+
+
+// Something that does not trigger the check:
+
+extern "C" void c_sig_handler(int sig) {
+#define cbrt(X) _Generic((X), long double \
+                         : cbrtl, default \
+                         : cbrt)(X)
+  auto char c = '1';
+  extern _Thread_local double _Complex d;
+  static const unsigned long int i = sizeof(float);
+  void f();
+  volatile struct c_struct {
+    enum e {};
+    union u;
+  };
+  typedef bool boolean;
+label:
+  switch (c) {
+  case ('1'):
+    break;
+  default:
+    d = 1.2;
+  }
+  goto label;
+  for (; i < 42;) {
+    if (d == 1.2)
+      continue;
+    else
+      return;
+  }
+  do {
+    _Atomic int v = _Alignof(char);
+    _Static_assert(42 == 42, "True");
+  } while (c == 42);
+}
+
+void install_c_sig_handler() {
+  if (SIG_ERR == std::signal(SIGTERM, c_sig_handler)) {
+    // Handle error
+  }
+}
+
+extern "C" void signal_handler_with_function_pointer(int sig) {
+  void (*funp) (void);
+  funp = &cpp_like;
+  funp();
+}
+
+void install_signal_handler_with_function_pointer() {
+  if (SIG_ERR == std::signal(SIGTERM, signal_handler_with_function_pointer))
+    return;
+}