[PATCH] D118370: [clang-tidy] bugprone-signal-handler: Code refactor (NFC)

[Balázs Kéri via Phabricator via cfe-commits]

balazske created this revision.
Herald added subscribers: carlosgalvezp, steakhal, martong, gamesh411, Szelethus, dkrupp, xazax.hun.
balazske requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D118370

Files:
  clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h


Index: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h
+++ clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h
@@ -33,14 +33,12 @@
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
+  bool checkFunction(const FunctionDecl *FD, const Expr *CallOrRef);
   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);
+  void reportHandlerChain(const llvm::df_iterator<clang::CallGraphNode *> &Itr,
+                          const FunctionDecl *HandlerDecl,
+                          const Expr *HandlerRef);
 
   clang::CallGraph CG;
 
Index: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
@@ -142,30 +142,41 @@
            "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);
+  if (!HandlerDecl->hasBody()) {
+    checkFunction(HandlerDecl, HandlerExpr);
     return;
   }
 
   CallGraphNode *HandlerNode = CG.getNode(HandlerDecl);
-  // Signal handler can be external but not unsafe, no call graph in this case.
-  if (!HandlerNode)
-    return;
+  assert(HandlerNode &&
+         "Handler has 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) {
     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);
+    if (CallF && CallF != HandlerDecl) {
+      if (checkFunction(
+              CallF, findCallExpr(Itr.getPath(Itr.getPathLength() - 2), *Itr)))
+        reportHandlerChain(Itr, HandlerDecl, HandlerExpr);
     }
   }
 }
 
+bool SignalHandlerCheck::checkFunction(const FunctionDecl *FD,
+                                       const Expr *CallOrRef) {
+  const bool FunctionIsCalled = isa<CallExpr>(CallOrRef);
+
+  if (!isFunctionAsyncSafe(FD)) {
+    diag(CallOrRef->getBeginLoc(), "%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;
+}
+
 bool SignalHandlerCheck::isFunctionAsyncSafe(const FunctionDecl *FD) const {
   if (isSystemCall(FD))
     return isSystemCallAsyncSafe(FD);
@@ -186,16 +197,8 @@
   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,
+void SignalHandlerCheck::reportHandlerChain(
+    const llvm::df_iterator<clang::CallGraphNode *> &Itr,
     const FunctionDecl *HandlerDecl, const Expr *HandlerRef) {
   int CallLevel = Itr.getPathLength() - 2;
   assert(CallLevel >= -1 && "Empty iterator?");


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D118370.403652.patch
Type: text/x-patch
Size: 4307 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220127/a238c26f/attachment.bin>