Page MenuHomePhabricator

[clang-tidy] Add spuriously-wake-up-functions check
ClosedPublic

Authored by abelkocsis on Dec 1 2019, 10:05 AM.

Details

Summary

According to
https://wiki.sei.cmu.edu/confluence/display/cplusplus/CON54-CPP.+Wrap+functions+that+can+spuriously+wake+up+in+a+loop
and
https://wiki.sei.cmu.edu/confluence/display/c/CON36-C.+Wrap+functions+that+can+spuriously+wake+up+in+a+loop
misc-spuriously-wake-up-functions check is created. The check finds
cnd_wait or wait function calls in an IfStmt and warns the user to
replace it with a WhileStmt or use it with a lambda parameter.

Diff Detail

Event Timeline

abelkocsis created this revision.Dec 1 2019, 10:05 AM
abelkocsis edited the summary of this revision. (Show Details)Dec 1 2019, 10:06 AM
Charusso retitled this revision from Add spuriously-wake-up-functions check to [clang-tidy] Add spuriously-wake-up-functions check.

I don't think that misc module should be used when checks belong to cert

clang-tools-extra/clang-tidy/misc/SpuriouslyWakeUpFunctionsCheck.cpp
77 ↗(On Diff #231616)

Unnecessary empty line.

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

Please move into aliases section (in alphabetical order). Same below.

163

Please use alphabetical order.

166

Please synchronize with first statement in documentation.

clang-tools-extra/test/clang-tidy/misc-spuriously-wake-up-functions.cpp
1 ↗(On Diff #231616)

What will happen if libcxx is not part build/source tree?

aaron.ballman added inline comments.Dec 2 2019, 8:22 AM
clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
45–46 ↗(On Diff #231616)

If we want to expose this check outside of the CERT module, I think it should go into bugprone rather than misc.

clang-tools-extra/clang-tidy/misc/SpuriouslyWakeUpFunctionsCheck.cpp
62 ↗(On Diff #231616)

What about cnd_timedwait?

clang-tools-extra/clang-tidy/misc/SpuriouslyWakeUpFunctionsCheck.h
19 ↗(On Diff #231616)

WhileStm -> WhileStmt

clang-tools-extra/test/clang-tidy/misc-spuriously-wake-up-functions.cpp
1 ↗(On Diff #231616)

Also, there are no tests for the C functionality yet.

mgehre removed a subscriber: mgehre.Dec 10 2019, 1:18 PM
abelkocsis marked 7 inline comments as done.Dec 23 2019, 2:36 PM

Checker is moved to bugprone section, tests added. C checker is improved, and documentations are synchronised.

Eugene.Zelenko added inline comments.Dec 23 2019, 2:50 PM
clang-tools-extra/docs/clang-tidy/checks/bugprone-spuriously-wake-up-functions.rst
5

Please make length same as in title.

abelkocsis marked 4 inline comments as done.Dec 24 2019, 3:34 AM

Adding matcher for cnd_timedwait and test cases.
Small fix in documentation

The checks.rst has recently been updated to show "normal" (main entry) checks and "aliases" in different tables in D36051. Please register the alias-ness accordingly.

abelkocsis updated this revision to Diff 235232.EditedDec 24 2019, 12:50 PM

docs/clang-tidy/checks/lsit.rst update

sylvestre.ledru added inline comments.
clang-tools-extra/docs/clang-tidy/checks/list.rst
12

Could you try to evaluate the severity? :)
thanks

aaron.ballman added inline comments.Dec 26 2019, 9:25 AM
clang-tools-extra/clang-tidy/bugprone/SpuriouslyWakeUpFunctionsCheck.cpp
23

These should all use ::std::whatever to guard against pathological code that uses std inside of another namespace.

81

This should be simplified into:

const auto *MatchedWait = Result.Nodes.getNodeAs<CallExpr>("wait");
StringRef WaitName = MatchedWait->getDirectCallee()->getName();
diag(MatchedWait->getExprLoc(), "'%0' should be placed inside a while statement %select{|or used with a conditional parameter}1") << WaitName << (WaitName != "cnd_wait" && WaitName != cnd_timedwait);
clang-tools-extra/docs/clang-tidy/checks/bugprone-spuriously-wake-up-functions.rst
7

Missing cnd_timedwait

clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/chrono.h
49 ↗(On Diff #235232)

Please add a newline to the end of the file.

clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/condition_variable.h
41 ↗(On Diff #235232)

Please add a newline to the end of the file.

clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/cstdint.h
4 ↗(On Diff #235232)

Please add a newline to the end of the file.

clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/mutex.h
20 ↗(On Diff #235232)

Please add a newline to the end of the file.

clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/ratio.h
29 ↗(On Diff #235232)

Please add a newline to the end of the file.

clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp
2 ↗(On Diff #235232)

Are you planning to use these declarations in multiple test files? If not, then we typically put all of the header code into the test file rather than added as explicit files.

99 ↗(On Diff #235232)

You are missing test cases that use other kinds of loops, like a do..while or for loop.

abelkocsis marked 11 inline comments as done.Dec 28 2019, 2:22 AM
abelkocsis added inline comments.
clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp
2 ↗(On Diff #235232)

I'm not sure yet, so I move it into the test file.

abelkocsis marked an inline comment as done.

Checker fixes, updates test file adding test cases for doWhile and for statements, test headers deleted.

aaron.ballman added inline comments.Dec 28 2019, 6:18 AM
clang-tools-extra/clang-tidy/bugprone/SpuriouslyWakeUpFunctionsCheck.cpp
61

Sorry for not noticing this earlier, but I think you can make two calls to addMatcher(), one for the C check and one for the C++ check. Then you can register only the matcher needed for the language mode you are currently in. e.g.,

if (getLangOpts().CPlusPlus)
  // Check for CON54-CPP
else
  // Check for CON36-C

This should make the check slightly more efficient because of the mutually exclusive nature of these APIs without changing the behavior.

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

Given that these are essentially CERT rules, I'd go with the CERT severity for them -- however, I say that only because I'm not certain what approach is supposed to be taken for these severity markings.

CERT marks these two rules as having a low severity, FWIW.

abelkocsis marked 3 inline comments as done.Feb 11 2020, 2:40 AM

Checker update to analyse language, separated checks for C and C++.

steakhal added inline comments.Feb 11 2020, 11:23 PM
clang-tools-extra/docs/clang-tidy/checks/list.rst
304–305

In case the checker does not offer automatic fixes, remove "Yes" from the given table entries.

abelkocsis marked an inline comment as done.

docs/list.rst update

aaron.ballman accepted this revision.Feb 12 2020, 11:25 AM

LGTM aside from a test coverage request.

clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.c
26–30 ↗(On Diff #244098)

Can you add a test case to demonstrate the behavior when there's not a compound statement involved? And another one for a slightly more complex if predicate? e.g.,

if (list_c.next == NULL)
  if (0 != cnd_wait(&condition_c, &lock))
    ;

if (list_c.next == NULL && 0 != cnd_wait(&condition_c, &lock))
  ;

Both of these should be flagged similar to the form with the compound statement.

This revision is now accepted and ready to land.Feb 12 2020, 11:25 AM

Test cases adding, checker modifying to pass new cases.

aaron.ballman accepted this revision.Feb 18 2020, 2:15 PM

LGTM, do you need me to commit on your behalf?

clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.c
165 ↗(On Diff #245113)

Please add a newline to the end of the file.

abelkocsis marked an inline comment as done.Mar 21 2020, 2:29 AM
This revision was automatically updated to reflect the committed changes.