[clang-tidy] bugprone-signal-handler improvements: display call chain

[balazske]

Summary

Add a new feature to display the call chain if an unsafe function is
found in a signal handler or function called from it.
Warning messages are improved too.

[njames93]

This needs tests demonstrating the notes. The defacto way is to replace all CHECK-MESSAGESDirectives with CHECK-NOTES. This'll for file check to look for lines containing note.

[balazske]

I tried to test the notes but could not get it to work. I think the problem is related to the fact that the notes are displayed at different locations that are past or before the current warning. The solution could be to manually specify line numbers in the CHECK-NOTE parts but I do not like this (and did not tried it). Or is there other solution?

[njames93]

The issue is FileCheck expects its directives to be in order they appear in the file being checked. Notes are always emitted just after the warning they are attached to.
So there are a few ways to go:

• use CHECK-NOTES-DAG to disregard the ordering and place them next to the location of the note in the code, to give correct line information.
• place the CHECK-NOTES directly after the warning they are attached.

Going with the first option makes it much harder to reason about.
Going with the second option you'll either have to just put a generic number regex for the line number, use (potentially large) relative @LINE offsets or absolute line numbers.

[balazske]

Adding test of notes.
Tests are re-arranged significantly.

[Eugene.Zelenko]

I think call stack may be useful for other checks too. May be code should be moved to utilities?

[balazske]

In D97960#2611531, @Eugene.Zelenko wrote:

I think call stack may be useful for other checks too. May be code should be moved to utilities?

Yes it is useful for other checks, but the code here is specialized for this problem only. It is possible to make a call graph that stores the "parents" of the calls, probably root items that are of a special kind, and can add notes for call chain. But there is already a clang::CallGraph that can compute call graph, if I remember correctly it was not usable here because the parent node (caller) is not available. That CallGraph could be extended too.

[martong]

So, about the tests, gentle ping @njames93

[balazske]

A new implementation is in D118224.

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 "llvm/ADT/DenseMap.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:
-  void reportBug(const FunctionDecl *CalledFunction, const Expr *CallOrRef,
-                 const CallExpr *SignalCall, const FunctionDecl *HandlerDecl);
+  using ParentMap =
+      llvm::DenseMap<const FunctionDecl *,
+                     std::pair<const Expr *, const FunctionDecl *>>;
+
+  bool checkCallAllowed(const FunctionDecl *F, const Expr *CallOrRef,
+                        const ParentMap &ParentOfCall);
+  void addCallStackNotes(const FunctionDecl *CheckedFunction,
+                         const ParentMap &ParentOfCall);
   void reportNonExternCBug(const Expr *HandlerRef,
                            const FunctionDecl *HandlerDecl);
   void reportLambdaBug(const Expr *HandlerRef);
 
   bool isSystemCallAllowed(const FunctionDecl *FD) const;
 
   AsyncSafeFunctionSetType AsyncSafeFunctionSet;
   llvm::StringSet<> &ConformingFunctions;

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

 
 void SignalHandlerCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *LambdaHandler = Result.Nodes.getNodeAs<LambdaExpr>("lambda");
   if (LambdaHandler) {
     reportLambdaBug(LambdaHandler);
     return;
   }
 
-  const auto *SignalCall = Result.Nodes.getNodeAs<CallExpr>("register_call");
   const auto *HandlerDecl =
-      Result.Nodes.getNodeAs<FunctionDecl>("handler_decl");
+      Result.Nodes.getNodeAs<FunctionDecl>("handler_decl")->getCanonicalDecl();
   const auto *HandlerExpr = Result.Nodes.getNodeAs<DeclRefExpr>("handler_expr");
+  assert(HandlerDecl && HandlerExpr &&
+         "Handler function and reference to it should both be found.");
 
   if (!HandlerDecl->isExternC()) {
     reportNonExternCBug(HandlerExpr, HandlerDecl);
     return;
   }
 
-  // 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;
+  std::deque<const Expr *> CallsToCheck;
+  // Visit each function encountered in the callgraph only once.
+  // Store its caller expression and the function where it appears.
+  ParentMap ParentOfCall;
 
-      reportBug(F, CallOrRef, SignalCall, HandlerDecl);
+  // For the real signal handler we have no parent call but a reference to it
+  // (in the call to 'signal').
+  ParentOfCall[HandlerDecl] = std::make_pair(HandlerExpr, nullptr);
+  CallsToCheck.push_back(HandlerExpr);
+
+  while (!CallsToCheck.empty()) {
+    const Expr *CallOrRef = CallsToCheck.front();
+    CallsToCheck.pop_front();
+    const FunctionDecl *F =
+        (CallOrRef == HandlerExpr)
+            ? HandlerDecl
+            : cast<FunctionDecl>(cast<CallExpr>(CallOrRef)->getCalleeDecl())
+                  ->getCanonicalDecl();
 
-      return false;
-    }
+    if (!checkCallAllowed(F, CallOrRef, ParentOfCall))
+      continue;
 
-    // Get the body of the encountered non-system call function.
     const FunctionDecl *FBody;
-    if (!F->hasBody(FBody)) {
-      reportBug(F, CallOrRef, SignalCall, HandlerDecl);
-      return false;
-    }
+    if (!F->hasBody(FBody))
+      continue;
 
     // 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);
+      if (const auto *CalleeF = dyn_cast<FunctionDecl>(CE->getCalleeDecl())) {
+        CalleeF = CalleeF->getCanonicalDecl();
+        if (!ParentOfCall.count(CalleeF)) {
+          ParentOfCall[CalleeF] = std::make_pair(CE, F);
+          CallsToCheck.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::isSystemCallAllowed(const FunctionDecl *FD) const {
-  const IdentifierInfo *II = FD->getIdentifier();
-  // Can not verify if std operators are safe to call.
-  if (!II)
-    return false;
-
-  if (!FD->isInStdNamespace() && !isInNoNamespace(FD))
-    return false;
-
-  if (ConformingFunctions.count(II->getName()))
+bool SignalHandlerCheck::checkCallAllowed(const FunctionDecl *F,
+                                          const Expr *CallOrRef,
+                                          const ParentMap &ParentOfCall) {
+  StringRef FunctionKindStr;
+  if (isSystemCall(F)) {
+    if (isSystemCallAllowed(F))
+      return true;
+    FunctionKindStr = "system call";
+  } else {
+    if (F->hasBody())
       return true;
+    FunctionKindStr = "function";
+  }
+
+  if (isa<DeclRefExpr>(CallOrRef)) {
+    // Function F appears directly in 'signal' call.
+    diag(CallOrRef->getBeginLoc(), "%0 %1 may not be asynchronous-safe; using "
+                                   "it as signal handler may be dangerous")
+        << FunctionKindStr << F;
+  } else {
+    // Function F is called from another function.
+    diag(CallOrRef->getBeginLoc(),
+         "%0 %1 may not be asynchronous-safe; calling it from a signal handler "
+         "may be dangerous")
+        << FunctionKindStr << F;
+    addCallStackNotes(F, ParentOfCall);
+  }
 
   return false;
 }
 
-void SignalHandlerCheck::reportBug(const FunctionDecl *CalledFunction,
-                                   const Expr *CallOrRef,
-                                   const CallExpr *SignalCall,
-                                   const FunctionDecl *HandlerDecl) {
-  diag(CallOrRef->getBeginLoc(),
-       "%0 may not be asynchronous-safe; "
-       "calling it from a signal handler may be dangerous")
-      << CalledFunction;
-  diag(SignalCall->getSourceRange().getBegin(),
-       "signal handler registered here", DiagnosticIDs::Note);
-  diag(HandlerDecl->getBeginLoc(), "handler function declared here",
-       DiagnosticIDs::Note);
+void SignalHandlerCheck::addCallStackNotes(const FunctionDecl *F,
+                                           const ParentMap &ParentOfCall) {
+  auto I = ParentOfCall.find(F);
+  while (I != ParentOfCall.end()) {
+    auto Caller = I->second;
+    if (const auto *HandlerRef = dyn_cast<DeclRefExpr>(Caller.first)) {
+      diag(HandlerRef->getBeginLoc(),
+           "function %0 registered here as signal handler", DiagnosticIDs::Note)
+          << F;
+      break;
+    } else {
+      const auto *Call = cast<CallExpr>(Caller.first);
+      diag(Call->getBeginLoc(), "function %0 called here from %1",
+           DiagnosticIDs::Note)
+          << F << Caller.second;
+    }
+    F = Caller.second;
+    I = ParentOfCall.find(F);
+  }
 }
 
 void SignalHandlerCheck::reportNonExternCBug(const Expr *HandlerRef,
                                              const FunctionDecl *HandlerDecl) {
   diag(HandlerRef->getBeginLoc(),
        "function %0 should have C language linkage if used as signal handler")
       << HandlerDecl;
   diag(HandlerDecl->getBeginLoc(), "handler function declared here",
        DiagnosticIDs::Note);
 }
 
 void SignalHandlerCheck::reportLambdaBug(const Expr *HandlerRef) {
   diag(HandlerRef->getBeginLoc(),
        "lambda expression is not allowed as signal handler");
 }
 
+bool SignalHandlerCheck::isSystemCallAllowed(const FunctionDecl *FD) const {
+  const IdentifierInfo *II = FD->getIdentifier();
+  // Can not verify if std operators are safe to call.
+  if (!II)
+    return false;
+
+  if (!FD->isInStdNamespace() && !isInNoNamespace(FD))
+    return false;
+
+  if (ConformingFunctions.count(II->getName()))
+    return true;
+
+  return false;
+}
+
 // 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{
     "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

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: system call '_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: system call '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() {
   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: system call '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() {
   signal(SIGINT, handler_good);
   signal(SIGINT, handler_bad);
 }

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

 sighandler_t signal(int signum, sighandler_t handler);
 
 void handler_abort(int) {
   abort();
 }
 
 void handler_other(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: system call 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
 }
 
 void handler_signal(int) {
   // FIXME: It is only OK to call signal with the current signal number.
   signal(0, SIG_DFL);
 }
 
 void f_ok() {
   abort();
 }
 
 void f_bad() {
   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: system call 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
 }
 
 void f_extern();
 
 void handler_ok(int) {
   f_ok();
 }
 
 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]
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'f_extern' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
 }
 
 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]
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: system call 'other_call' may not be asynchronous-safe; using it as signal handler may be dangerous [bugprone-signal-handler]
 
   signal(SIGINT, SIG_IGN);
   signal(SIGINT, SIG_DFL);
 }

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

 struct S {
   static void signal(int, callback_t);
   static void handler(int);
 
   S() = default;
   S(int) {
     std::other_call();
     // Should be fixed.
-    // CHECK-MESSAGES-NOT: :[[@LINE-2]]:5: warning: 'other_call' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+    // CHECK-MESSAGES-NOT: :[[@LINE-2]]:5: warning: system call 'other_call' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
   };
   int operator-() const {
     std::other_call();
-    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'other_call' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: system call 'other_call' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
   }
 };
 
 struct TestMemberInit {
   static int memberInit() {
     std::other_call();
     // Should be fixed.
-    // CHECK-MESSAGES-NOT: :[[@LINE-2]]:5: warning: 'other_call' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+    // CHECK-MESSAGES-NOT: :[[@LINE-2]]:5: warning: system call 'other_call' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
   }
   int M = memberInit();
 };
 
 int defaultInit() {
   std::other_call();
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'other_call' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: system call 'other_call' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
 }
 
 void testDefaultInit(int I = defaultInit()) {
 }
 
 void S::handler(int) {
   std::other_call();
 }

 void handler_signal(int) {
   // FIXME: It is only OK to call signal with the current signal number.
   std::signal(0, SIG_DFL);
   std::signal(0, SIG_IGN);
 }
 
 void handler_bad1(int) {
   std::other_call();
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'other_call' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: system call 'other_call' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
 }
 
 void handler_bad2(int) {
   std::SysStruct S;
   S << 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'operator<<' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: system call 'operator<<' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
 }
 
 void handler_bad3(int) {
   S S1(0);
 }
 
 void handler_bad4(int) {
   S S1;
   -S1;
 }
 
 void handler_bad5(int) {
   TestMemberInit MI;
 }
 
 void handler_bad6(int) {
   testDefaultInit();
 }
 
 void handler_bad7(int) {
   int I = []() { std::other_call(); return 2; }();
-  // 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]
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: system call 'other_call' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
 }
 }
 
 void handler_noexternc(int) {
   std::other_call();
 }
 
 void test() {
   std::signal(SIGINT, handler_abort);
   std::signal(SIGINT, handler__Exit);
   std::signal(SIGINT, handler_signal);
+
   std::signal(SIGINT, handler_bad1);
+
   std::signal(SIGINT, handler_bad2);
+
   // test call of user-defined operator
   std::signal(SIGINT, handler_bad3);
+
   // test call of constructor
   std::signal(SIGINT, handler_bad4);
+
   // test call of default member initializer
   std::signal(SIGINT, handler_bad5);
+
   // test call of default argument initializer
   std::signal(SIGINT, handler_bad6);
+
   // test lambda call in handler
   std::signal(SIGINT, handler_bad7);
 
   std::signal(SIGINT, handler_noexternc);
   // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: function 'handler_noexternc' should have C language linkage if used as signal handler [bugprone-signal-handler]
   // make a ExprWithCleanups
   std::signal(-S(), handler_noexternc);
   // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: function 'handler_noexternc' should have C language linkage if used as signal handler [bugprone-signal-handler]