aaron.ballman added inline comments.
================ Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:31-33 + // This check does not work with function calls in std namespace. + if (!FD->isGlobal() || FD->isInStdNamespace()) + return false; ---------------- baloghadamsoftware wrote: > 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. This seems incorrect to me. `::std::quick_exit()` resolves to `::quick_exit()`, so why should that call be ignored as a system call? I would assume that anything in namespace `std` is a system call. It's a bit questionable whether a user-written template specialization in namespace `std` should be handled that way, but I think that's still reasonable to consider as a system call. ================ Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:34 + return false; + // It is assumed that the function has no other re-declaration that is not + // in a system header. Otherwise this may produce wrong result. ---------------- re-declaration -> redeclaration ================ Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:41 +static bool isAllowedSystemCall(const FunctionDecl *FD) { + if (!FD->getIdentifier()) + return true; ---------------- A function without an identifier is not a system call, so I would have expected this to return `false` based on the function name. ================ Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:43 + return true; + const StringRef N = FD->getName(); + if (N == AbortFun || N == ExitFun || N == QuickExitFun || N == SignalFun) ---------------- We don't typically use top-level `const` in the project, so this can be dropped. Same comment applies elsewhere in the patch. ================ Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:44-46 + if (N == AbortFun || N == ExitFun || N == QuickExitFun || N == SignalFun) + return true; + return false; ---------------- baloghadamsoftware wrote: > Maybe you could use `IdentifierInfo` instead of string comparisons. The logic isn't quite correct here as this will claim to be an allowed system call: ``` namespace awesome { void quick_exit(void); // Considers this to be a system call } ``` You should be checking the namespace as well. Also, this list is very incomplete depending on your platform. The CERT rule lists a whole bunch of POSIX functions that are required to be async signal safe. I am guessing Microsoft likely has a list somewhere as well. I worry about the number of false positives this check will issue if we don't at least consider POSIX (where signals are much, much more useful than in strictly conforming C). ================ Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:73 + const auto IsSignalFunction = + callee(functionDecl(hasName(SignalFun), parameterCountIs(2))); + const auto HandlerAsSecondArg = hasArgument( ---------------- Similar issue here with finding functions in the wrong namespace. ================ Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:117-118 + diag(FunctionCall->getBeginLoc(), + "'%0' is considered as non asynchronous-safe and " + "should not be called from a signal handler") + << FunctionToCheck->getName(); ---------------- How about: `'%0' may not be asynchronous-safe; calling it from a signal handler may be dangerous`? ================ Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:135 + [&CalledFunctions](const FunctionDecl *FD, const CallExpr *CE) { + CalledFunctions.push_back(std::make_pair(FD, CE)); + }}; ---------------- `emplace_back(FD, CE)`? ================ Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst:13 +(for ``signal`` there are additional conditions that are not checked). +Every other system call is considered as non asynchronous-safe by the checker. + ---------------- I would document this as: `Any function that cannot be determined to be an asynchronous-safe function call is assumed to be non-asynchronous-safe by the checker, including function calls for which only the declaration of the called function is visible.` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87449/new/ https://reviews.llvm.org/D87449 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits