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

[Whisperity via Phabricator via cfe-commits]

whisperity added a reviewer: whisperity.
whisperity added a comment.
Herald added a subscriber: rnkovacs.

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



================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:72-73
 
 /// 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
----------------
(Nit: There are a lot of indirections in the documentation that did not help understanding what is happening here.)


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:159-160
        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) {
+      unsigned int PathL = Itr.getPathLength();
----------------



================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:165-171
+      if (checkFunction(CallF, CallOrRef))
+        reportHandlerChain(Itr, HandlerExpr);
     }
   }
 }
 
+bool SignalHandlerCheck::checkFunction(const FunctionDecl *FD,
----------------
What does the return value of `checkFunction` signal to the user, and the `if`?


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:188
+  if (!FD->hasBody()) {
+    diag(CallOrRef->getBeginLoc(), "cannot verify if external function %0 is "
+                                   "asynchronous-safe; "
----------------



================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:239
 // https://wiki.sei.cmu.edu/confluence/display/c/SIG30-C.+Call+only+asynchronous-safe+functions+within+signal+handlers
-llvm::StringSet<> SignalHandlerCheck::MinimalConformingFunctions{
+const llvm::StringSet<> SignalHandlerCheck::MinimalConformingFunctions{
     "signal", "abort", "_Exit", "quick_exit"};
----------------
whisperity wrote:
> 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.
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.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:239-240
 // https://wiki.sei.cmu.edu/confluence/display/c/SIG30-C.+Call+only+asynchronous-safe+functions+within+signal+handlers
-llvm::StringSet<> SignalHandlerCheck::MinimalConformingFunctions{
+const llvm::StringSet<> SignalHandlerCheck::MinimalConformingFunctions{
     "signal", "abort", "_Exit", "quick_exit"};
 
----------------
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.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h:48-49
   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);
+  /// Add bug report notes to show the call chain of functions from a signal
+  /// handler to an actual called function (from it).
+  /// @param Itr Position during a call graph depth-first iteration. It contains
----------------
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.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h:54
+  /// @param HandlerRef Reference to the signal handler function where it is
+  /// registered (considered as first part of the call chain).
+  void reportHandlerChain(const llvm::df_iterator<clang::CallGraphNode *> &Itr,
----------------



================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h:46-49
   llvm::StringSet<> &ConformingFunctions;
 
   static llvm::StringSet<> MinimalConformingFunctions;
   static llvm::StringSet<> POSIXConformingFunctions;
----------------
njames93 wrote:
> 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.
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!)


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler-posix.c:14
   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]
 }
----------------
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"//


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c:31
   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 if 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'
----------------



================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c:133
   printf("");
   // first 'printf' call (in other function) found only
 }
----------------



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118370/new/

https://reviews.llvm.org/D118370