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

[balazske]

Summary

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.
More details are added to the bug report messages.

[LegalizeAdulthood]

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

146

Why do we ignore the return value of checkFunction here?

Also, the name doesn't reveal to me why the result is true
or false, e.g. it acts like a predicate but its name doesn't ask
a question.

[balazske]

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

146

The function returns if a problem (warning) was found. At the next call site it is used to display additional notes, but here it is not needed. A better name can be isFunctionValidSignalHandler but the function checks not only a fact, it generates the warnings too.

151

There are problems with this approach in C++ code.

[njames93]

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

167

This can probably just be inlined into its only use.

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

36

This isn't a very descriptive name, and the bool return value conveys no meaning. Probably could do with some documentation.

46–49

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.

[balazske]

Even more simplification, added documentation.
Text of bug reports is more detailed, no NFC now.

[balazske]

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

151

The call graph adds nodes for the canonical declaration only. When getting a node it works only with the CanonicalDecl unless the CallGraph code is changed (it seems to be worth to do this change).

167

The value is used now multiple times. (But maybe the diag call can be put into a lambda later.)

[LegalizeAdulthood]

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

146

Can we put (void) checkFunction(...) here to make it clear
we're intentionally ignoring the return value?

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

37

... as a signal handler

38

Should test the properties ...

39–41

... from a signal

42

.. from the signal handler ...

43

... as a registered

45

... only if a problem ...

[balazske]

Corrected the comment problems, explained why return value of checkFunction is ignored.

[balazske]

Rebase

[balazske]

ping

[aaron.ballman]

Mostly small nits, but it looks like precommit CI is failing due to the changes:

Failed Tests (2):
  Clang Tools :: clang-tidy/checkers/bugprone-signal-handler-minimal.c
  Clang Tools :: clang-tidy/checkers/bugprone-signal-handler-posix.c

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

149

186

201

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

47

[balazske]

Applied review comments, fixed the failing tests.

[whisperity]

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

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

72–73

(Nit: There are a lot of indirections in the documentation that did not help understanding what is happening here.)

156–160

162–168

What does the return value of checkFunction signal to the user, and the if?

201

225

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.

225–226

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.

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

39–42

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.

45

46–49

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!)

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

14 ↗

(On Diff #411047)

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"

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

31 ↗

(On Diff #411047)

133 ↗

(On Diff #411047)

[balazske]

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

156–160

Is this code not too long to put in the condition section?

162–168

This has a description in the header file.

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

14 ↗

(On Diff #411047)

Call it "standard function"?

[whisperity]

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

156–160

The formatter will break it up appropriately, no problem with that.

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

14 ↗

(On Diff #411047)

Yes, that would be better.

[balazske]

Applied the review proposals.
Changed "system call" to "standard function".
Put the string lists into constant std::initializer_list.

[balazske]

Additional documentation related fixes.

[whisperity]

Alright, I think this is good to go. I like that it makes it clear that the called function is coming from some external source (system header or otherwise).

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

public:
 
   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 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,
 
   clang::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

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() &&
if (CG.size() <= 1) {
     // (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, /*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);
   // For external (not checkable) functions assume that these are unsafe.
   return FD->hasBody();
 }
 
 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;
 
   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?");
 
   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.
 // 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
 // mentioned POSIX specification was not updated after 'quick_exit' appeared
 // in the C11 standard.
 // Also, we want to keep the "minimal set" a subset of the "POSIX set".
 llvm::StringSet<> SignalHandlerCheck::POSIXConformingFunctions{