Page MenuHomePhabricator

[clang-tidy] Add signal-in-multithreaded-program check
Needs ReviewPublic

Authored by abelkocsis on Feb 27 2020, 12:55 AM.

Details

Summary

According to
https://wiki.sei.cmu.edu/confluence/display/c/CON37-C.+Do+not+call+signal%28%29+in+a+multithreaded+program
bugprone-signal-in-multithreaded-program check is created.
The check finds signal function call when the program is
multithreaded.

Diff Detail

Event Timeline

abelkocsis created this revision.Feb 27 2020, 12:55 AM
lebedev.ri retitled this revision from Add signal-in-multithreaded-program check to [clang-tidy] Add signal-in-multithreaded-program check.Feb 27 2020, 1:00 AM

Will be good idea to make callers function list configurable through option. See other checks as example.

clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.cpp
21

Unnecessary empty line.

clang-tools-extra/docs/ReleaseNotes.rst
135

Please add empty line above.

clang-tools-extra/docs/clang-tidy/checks/bugprone-signal-in-multithreaded-program.rst
13

Should be double colon.

19

Please enclose signal() in double back-ticks.

abelkocsis updated this revision to Diff 247833.Mar 3 2020, 2:28 AM
abelkocsis marked 4 inline comments as done.

Configurable callers option and new test cases add.

steakhal added inline comments.Mar 4 2020, 5:34 AM
clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.cpp
34–51

I apologize for interrupting the review.
Should we use hasDescendant for matching like everything in translation unit?

Wouldn't it be more performant to collect all the signal calls in a set and set a bool variable if a ThreadList function seen?
At the end of the analysis of the translation unit, emit warnings for each recorded signal call if the variable was set (aka. we are in multithreaded environment).

Treat this comment rather a question since I'm not really familiar with clang-tidy.

abelkocsis marked 2 inline comments as done.Apr 5 2020, 8:30 AM
abelkocsis added inline comments.
clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.cpp
34–51

I updated the checker not exactly in that way you mentioned, but I think you are right that we should not match for all translationUnitDecl.

abelkocsis updated this revision to Diff 255169.Apr 5 2020, 8:30 AM
abelkocsis marked an inline comment as done.
Charusso added inline comments.Apr 5 2020, 8:51 AM
clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.cpp
34–51

Every single check runs on the whole TranslationUnitDecl, and the matcher will try to match on every of the TU's descendant, that is why it emit multiple reports in the same single run. There is no need to use translationUnitDecl() and nor to use forEachDescendant().

39

You can emit the binding of thread.

57

singal -> signal

mgehre removed a subscriber: mgehre.Apr 5 2020, 10:39 AM
abelkocsis updated this revision to Diff 255250.Apr 6 2020, 1:22 AM
abelkocsis marked 3 inline comments as done.

Small fixes

gamesh411 added inline comments.
clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.h
24

founds -> finds, but I would rather reword it like this:
The checker considers the analyzed program multithreaded if it finds ....

clang-tools-extra/docs/ReleaseNotes.rst
101

Same as before.

Minor nits inline. Good job, I am not entitled to accept it, but LGTM!

clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.h
24

Or better:
The check considers ...
as the term checker is used more in ClangSA, and check in clang-tidy.

Eugene.Zelenko added inline comments.Jun 17 2020, 6:45 AM
clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.h
24

Clang-tidy uses check.

abelkocsis marked 4 inline comments as done.

Fixes on description and documentation

clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
89

Please use check here.

abelkocsis marked an inline comment as done.Jun 18 2020, 10:02 AM
abelkocsis added inline comments.
clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
89

Should I replace the // C++ checkers comment too? I mean to // C++ checks instead of // C++ checkers?

clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
89

Sure. It's good idea to keep user and developer terminology consistent.

abelkocsis marked 2 inline comments as done.

Small comment fix

dkrupp added a subscriber: dkrupp.Jul 3 2020, 5:13 AM

In general, the test files should be cleaned up.

clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-in-multithreaded-program-config-std::thread.cpp
1 ↗(On Diff #271789)

Please rename this file to something that doesn't conatain : in the name. Windows peoples' computers will explode if it's merged like this...

5–6 ↗(On Diff #271789)

Are these definitions needed for C++ test?

32 ↗(On Diff #271789)

As far as I know, int main(void) is not valid C++ code. Only int main() and int main(int, char**) are valid, see [basic.start.main]/2.

If we are writing C++ test code, let's make it appear as if it was C++ code.

clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-in-multithreaded-program-noconfig-std::thread.cpp
1 ↗(On Diff #271789)

Same rename here.

clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-in-multithreaded-program-noconfig-thrd_create.cpp
1 ↗(On Diff #271789)

thrd_create is a C standard function, and this file looks like a C file to me, so why is the extension .cpp?

jfb added a comment.Jul 9 2020, 8:36 AM

It seems like you don't want this check to trigger on POSIX platforms, given:

Exceptions CON37-C-EX1: Implementations such as POSIX that provide defined behavior when multithreaded programs use custom signal handlers are exempt from this rule [IEEE Std 1003.1-2013].

abelkocsis updated this revision to Diff 282472.Aug 2 2020, 1:05 PM
abelkocsis marked 5 inline comments as done.

Fixes

I have managed to improve the checker, but could not set up the test files to work only in POSIX platforms. Could you help me or show me an example?

I have managed to improve the checker, but could not set up the test files to work only in POSIX platforms. Could you help me or show me an example?

It's more so that, if I understand correctly, you don't want the diagnostics to be emitted when dealing with POSIX multithreading.

Also, have you checked in the test if __unix__ is defined? I'm not seeing any indication of targets in the test configuration, nor checking for this macro. Maybe a Windows-specific test file should be added? clang/test/Preprocessor/init.c lists a bunch of macros that are supposedly pre-defined based on the target.

Or maybe the OSType in the triple would be more useful than the vendor?

clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.cpp
69

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.

clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
50–85

Please upload the diff with full context.

clang-tools-extra/docs/clang-tidy/checks/list.rst
102

There is a table in this file towards the end where alias checks are explicitly listed in a "which check is alias of which other check" format.

aaron.ballman added inline comments.Tue, Nov 10, 12:43 PM
clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.cpp
42

::std::thread

48

::signal

57

This does not seem like the right way to check for POSIX -- isn't that _POSIX_C_SOURCE?

clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.h
25

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

35

I think that should be ::std::thread and ::boost::thread to avoid pathological naming issues.

abelkocsis marked 4 inline comments as done.

Some fixes. Needs further improvement

abelkocsis marked 3 inline comments as done.

Fixes

abelkocsis added inline comments.Mon, Nov 16, 2:18 AM
clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.cpp
69

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.

aaron.ballman added inline comments.Thu, Nov 19, 7:54 AM
clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.cpp
58

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.

69

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!

clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.h
25

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.

clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
91

We try to keep these in order, so this should move down below con36-c.

abelkocsis marked 4 inline comments as done.

Fixes, PPCallbacks class add

jfb added a comment.Thu, Nov 19, 3:00 PM

Most of the tests as written should be failing right now, at least on macOS and Linux, because they likely should be identified as POSIX, right?

Fix, MacroName == "__unix__" seems really necessary. Without that, it throws a warning for me on Linux.

In D75229#2406758, @jfb wrote:

Most of the tests as written should be failing right now, at least on macOS and Linux, because they likely should be identified as POSIX, right?

Yes, now it fails on my system too. What would be the solution?