[clang-tidy] Change code of SignalHandlerCheck (NFC).

[balazske]

Summary

Using clang::CallGraph to get the called functions.
This makes a better foundation to improve support for
C++ and print the call chain.

[balazske]

The plan is to take over functionality from D91164, D97960 and D33825 after this change.

[aaron.ballman]

Thanks for the cleanup, I like the direction this is heading. But is this actually NFC? I thought using a CallGraph would change the behavior in code like:

if (false) { // Statically known never to take this branch
  bad_call(); // Used to diagnose, does it still?
}

but I could be remembering incorrectly.

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

129

Should we also assert that this matcher is triggered before any other matcher is matched?

Alternatively, instead of matching on the TU decl itself, would it make sense to walk the decl contexts up to the TU level on the first match?

[njames93]

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

129

The nicest solution would be to change the OnStartOfTranslationUnit callback to pass the ASTContext. That way checks which require setup code like this don't need to register matchers.
However this would be a breaking change.

[balazske]

Added tests to check more specific behavior.
Removed AST matcher of TU.

[balazske]

The CallGraph just scans for FunctionDecl and CallExpr, no check for unreachable code is there.

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

129

Call graph is now populated at the first normal match, no need for the additional AST matcher.

[aaron.ballman]

LGTM aside from a commenting nit. Thanks!

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

136

[balazske]

Corrected a comment.

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

 //===--- SignalHandlerCheck.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_BUGPRONE_SIGNALHANDLERCHECK_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SIGNALHANDLERCHECK_H
 
 #include "../ClangTidyCheck.h"
+#include "clang/Analysis/CallGraph.h"
 #include "llvm/ADT/StringSet.h"
 
 namespace clang {
 namespace tidy {
 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 };
 
   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,
                  const CallExpr *SignalCall, const FunctionDecl *HandlerDecl);
-  bool isSystemCallAllowed(const FunctionDecl *FD) const;
+
+  CallGraph CG;
 
   AsyncSafeFunctionSetType AsyncSafeFunctionSet;
   llvm::StringSet<> &ConformingFunctions;
 
   static llvm::StringSet<> MinimalConformingFunctions;
   static llvm::StringSet<> POSIXConformingFunctions;
 };
 
 } // 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/AST/ASTContext.h"
-#include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
-#include "clang/Analysis/CallGraph.h"
-#include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/DepthFirstIterator.h"
 #include "llvm/ADT/STLExtras.h"
-#include "llvm/ADT/SmallVector.h"
-#include <iterator>
-#include <queue>
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 
 template <>
 struct OptionEnumMapping<
static constexpr std::pair<
              "POSIX"},
         };
     return makeArrayRef(Mapping);
   }
 };
 
 namespace bugprone {
 
-static bool isSystemCall(const FunctionDecl *FD) {
+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>
bool isSystemCall(const FunctionDecl *FD) {
   // 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 ==
void SignalHandlerCheck::registerMatchers(MatchFinder *Finder) {
   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.");
 
-  // Visit each function encountered in the callgraph only once.
-  llvm::DenseSet<const FunctionDecl *> SeenFunctions;
-
-  // The worklist of the callgraph visitation algorithm.
-  std::deque<const CallExpr *> CalledFunctions;
-
-  auto ProcessFunction = [&](const FunctionDecl *F, const Expr *CallOrRef) {
-    // Ensure that canonical declaration is used.
-    F = F->getCanonicalDecl();
-
-    // Do not visit function if already encountered.
-    if (!SeenFunctions.insert(F).second)
-      return true;
-
-    // Check if the call is allowed.
-    // Non-system calls are not considered.
-    if (isSystemCall(F)) {
-      if (isSystemCallAllowed(F))
-        return true;
-
-      reportBug(F, CallOrRef, SignalCall, HandlerDecl);
-
-      return false;
-    }
+  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, SignalCall, HandlerDecl);
+    return;
 
-    // Get the body of the encountered non-system call function.
-    const FunctionDecl *FBody;
-    if (!F->hasBody(FBody)) {
-      reportBug(F, CallOrRef, SignalCall, HandlerDecl);
-      return false;
+  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),
+                SignalCall, HandlerDecl);
     }
-
-    // Collect all called functions.
-    auto Matches = match(decl(forEachDescendant(callExpr().bind("call"))),
-                         *FBody, FBody->getASTContext());
-    for (const auto &Match : Matches) {
-      const auto *CE = Match.getNodeAs<CallExpr>("call");
-      if (isa<FunctionDecl>(CE->getCalleeDecl()))
-        CalledFunctions.push_back(CE);
-    }
-
-    return true;
-  };
-
-  if (!ProcessFunction(HandlerDecl, HandlerExpr))
-    return;
-
-  // Visit the definition of every function referenced by the handler function.
-  // Check for allowed function calls.
-  while (!CalledFunctions.empty()) {
-    const CallExpr *FunctionCall = CalledFunctions.front();
-    CalledFunctions.pop_front();
-    // At insertion we have already ensured that only function calls are there.
-    const auto *F = cast<FunctionDecl>(FunctionCall->getCalleeDecl());
-
-    if (!ProcessFunction(F, FunctionCall))
-      break;
-  }
+
+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::isSystemCallAllowed(const FunctionDecl *FD) const {
+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;

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

void handler_bad(int) {
   f_bad();
 }
 
 void handler_extern(int) {
   f_extern();
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'f_extern' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
 }
 
+void test_false_condition(int) {
+  if (0)
+    printf("1234");
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+}
+
+void test_multiple_calls(int) {
+  f_extern();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'f_extern' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  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]
+  f_extern();
+  // first 'f_extern' call found only
+}
+
+void f_recursive();
+
+void test_recursive(int) {
+  f_recursive();
+  printf("");
+  // first 'printf' call (in other function) found only
+}
+
+void f_recursive() {
+  f_extern();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'f_extern' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  printf("");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  f_recursive(2);
+}
+
 void test() {
   signal(SIGINT, handler_abort);
   signal(SIGINT, handler_signal);
   signal(SIGINT, handler_other);
 
   signal(SIGINT, handler_ok);
   signal(SIGINT, handler_bad);
   signal(SIGINT, handler_extern);
 
   signal(SIGINT, _Exit);
   signal(SIGINT, other_call);
   // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 'other_call' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
 
   signal(SIGINT, SIG_IGN);
   signal(SIGINT, SIG_DFL);
+
+  signal(SIGINT, test_false_condition);
+  signal(SIGINT, test_multiple_calls);
+  signal(SIGINT, test_recursive);
 }