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

Display notes for a possible call chain if an unsafe function is found to be
called (maybe indirectly) from a signal handler.
The call chain displayed this way includes probably not the first calls of
the functions, but it is a valid possible (in non path-sensitive way) one.

Reviewed By: aaron.ballman

Differential Revision: https://reviews.llvm.org/D118224

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

@@ -145,7 +145,7 @@ void SignalHandlerCheck::check(const MatchFinder::MatchResult &Result) {
   // Check for special case when the signal handler itself is an unsafe external
   // function.
   if (!isFunctionAsyncSafe(HandlerDecl)) {
-    reportBug(HandlerDecl, HandlerExpr, SignalCall, HandlerDecl);
+    reportBug(HandlerDecl, HandlerExpr, /*DirectHandler=*/true);
     return;
   }
 
@@ -160,7 +160,8 @@ void SignalHandlerCheck::check(const MatchFinder::MatchResult &Result) {
     if (CallF && !isFunctionAsyncSafe(CallF)) {
       assert(Itr.getPathLength() >= 2);
       reportBug(CallF, findCallExpr(Itr.getPath(Itr.getPathLength() - 2), *Itr),
-                SignalCall, HandlerDecl);
+                /*DirectHandler=*/false);
+      reportHandlerCommon(Itr, SignalCall, HandlerDecl, HandlerExpr);
     }
   }
 }
@@ -186,17 +187,34 @@ bool SignalHandlerCheck::isSystemCallAsyncSafe(const FunctionDecl *FD) const {
 }
 
 void SignalHandlerCheck::reportBug(const FunctionDecl *CalledFunction,
-                                   const Expr *CallOrRef,
-                                   const CallExpr *SignalCall,
-                                   const FunctionDecl *HandlerDecl) {
+                                   const Expr *CallOrRef, bool DirectHandler) {
   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);
+       "%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.

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

@@ -11,6 +11,7 @@
 
 #include "../ClangTidyCheck.h"
 #include "clang/Analysis/CallGraph.h"
+#include "llvm/ADT/DepthFirstIterator.h"
 #include "llvm/ADT/StringSet.h"
 
 namespace clang {
@@ -35,9 +36,13 @@ class SignalHandlerCheck : public ClangTidyCheck {
   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 DirectHandler);
+  void reportHandlerCommon(llvm::df_iterator<clang::CallGraphNode *> Itr,
+                           const CallExpr *SignalCall,
+                           const FunctionDecl *HandlerDecl,
+                           const Expr *HandlerRef);
 
-  CallGraph CG;
+  clang::CallGraph CG;
 
   AsyncSafeFunctionSetType AsyncSafeFunctionSet;
   llvm::StringSet<> &ConformingFunctions;

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

@@ -13,92 +13,181 @@ int printf(const char *, ...);
 typedef void (*sighandler_t)(int);
 sighandler_t signal(int signum, sighandler_t handler);
 
-void handler_abort(int) {
-  abort();
-}
+void f_extern();
 
-void handler_other(int) {
+void handler_printf(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-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-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 handler_signal(int) {
-  // FIXME: It is only OK to call signal with the current signal number.
-  signal(0, SIG_DFL);
+void test_printf() {
+  signal(SIGINT, handler_printf);
 }
 
-void f_ok() {
-  abort();
+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-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 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]
+void test_extern() {
+  signal(SIGINT, handler_extern);
 }
 
-void f_extern();
+void f_ok() {
+  abort();
+}
 
 void handler_ok(int) {
   f_ok();
 }
 
+void test_ok() {
+  signal(SIGINT, handler_ok);
+}
+
+void f_bad() {
+  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-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 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_bad() {
+  signal(SIGINT, handler_bad);
+}
+
+void f_bad1() {
+  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-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() {
+  f_bad1();
+}
+
+void handler_bad1(int) {
+  f_bad2();
+  f_bad1();
+}
+
+void test_bad1() {
+  signal(SIGINT, handler_bad1);
 }
 
-void test_false_condition(int) {
+void handler_abort(int) {
+  abort();
+}
+
+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-MESSAGES: :[[@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: '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() {
+  signal(SIGINT, handler_false_condition);
 }
 
-void test_multiple_calls(int) {
+void handler_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]
+  // 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-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-MESSAGES: :[[@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: '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() {
+  signal(SIGINT, handler_multiple_calls);
+}
+
 void f_recursive();
 
-void test_recursive(int) {
+void handler_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]
+  // 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-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-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() {
+  signal(SIGINT, handler_recursive);
+}
+
+void f_multiple_paths() {
   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);
+  // 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-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 test() {
+void handler_multiple_paths(int) {
+  f_multiple_paths();
+  f_multiple_paths();
+}
+
+void test_multiple_paths() {
+  signal(SIGINT, handler_multiple_paths);
+}
+
+void handler_function_pointer(int) {
+  void (*fp)() = f_extern;
+  // Call with function pointer is not evalauted by the check.
+  (*fp)();
+}
+
+void test_function_pointer() {
+  signal(SIGINT, handler_function_pointer);
+}
+
+void test_other() {
   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-NOTES: :[[@LINE-1]]:18: warning: '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]
 
   signal(SIGINT, SIG_IGN);
   signal(SIGINT, SIG_DFL);
-
-  signal(SIGINT, test_false_condition);
-  signal(SIGINT, test_multiple_calls);
-  signal(SIGINT, test_recursive);
 }