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
142

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