[PATCH] D87449: [clang-tidy] Add new check for SEI CERT rule SIG30-C.

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:31
+static bool isSystemCall(const FunctionDecl *FD) {
+  // This check does not work with function calls in std namespace.
+  if (!FD->isGlobal() || FD->isInStdNamespace())
----------------
Why? In //C++// we have everything in `std` namespace, such as `std::signal()`, `std::abort()` or `std::_Exit()`. In `C++` the general rule is to use them instead of the global //C// variants.


================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:35
+  // It is assumed that the function has no other re-declaration that is not
+  // in a system header. Otherwise this may produce wrong result.
+  return FD->getASTContext().getSourceManager().isInSystemHeader(
----------------
The assumption is basically right, we do not repeat declarations from system headers but maybe we could loop over the redeclaration chain.


================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:40
+
+static bool isAllowedSystemCall(const FunctionDecl *FD) {
+  if (!FD->getIdentifier())
----------------
The name suggests that this function checks for both //system call// and //allowed call//. I would either rename this function to simply `isAllowedCall()` or at least put an assertion to the beginning: `assert(isSystemCall(FD));`.


================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:44
+  const StringRef N = FD->getName();
+  if (N == AbortFun || N == ExitFun || N == QuickExitFun || N == SignalFun)
+    return true;
----------------
Maybe you could use `IdentifierInfo` instead of string comparisons.


================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:81
+      callExpr(IsSignalFunction, HandlerAsSecondArg).bind("register_call"),
+      this);
+}
----------------
More readable would be this way:
```
const auto HandlerExpr = declRefExpr(hasDeclaration(functionDecl().bind("handler_decl")),
   unless(isExpandedFromMacro("SIG_IGN")),
   unless(isExpandedFromMacro("SIG_DFL")))
      .bind("handler_expr");
Finder->addMatcher(
   callExpr(IsSignalFunction, hasArgument(1, HandlerExpr)).bind("register_call"),
   this);
```


================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:95
+  std::deque<std::pair<const FunctionDecl *, const Expr *>> CalledFunctions{
+      {HandlerDecl, HandlerExpr}};
+
----------------
Do we really need to store `FunctionDecl` in the map? The whole code would be much simpler if you only store the call expression and the retrieve the callee declaration once at the beginning of the loop body. Beside simplicity this would also reduce the memory footprint and surely not increase the execution time.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst:1-17
+.. title:: clang-tidy - cert-sig30-c
+
+cert-sig30-c
+============
+
+Finds functions registered as signal handlers that call non asynchronous-safe
+functions. User functions called from the handlers are checked too, as far as
----------------
Please add at least one minimal code example. (E.g. from the tests.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449