[clang-tidy] bugprone-signal-handler: Message improvement and code refactoring.

[balazske]

Summary

Another change of the code design.
Code simplified again, now there is a single place to check
a handler function and less functions for bug report emitting.
More details are added to the bug report messages.

[LegalizeAdulthood]

clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp

14

Why do we ignore the return value of checkFunction here?

Also, the name doesn't reveal to me why the result is true
or false, e.g. it acts like a predicate but its name doesn't ask
a question.

[balazske]

clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp

13–14

There are problems with this approach in C++ code.

14

The function returns if a problem (warning) was found. At the next call site it is used to display additional notes, but here it is not needed. A better name can be isFunctionValidSignalHandler but the function checks not only a fact, it generates the warnings too.

[njames93]

clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp

16

This can probably just be inlined into its only use.

clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h

36

This isn't a very descriptive name, and the bool return value conveys no meaning. Probably could do with some documentation.

61–63

While you're refactoring, those static StringSets really belong in the implementation file, and the ConformingFunctions should be a const ref.

However global destructores are kind of a taboo thing, Maybe there is case to change them into a sorted array and binary search them to see if a function is conforming.
Probably wouldn't advise using TableGens StringMatcher to build this functionality as that maybe a little too far.

[balazske]

Even more simplification, added documentation.
Text of bug reports is more detailed, no NFC now.

[balazske]

clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp

13–14

The call graph adds nodes for the canonical declaration only. When getting a node it works only with the CanonicalDecl unless the CallGraph code is changed (it seems to be worth to do this change).

16

The value is used now multiple times. (But maybe the diag call can be put into a lambda later.)

[LegalizeAdulthood]

clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp

14

Can we put (void) checkFunction(...) here to make it clear
we're intentionally ignoring the return value?

clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h

35–36

... as a signal handler

36

Should test the properties ...

36–58

... from a signal

39

.. from the signal handler ...

41

... as a registered

43

... only if a problem ...

[balazske]

Corrected the comment problems, explained why return value of checkFunction is ignored.

[balazske]

Rebase

[balazske]

ping

[aaron.ballman]

Mostly small nits, but it looks like precommit CI is failing due to the changes:

Failed Tests (2):
  Clang Tools :: clang-tidy/checkers/bugprone-signal-handler-minimal.c
  Clang Tools :: clang-tidy/checkers/bugprone-signal-handler-posix.c

clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp

15

17

30

clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h

45

[balazske]

Applied review comments, fixed the failing tests.

[whisperity]

This generally looks good, and thank you! I have a few minor comments (mostly presentation and documentation).

clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp

13–14

13–14

(Nit: There are a lot of indirections in the documentation that did not help understanding what is happening here.)

16

Otherwise, if these are truly const and we are reasonably sure they can be instantiated on any meaningful system without crashing, then these could be a TU-static. It's still iffy, but that technique is used in many places already.

16–17

Perchance this could work with constexpr? Although I'm not sure how widely supported that is, given that we're pegged at C++14 still.

18–24

What does the return value of checkFunction signal to the user, and the if?

30

clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h

36–59

First, we usually call these "diagnostics" in Tidy and not "bug report notes", which to me seems like CSA terminology. Second... It's not clear to me what (from it) is referring to. "[...] show call chain from a signal handler to a [... specific? provided? expected? ...] function" sounds alright to me.

42

61–63

Once the ConformingFunctions is const, these symbols are not needed here, right? Static initialisation does feel dirty... (heck, there's even a CERT rule against that too!)

clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler-posix.c

14

I'm not exactly sure we should call printf (and memcpy) a "system call". As far as I can see, in this patch, isSystemCall() boils down to "declaration in file included as system header"

clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c

31

133

[balazske]

clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp

13–14

Is this code not too long to put in the condition section?

18–24

This has a description in the header file.

clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler-posix.c

14

Call it "standard function"?

[whisperity]

clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp

13–14

The formatter will break it up appropriately, no problem with that.

clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler-posix.c

14

Yes, that would be better.

[balazske]

Applied the review proposals.
Changed "system call" to "standard function".
Put the string lists into constant std::initializer_list.

[balazske]

Additional documentation related fixes.

[whisperity]

Alright, I think this is good to go. I like that it makes it clear that the called function is coming from some external source (system header or otherwise).

clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h

 namespace bugprone {
 
 /// Checker for signal handler functions.
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-signal-handler-check.html
 class SignalHandlerCheck : public ClangTidyCheck {
 public:
-  enum class AsyncSafeFunctionSetType { Minimal, POSIX };
+  enum class AsyncSafeFunctionSetKind { Minimal, POSIX };
 
   SignalHandlerCheck(StringRef Name, ClangTidyContext *Context);
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
-  bool isFunctionAsyncSafe(const FunctionDecl *FD) const;
-  bool isSystemCallAsyncSafe(const FunctionDecl *FD) const;
-  void reportBug(const FunctionDecl *CalledFunction, const Expr *CallOrRef,
-                 bool DirectHandler);
-  void reportHandlerCommon(llvm::df_iterator<clang::CallGraphNode *> Itr,
-                           const CallExpr *SignalCall,
-                           const FunctionDecl *HandlerDecl,
-                           const Expr *HandlerRef);
+  /// Check if a function is allowed as a signal handler.
+  /// Should test the properties of the function, and check in the code body.
+  /// Should not check function calls in the code (this part is done by the call
+  /// graph scan).
+  /// @param FD The function to check. It may or may not have a definition.
+  /// @param CallOrRef Location of the call to this function (in another
+  /// function) or the reference to the function (if it is used as a registered
+  /// signal handler). This is the location where diagnostics are to be placed.
+  /// @return Returns true if a diagnostic was emitted for this function.
+  bool checkFunction(const FunctionDecl *FD, const Expr *CallOrRef);
+  /// Returns true if a standard library function is considered as
+  /// asynchronous-safe.
+  bool isStandardFunctionAsyncSafe(const FunctionDecl *FD) const;
+  /// Add diagnostic notes to show the call chain of functions from a signal
+  /// handler to a function that is called (directly or indirectly) from it.
+  /// Also add a note to the place where the signal handler is registered.
+  /// @param Itr Position during a call graph depth-first iteration. It contains
+  /// the "path" (call chain) from the signal handler to the actual found
+  /// function call.
+  /// @param HandlerRef Reference to the signal handler function where it is
+  /// registered as signal handler.
+  void reportHandlerChain(const llvm::df_iterator<clang::CallGraphNode *> &Itr,
+                          const DeclRefExpr *HandlerRef);
 
   clang::CallGraph CG;
 
-  AsyncSafeFunctionSetType AsyncSafeFunctionSet;
-  llvm::StringSet<> &ConformingFunctions;
-
-  static llvm::StringSet<> MinimalConformingFunctions;
-  static llvm::StringSet<> POSIXConformingFunctions;
+  AsyncSafeFunctionSetKind AsyncSafeFunctionSet;
+  const llvm::StringSet<> ConformingFunctions;
 };
 
 } // namespace bugprone
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SIGNALHANDLERCHECK_H

clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp

 //===--- SignalHandlerCheck.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 "SignalHandlerCheck.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "llvm/ADT/DepthFirstIterator.h"
 #include "llvm/ADT/STLExtras.h"
 
-using namespace clang::ast_matchers;
-
-namespace clang {
-namespace tidy {
-
-template <>
-struct OptionEnumMapping<
-    bugprone::SignalHandlerCheck::AsyncSafeFunctionSetType> {
-  static llvm::ArrayRef<std::pair<
-      bugprone::SignalHandlerCheck::AsyncSafeFunctionSetType, StringRef>>
-  getEnumMapping() {
-    static constexpr std::pair<
-        bugprone::SignalHandlerCheck::AsyncSafeFunctionSetType, StringRef>
-        Mapping[] = {
-            {bugprone::SignalHandlerCheck::AsyncSafeFunctionSetType::Minimal,
-             "minimal"},
-            {bugprone::SignalHandlerCheck::AsyncSafeFunctionSetType::POSIX,
-             "POSIX"},
-        };
-    return makeArrayRef(Mapping);
-  }
-};
-
-namespace bugprone {
-
-namespace {
-
-bool isSystemCall(const FunctionDecl *FD) {
-  // Find a possible redeclaration in system header.
-  // FIXME: Looking at the canonical declaration is not the most exact way
-  // to do this.
-
-  // Most common case will be inclusion directly from a header.
-  // This works fine by using canonical declaration.
-  // a.c
-  // #include <sysheader.h>
-
-  // Next most common case will be extern declaration.
-  // Can't catch this with either approach.
-  // b.c
-  // extern void sysfunc(void);
-
-  // Canonical declaration is the first found declaration, so this works.
-  // c.c
-  // #include <sysheader.h>
-  // extern void sysfunc(void); // redecl won't matter
-
-  // This does not work with canonical declaration.
-  // Probably this is not a frequently used case but may happen (the first
-  // declaration can be in a non-system header for example).
-  // d.c
-  // extern void sysfunc(void); // Canonical declaration, not in system header.
-  // #include <sysheader.h>
-
-  return FD->getASTContext().getSourceManager().isInSystemHeader(
-      FD->getCanonicalDecl()->getLocation());
-}
-
-/// Given a call graph node of a function and another one that is called from
-/// this function, get a CallExpr of the corresponding function call.
-/// It is unspecified which call is found if multiple calls exist, but the order
-/// should be deterministic (depend only on the AST).
-Expr *findCallExpr(const CallGraphNode *Caller, const CallGraphNode *Callee) {
-  auto FoundCallee = llvm::find_if(
-      Caller->callees(), [Callee](const CallGraphNode::CallRecord &Call) {
-        return Call.Callee == Callee;
-      });
-  assert(FoundCallee != Caller->end() &&
-         "Callee should be called from the caller function here.");
-  return FoundCallee->CallExpr;
-}
-
-} // namespace
-
-AST_MATCHER(FunctionDecl, isSystemCall) { return isSystemCall(&Node); }
-
-SignalHandlerCheck::SignalHandlerCheck(StringRef Name,
-                                       ClangTidyContext *Context)
-    : ClangTidyCheck(Name, Context),
-      AsyncSafeFunctionSet(
-          Options.get("AsyncSafeFunctionSet", AsyncSafeFunctionSetType::POSIX)),
-      ConformingFunctions(AsyncSafeFunctionSet ==
-                                  AsyncSafeFunctionSetType::Minimal
-                              ? MinimalConformingFunctions
-                              : POSIXConformingFunctions) {}
-
-void SignalHandlerCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
-  Options.store(Opts, "AsyncSafeFunctionSet", AsyncSafeFunctionSet);
-}
-
-bool SignalHandlerCheck::isLanguageVersionSupported(
-    const LangOptions &LangOpts) const {
-  // FIXME: Make the checker useful on C++ code.
-  if (LangOpts.CPlusPlus)
-    return false;
-
-  return true;
-}
-
-void SignalHandlerCheck::registerMatchers(MatchFinder *Finder) {
-  auto SignalFunction = functionDecl(hasAnyName("::signal", "::std::signal"),
-                                     parameterCountIs(2), isSystemCall());
-  auto HandlerExpr =
-      declRefExpr(hasDeclaration(functionDecl().bind("handler_decl")),
-                  unless(isExpandedFromMacro("SIG_IGN")),
-                  unless(isExpandedFromMacro("SIG_DFL")))
-          .bind("handler_expr");
-  Finder->addMatcher(
-      callExpr(callee(SignalFunction), hasArgument(1, HandlerExpr))
-          .bind("register_call"),
-      this);
-}
-
-void SignalHandlerCheck::check(const MatchFinder::MatchResult &Result) {
-  const auto *SignalCall = Result.Nodes.getNodeAs<CallExpr>("register_call");
-  const auto *HandlerDecl =
-      Result.Nodes.getNodeAs<FunctionDecl>("handler_decl");
-  const auto *HandlerExpr = Result.Nodes.getNodeAs<DeclRefExpr>("handler_expr");
-  assert(SignalCall && HandlerDecl && HandlerExpr &&
-         "All of these should exist in a match here.");
-
-  if (CG.size() <= 1) {
-    // Call graph must be populated with the entire TU at the beginning.
-    // (It is possible to add a single function but the functions called from it
-    // are not analysed in this case.)
-    CG.addToCallGraph(const_cast<TranslationUnitDecl *>(
-        HandlerDecl->getTranslationUnitDecl()));
-    assert(CG.size() > 1 &&
-           "There should be at least one function added to call graph.");
-  }
-
-  // Check for special case when the signal handler itself is an unsafe external
-  // function.
-  if (!isFunctionAsyncSafe(HandlerDecl)) {
-    reportBug(HandlerDecl, HandlerExpr, /*DirectHandler=*/true);
-    return;
-  }
-
-  CallGraphNode *HandlerNode = CG.getNode(HandlerDecl);
-  // Signal handler can be external but not unsafe, no call graph in this case.
-  if (!HandlerNode)
-    return;
-  // Start from signal handler and visit every function call.
-  for (auto Itr = llvm::df_begin(HandlerNode), ItrE = llvm::df_end(HandlerNode);
-       Itr != ItrE; ++Itr) {
-    const auto *CallF = dyn_cast<FunctionDecl>((*Itr)->getDecl());
-    if (CallF && !isFunctionAsyncSafe(CallF)) {
-      assert(Itr.getPathLength() >= 2);
-      reportBug(CallF, findCallExpr(Itr.getPath(Itr.getPathLength() - 2), *Itr),
-                /*DirectHandler=*/false);
-      reportHandlerCommon(Itr, SignalCall, HandlerDecl, HandlerExpr);
-    }
-  }
-}
-
-bool SignalHandlerCheck::isFunctionAsyncSafe(const FunctionDecl *FD) const {
-  if (isSystemCall(FD))
-    return isSystemCallAsyncSafe(FD);
-  // For external (not checkable) functions assume that these are unsafe.
-  return FD->hasBody();
-}
-
-bool SignalHandlerCheck::isSystemCallAsyncSafe(const FunctionDecl *FD) const {
-  const IdentifierInfo *II = FD->getIdentifier();
-  // Unnamed functions are not explicitly allowed.
-  if (!II)
-    return false;
-
-  // FIXME: Improve for C++ (check for namespace).
-  if (ConformingFunctions.count(II->getName()))
-    return true;
-
-  return false;
-}
-
-void SignalHandlerCheck::reportBug(const FunctionDecl *CalledFunction,
-                                   const Expr *CallOrRef, bool DirectHandler) {
-  diag(CallOrRef->getBeginLoc(),
-       "%0 may not be asynchronous-safe; %select{calling it from|using it as}1 "
-       "a signal handler may be dangerous")
-      << CalledFunction << DirectHandler;
-}
-
-void SignalHandlerCheck::reportHandlerCommon(
-    llvm::df_iterator<clang::CallGraphNode *> Itr, const CallExpr *SignalCall,
-    const FunctionDecl *HandlerDecl, const Expr *HandlerRef) {
-  int CallLevel = Itr.getPathLength() - 2;
-  assert(CallLevel >= -1 && "Empty iterator?");
-
-  const CallGraphNode *Caller = Itr.getPath(CallLevel + 1), *Callee = nullptr;
-  while (CallLevel >= 0) {
-    Callee = Caller;
-    Caller = Itr.getPath(CallLevel);
-    const Expr *CE = findCallExpr(Caller, Callee);
-    diag(CE->getBeginLoc(), "function %0 called here from %1",
-         DiagnosticIDs::Note)
-        << cast<FunctionDecl>(Callee->getDecl())
-        << cast<FunctionDecl>(Caller->getDecl());
-    --CallLevel;
-  }
-
-  diag(HandlerRef->getBeginLoc(),
-       "function %0 registered here as signal handler", DiagnosticIDs::Note)
-      << HandlerDecl;
-}
-
 // This is the minimal set of safe functions.
 // https://wiki.sei.cmu.edu/confluence/display/c/SIG30-C.+Call+only+asynchronous-safe+functions+within+signal+handlers
-llvm::StringSet<> SignalHandlerCheck::MinimalConformingFunctions{
+constexpr std::initializer_list<llvm::StringRef> MinimalConformingFunctions = {
     "signal", "abort", "_Exit", "quick_exit"};
 
 // The POSIX-defined set of safe functions.
 // https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03
 // 'quick_exit' is added to the set additionally because it looks like the
 // mentioned POSIX specification was not updated after 'quick_exit' appeared
 // in the C11 standard.
 // Also, we want to keep the "minimal set" a subset of the "POSIX set".
-llvm::StringSet<> SignalHandlerCheck::POSIXConformingFunctions{
+constexpr std::initializer_list<llvm::StringRef> POSIXConformingFunctions = {
     "_Exit",
     "_exit",
     "abort",
     "accept",
     "access",
     "aio_error",
     "aio_return",
     "aio_suspend",
     "alarm",
     "bind",
     "cfgetispeed",
     "cfgetospeed",
     "cfsetispeed",
constexpr std::initializer_list<llvm::StringRef> POSIXConformingFunctions = {
     "wcstok",
     "wmemchr",
     "wmemcmp",
     "wmemcpy",
     "wmemmove",
     "wmemset",
     "write"};
 
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+
+template <>
+struct OptionEnumMapping<
+    bugprone::SignalHandlerCheck::AsyncSafeFunctionSetKind> {
+  static llvm::ArrayRef<std::pair<
+      bugprone::SignalHandlerCheck::AsyncSafeFunctionSetKind, StringRef>>
+  getEnumMapping() {
+    static constexpr std::pair<
+        bugprone::SignalHandlerCheck::AsyncSafeFunctionSetKind, StringRef>
+        Mapping[] = {
+            {bugprone::SignalHandlerCheck::AsyncSafeFunctionSetKind::Minimal,
+             "minimal"},
+            {bugprone::SignalHandlerCheck::AsyncSafeFunctionSetKind::POSIX,
+             "POSIX"},
+        };
+    return makeArrayRef(Mapping);
+  }
+};
+
+namespace bugprone {
+
+namespace {
+
+/// Returns if a function is declared inside a system header.
+/// These functions are considered to be "standard" (system-provided) library
+/// functions.
+bool isStandardFunction(const FunctionDecl *FD) {
+  // Find a possible redeclaration in system header.
+  // FIXME: Looking at the canonical declaration is not the most exact way
+  // to do this.
+
+  // Most common case will be inclusion directly from a header.
+  // This works fine by using canonical declaration.
+  // a.c
+  // #include <sysheader.h>
+
+  // Next most common case will be extern declaration.
+  // Can't catch this with either approach.
+  // b.c
+  // extern void sysfunc(void);
+
+  // Canonical declaration is the first found declaration, so this works.
+  // c.c
+  // #include <sysheader.h>
+  // extern void sysfunc(void); // redecl won't matter
+
+  // This does not work with canonical declaration.
+  // Probably this is not a frequently used case but may happen (the first
+  // declaration can be in a non-system header for example).
+  // d.c
+  // extern void sysfunc(void); // Canonical declaration, not in system header.
+  // #include <sysheader.h>
+
+  return FD->getASTContext().getSourceManager().isInSystemHeader(
+      FD->getCanonicalDecl()->getLocation());
+}
+
+/// Given a call graph node of a \p Caller function and a \p Callee that is
+/// called from \p Caller, get a \c CallExpr of the corresponding function call.
+/// It is unspecified which call is found if multiple calls exist, but the order
+/// should be deterministic (depend only on the AST).
+Expr *findCallExpr(const CallGraphNode *Caller, const CallGraphNode *Callee) {
+  auto FoundCallee = llvm::find_if(
+      Caller->callees(), [Callee](const CallGraphNode::CallRecord &Call) {
+        return Call.Callee == Callee;
+      });
+  assert(FoundCallee != Caller->end() &&
+         "Callee should be called from the caller function here.");
+  return FoundCallee->CallExpr;
+}
+
+} // namespace
+
+AST_MATCHER(FunctionDecl, isStandardFunction) {
+  return isStandardFunction(&Node);
+}
+
+SignalHandlerCheck::SignalHandlerCheck(StringRef Name,
+                                       ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      AsyncSafeFunctionSet(
+          Options.get("AsyncSafeFunctionSet", AsyncSafeFunctionSetKind::POSIX)),
+      ConformingFunctions(AsyncSafeFunctionSet ==
+                                  AsyncSafeFunctionSetKind::Minimal
+                              ? MinimalConformingFunctions
+                              : POSIXConformingFunctions) {}
+
+void SignalHandlerCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "AsyncSafeFunctionSet", AsyncSafeFunctionSet);
+}
+
+bool SignalHandlerCheck::isLanguageVersionSupported(
+    const LangOptions &LangOpts) const {
+  // FIXME: Make the checker useful on C++ code.
+  if (LangOpts.CPlusPlus)
+    return false;
+
+  return true;
+}
+
+void SignalHandlerCheck::registerMatchers(MatchFinder *Finder) {
+  auto SignalFunction = functionDecl(hasAnyName("::signal", "::std::signal"),
+                                     parameterCountIs(2), isStandardFunction());
+  auto HandlerExpr =
+      declRefExpr(hasDeclaration(functionDecl().bind("handler_decl")),
+                  unless(isExpandedFromMacro("SIG_IGN")),
+                  unless(isExpandedFromMacro("SIG_DFL")))
+          .bind("handler_expr");
+  Finder->addMatcher(
+      callExpr(callee(SignalFunction), hasArgument(1, HandlerExpr))
+          .bind("register_call"),
+      this);
+}
+
+void SignalHandlerCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *SignalCall = Result.Nodes.getNodeAs<CallExpr>("register_call");
+  const auto *HandlerDecl =
+      Result.Nodes.getNodeAs<FunctionDecl>("handler_decl");
+  const auto *HandlerExpr = Result.Nodes.getNodeAs<DeclRefExpr>("handler_expr");
+  assert(SignalCall && HandlerDecl && HandlerExpr &&
+         "All of these should exist in a match here.");
+
+  if (CG.size() <= 1) {
+    // Call graph must be populated with the entire TU at the beginning.
+    // (It is possible to add a single function but the functions called from it
+    // are not analysed in this case.)
+    CG.addToCallGraph(const_cast<TranslationUnitDecl *>(
+        HandlerDecl->getTranslationUnitDecl()));
+    assert(CG.size() > 1 &&
+           "There should be at least one function added to call graph.");
+  }
+
+  if (!HandlerDecl->hasBody()) {
+    (void)checkFunction(HandlerDecl, HandlerExpr);
+    // Here checkFunction will put the messages to HandlerExpr.
+    // No need to show a call chain.
+    // Without code body there is nothing more to check.
+    return;
+  }
+
+  CallGraphNode *HandlerNode = CG.getNode(HandlerDecl->getCanonicalDecl());
+  assert(HandlerNode &&
+         "Handler with body should be present in the call graph.");
+  // Start from signal handler and visit every function call.
+  for (auto Itr = llvm::df_begin(HandlerNode), ItrE = llvm::df_end(HandlerNode);
+       Itr != ItrE; ++Itr) {
+    if (const auto *CallF = dyn_cast<FunctionDecl>((*Itr)->getDecl())) {
+      unsigned int PathL = Itr.getPathLength();
+      const Expr *CallOrRef = (PathL == 1)
+                                  ? HandlerExpr
+                                  : findCallExpr(Itr.getPath(PathL - 2), *Itr);
+      if (checkFunction(CallF, CallOrRef))
+        reportHandlerChain(Itr, HandlerExpr);
+    }
+  }
+}
+
+bool SignalHandlerCheck::checkFunction(const FunctionDecl *FD,
+                                       const Expr *CallOrRef) {
+  bool FunctionIsCalled = isa<CallExpr>(CallOrRef);
+
+  if (isStandardFunction(FD)) {
+    if (!isStandardFunctionAsyncSafe(FD)) {
+      diag(CallOrRef->getBeginLoc(), "standard function %0 may not be "
+                                     "asynchronous-safe; "
+                                     "%select{using it as|calling it from}1 "
+                                     "a signal handler may be dangerous")
+          << FD << FunctionIsCalled;
+      return true;
+    }
+    return false;
+  }
+
+  if (!FD->hasBody()) {
+    diag(CallOrRef->getBeginLoc(), "cannot verify that external function %0 is "
+                                   "asynchronous-safe; "
+                                   "%select{using it as|calling it from}1 "
+                                   "a signal handler may be dangerous")
+        << FD << FunctionIsCalled;
+    return true;
+  }
+
+  return false;
+}
+
+bool SignalHandlerCheck::isStandardFunctionAsyncSafe(
+    const FunctionDecl *FD) const {
+  assert(isStandardFunction(FD));
+
+  const IdentifierInfo *II = FD->getIdentifier();
+  // Unnamed functions are not explicitly allowed.
+  if (!II)
+    return false;
+
+  // FIXME: Improve for C++ (check for namespace).
+  if (ConformingFunctions.count(II->getName()))
+    return true;
+
+  return false;
+}
+
+void SignalHandlerCheck::reportHandlerChain(
+    const llvm::df_iterator<clang::CallGraphNode *> &Itr,
+    const DeclRefExpr *HandlerRef) {
+  int CallLevel = Itr.getPathLength() - 2;
+  assert(CallLevel >= -1 && "Empty iterator?");
+
+  const CallGraphNode *Caller = Itr.getPath(CallLevel + 1), *Callee = nullptr;
+  while (CallLevel >= 0) {
+    Callee = Caller;
+    Caller = Itr.getPath(CallLevel);
+    const Expr *CE = findCallExpr(Caller, Callee);
+    diag(CE->getBeginLoc(), "function %0 called here from %1",
+         DiagnosticIDs::Note)
+        << cast<FunctionDecl>(Callee->getDecl())
+        << cast<FunctionDecl>(Caller->getDecl());
+    --CallLevel;
+  }
+
+  diag(HandlerRef->getBeginLoc(),
+       "function %0 registered here as signal handler", DiagnosticIDs::Note)
+      << cast<FunctionDecl>(Caller->getDecl()) << HandlerRef->getSourceRange();
+}
+
 } // namespace bugprone
 } // namespace tidy
 } // namespace clang

clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler-minimal.c

 // RUN: %check_clang_tidy %s bugprone-signal-handler %t \
 // RUN: -config='{CheckOptions: \
 // RUN:  [{key: bugprone-signal-handler.AsyncSafeFunctionSet, value: "minimal"}]}' \
 // RUN: -- -isystem %S/Inputs/Headers
 
 #include "signal.h"
 #include "stdlib.h"
 #include "string.h"
 #include "unistd.h"
 
 void handler_bad1(int) {
   _exit(0);
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: '_exit' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: standard function '_exit' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
 }
 
 void handler_bad2(void *dst, const void *src) {
   memcpy(dst, src, 10);
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'memcpy' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: standard function 'memcpy' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
 }
 
 void handler_good(int) {
   abort();
   _Exit(0);
   quick_exit(0);
   signal(0, SIG_DFL);
 }
 
 void test(void) {
   signal(SIGINT, handler_bad1);
   signal(SIGINT, handler_bad2);
   signal(SIGINT, handler_good);
 }

clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler-posix.c

 // RUN: %check_clang_tidy %s bugprone-signal-handler %t \
 // RUN: -config='{CheckOptions: \
 // RUN:  [{key: bugprone-signal-handler.AsyncSafeFunctionSet, value: "POSIX"}]}' \
 // RUN: -- -isystem %S/Inputs/Headers
 
 #include "signal.h"
 #include "stdlib.h"
 #include "stdio.h"
 #include "string.h"
 #include "unistd.h"
 
 void handler_bad(int) {
   printf("1234");
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: standard function 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
 }
 
 void handler_good(int) {
   abort();
   _Exit(0);
   _exit(0);
   quick_exit(0);
   signal(0, SIG_DFL);
   memcpy((void*)10, (const void*)20, 1);
 }
 
 void test(void) {
   signal(SIGINT, handler_good);
   signal(SIGINT, handler_bad);
 }

clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c

 // RUN: %check_clang_tidy %s bugprone-signal-handler %t -- -- -isystem %S/Inputs/Headers
 
 #include "signal.h"
 #include "stdlib.h"
 #include "stdio.h"
 #include "system-other.h"
 
-// The function should be classified as system call even if there is
+// The function should be classified as standard function even if there is
 // declaration the in source file.
 // FIXME: The detection works only if the first declaration is in system
 // header.
 int printf(const char *, ...);
 typedef void (*sighandler_t)(int);
 sighandler_t signal(int signum, sighandler_t handler);
 
 void f_extern(void);
 
 void handler_printf(int) {
   printf("1234");
-  // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-NOTES: :[[@LINE-1]]:3: warning: standard function 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
   // CHECK-NOTES: :[[@LINE-2]]:3: note: function 'printf' called here from 'handler_printf'
   // CHECK-NOTES: :[[@LINE+4]]:18: note: function 'handler_printf' registered here as signal handler
 }
 
 void test_printf(void) {
   signal(SIGINT, handler_printf);
 }
 
 void handler_extern(int) {
   f_extern();
-  // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'f_extern' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-NOTES: :[[@LINE-1]]:3: warning: cannot verify that external function 'f_extern' is asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
   // CHECK-NOTES: :[[@LINE-2]]:3: note: function 'f_extern' called here from 'handler_extern'
   // CHECK-NOTES: :[[@LINE+4]]:18: note: function 'handler_extern' registered here as signal handler
 }
 
 void test_extern(void) {
   signal(SIGINT, handler_extern);
 }
 
 void f_ok(void) {
   abort();
 }
 
 void handler_ok(int) {
   f_ok();
 }
 
 void test_ok(void) {
   signal(SIGINT, handler_ok);
 }
 
 void f_bad(void) {
   printf("1234");
-  // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-NOTES: :[[@LINE-1]]:3: warning: standard function 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
   // CHECK-NOTES: :[[@LINE-2]]:3: note: function 'printf' called here from 'f_bad'
   // CHECK-NOTES: :[[@LINE+5]]:3: note: function 'f_bad' called here from 'handler_bad'
   // CHECK-NOTES: :[[@LINE+8]]:18: note: function 'handler_bad' registered here as signal handler
 }
 
 void handler_bad(int) {
   f_bad();
 }
 
 void test_bad(void) {
   signal(SIGINT, handler_bad);
 }
 
 void f_bad1(void) {
   printf("1234");
-  // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-NOTES: :[[@LINE-1]]:3: warning: standard function 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
   // CHECK-NOTES: :[[@LINE-2]]:3: note: function 'printf' called here from 'f_bad1'
   // CHECK-NOTES: :[[@LINE+6]]:3: note: function 'f_bad1' called here from 'f_bad2'
   // CHECK-NOTES: :[[@LINE+9]]:3: note: function 'f_bad2' called here from 'handler_bad1'
   // CHECK-NOTES: :[[@LINE+13]]:18: note: function 'handler_bad1' registered here as signal handler
 }
 
 void f_bad2(void) {
   f_bad1();

 void handler_signal(int) {
   // FIXME: It is only OK to call signal with the current signal number.
   signal(0, SIG_DFL);
 }
 
 void handler_false_condition(int) {
   if (0)
     printf("1234");
-  // CHECK-NOTES: :[[@LINE-1]]:5: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-NOTES: :[[@LINE-1]]:5: warning: standard function 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
   // CHECK-NOTES: :[[@LINE-2]]:5: note: function 'printf' called here from 'handler_false_condition'
   // CHECK-NOTES: :[[@LINE+4]]:18: note: function 'handler_false_condition' registered here as signal handler
 }
 
 void test_false_condition(void) {
   signal(SIGINT, handler_false_condition);
 }
 
 void handler_multiple_calls(int) {
   f_extern();
-  // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'f_extern' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-NOTES: :[[@LINE-1]]:3: warning: cannot verify that external function 'f_extern' is asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
   // CHECK-NOTES: :[[@LINE-2]]:3: note: function 'f_extern' called here from 'handler_multiple_calls'
   // CHECK-NOTES: :[[@LINE+10]]:18: note: function 'handler_multiple_calls' registered here as signal handler
   printf("1234");
-  // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-NOTES: :[[@LINE-1]]:3: warning: standard function 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
   // CHECK-NOTES: :[[@LINE-2]]:3: note: function 'printf' called here from 'handler_multiple_calls'
   // CHECK-NOTES: :[[@LINE+6]]:18: note: function 'handler_multiple_calls' registered here as signal handler
   f_extern();
   // first 'f_extern' call found only
 }
 
 void test_multiple_calls(void) {
   signal(SIGINT, handler_multiple_calls);
 }
 
 void f_recursive(void);
 
 void handler_recursive(int) {
   f_recursive();
   printf("");
-  // first 'printf' call (in other function) found only
+  // first 'printf' call (in f_recursive) found only
 }
 
 void f_recursive(void) {
   f_extern();
-  // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'f_extern' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-NOTES: :[[@LINE-1]]:3: warning: cannot verify that external function 'f_extern' is asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
   // CHECK-NOTES: :[[@LINE-2]]:3: note: function 'f_extern' called here from 'f_recursive'
   // CHECK-NOTES: :[[@LINE-9]]:3: note: function 'f_recursive' called here from 'handler_recursive'
   // CHECK-NOTES: :[[@LINE+10]]:18: note: function 'handler_recursive' registered here as signal handler
   printf("");
-  // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-NOTES: :[[@LINE-1]]:3: warning: standard function 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
   // CHECK-NOTES: :[[@LINE-2]]:3: note: function 'printf' called here from 'f_recursive'
   // CHECK-NOTES: :[[@LINE-14]]:3: note: function 'f_recursive' called here from 'handler_recursive'
   // CHECK-NOTES: :[[@LINE+5]]:18: note: function 'handler_recursive' registered here as signal handler
   handler_recursive(2);
 }
 
 void test_recursive(void) {
   signal(SIGINT, handler_recursive);
 }
 
 void f_multiple_paths(void) {
   printf("");
-  // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-NOTES: :[[@LINE-1]]:3: warning: standard function 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
   // CHECK-NOTES: :[[@LINE-2]]:3: note: function 'printf' called here from 'f_multiple_paths'
   // CHECK-NOTES: :[[@LINE+5]]:3: note: function 'f_multiple_paths' called here from 'handler_multiple_paths'
   // CHECK-NOTES: :[[@LINE+9]]:18: note: function 'handler_multiple_paths' registered here as signal handler
 }
 
 void handler_multiple_paths(int) {
   f_multiple_paths();
   f_multiple_paths();

 }
 
 void test_other(void) {
   signal(SIGINT, handler_abort);
   signal(SIGINT, handler_signal);
 
   signal(SIGINT, _Exit);
   signal(SIGINT, other_call);
-  // CHECK-NOTES: :[[@LINE-1]]:18: warning: 'other_call' may not be asynchronous-safe; using it as a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-NOTES: :[[@LINE-1]]:18: warning: standard function 'other_call' may not be asynchronous-safe; using it as a signal handler may be dangerous [bugprone-signal-handler]
   signal(SIGINT, f_extern);
-  // CHECK-NOTES: :[[@LINE-1]]:18: warning: 'f_extern' may not be asynchronous-safe; using it as a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-NOTES: :[[@LINE-1]]:18: warning: cannot verify that external function 'f_extern' is asynchronous-safe; using it as a signal handler may be dangerous [bugprone-signal-handler]
 
   signal(SIGINT, SIG_IGN);
   signal(SIGINT, SIG_DFL);
 }