[PATCH] D75229: [clang-tidy] Add signal-in-multithreaded-program check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.cpp:57
+    const MatchFinder::MatchResult &Result) {
+  bool IsPosix = PP->isMacroDefined("_POSIX_C_SOURCE") ||
+                 Result.Context->getTargetInfo().getTriple().getVendor() ==
----------------
Nothing for you to change here, but this demonstrates that `isLanguageVersionSupported()` isn't sufficient. We don't want to register the check at all for POSIX platforms in the same way we don't want to register checks at all based on the language standard being enforced, but we can't because we can't get to the `Preprocessor` or target information from there.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.cpp:68
+    const SourceManager &SM, Preprocessor *pp, Preprocessor *ModuleExpanderPP) {
+  PP = pp;
+}
----------------
abelkocsis wrote:
> whisperity wrote:
> > Global state is always a potential problem. Are you sure this is the right way to do things? Most other tidy checks that actively use this function define their own `PPCallback` subclass and use the existing preprocessor here to ensure the preprocessor executes the callback.
> I've used the same way in another check (https://reviews.llvm.org/D69181). I'm not entirely sure which is the better way to do.
Defining the `PPCallback` subclass is a better approach (https://github.com/llvm/llvm-project/blob/e40a742a5008c5a4cf647c0ea805486498786393/clang-tools-extra/clang-tidy/cert/SetLongJmpCheck.cpp#L49 shows a reasonable demonstration). At the very least, this doesn't have to set a global variable, it could set a member variable on `SignalInMultithreadedProgramCheck`.

I hadn't caught that in D69181, sorry about that! 


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.h:24
+/// check considers the analyzed program multithreaded if it finds at least
+/// one function call of the following: ``thrd_create``, ``std::thread``,
+/// ``boost::thread``, ``pthread_t``.
----------------
aaron.ballman wrote:
> The comment is a bit stale as it also accepts user-defined functions. Note, I think `CreateThread`, `CreateRemoteThread`, `_beginthread`, and `_beginthreadex` should be added to the list as those are the common Win32 APIs for creating threads (where pthreads aren't available).
The comment has gone stale again. Rather than listing the functions in the comment, I think it's fine to change it to: `... if it finds at least one threading-related function.`


================
Comment at: clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp:91
     // CON
+    CheckFactories.registerCheck<bugprone::SignalInMultithreadedProgramCheck>(
+        "cert-con37-c");
----------------
We try to keep these in order, so this should move down below con36-c.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D75229