Index: clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp =================================================================== --- clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp +++ clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp @@ -15,6 +15,7 @@ #include "../misc/NewDeleteOverloadsCheck.h" #include "../misc/NonCopyableObjects.h" #include "../misc/StaticAssertCheck.h" +#include "../misc/SpuriouslyWakeUpFunctionsCheck.h" #include "../misc/ThrowByValueCatchByReferenceCheck.h" #include "../performance/MoveConstructorInitCheck.h" #include "../readability/UppercaseLiteralSuffixCheck.h" @@ -39,6 +40,9 @@ public: void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { // C++ checkers + // CON + CheckFactories.registerCheck( + "cert-con54-cpp"); // DCL CheckFactories.registerCheck( "cert-dcl21-cpp"); @@ -71,6 +75,9 @@ "cert-oop54-cpp"); // C checkers + // CON + CheckFactories.registerCheck( + "cert-con36-c"); // DCL CheckFactories.registerCheck("cert-dcl03-c"); CheckFactories.registerCheck( Index: clang-tools-extra/clang-tidy/misc/CMakeLists.txt =================================================================== --- clang-tools-extra/clang-tidy/misc/CMakeLists.txt +++ clang-tools-extra/clang-tidy/misc/CMakeLists.txt @@ -8,6 +8,7 @@ NonCopyableObjects.cpp NonPrivateMemberVariablesInClassesCheck.cpp RedundantExpressionCheck.cpp + SpuriouslyWakeUpFunctionsCheck.cpp StaticAssertCheck.cpp ThrowByValueCatchByReferenceCheck.cpp UnconventionalAssignOperatorCheck.cpp Index: clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp =================================================================== --- clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp +++ clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp @@ -15,6 +15,7 @@ #include "NonCopyableObjects.h" #include "NonPrivateMemberVariablesInClassesCheck.h" #include "RedundantExpressionCheck.h" +#include "SpuriouslyWakeUpFunctionsCheck.h" #include "StaticAssertCheck.h" #include "ThrowByValueCatchByReferenceCheck.h" #include "UnconventionalAssignOperatorCheck.h" @@ -41,6 +42,8 @@ "misc-non-private-member-variables-in-classes"); CheckFactories.registerCheck( "misc-redundant-expression"); + CheckFactories.registerCheck( + "misc-spuriously-wake-up-functions"); CheckFactories.registerCheck("misc-static-assert"); CheckFactories.registerCheck( "misc-throw-by-value-catch-by-reference"); Index: clang-tools-extra/clang-tidy/misc/SpuriouslyWakeUpFunctionsCheck.h =================================================================== --- /dev/null +++ clang-tools-extra/clang-tidy/misc/SpuriouslyWakeUpFunctionsCheck.h @@ -0,0 +1,35 @@ +//===--- SpuriouslyWakeUpFunctionsCheck.h - clang-tidy ----------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SPURIOUSLYWAKEUPFUNCTIONSCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SPURIOUSLYWAKEUPFUNCTIONSCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang { +namespace tidy { +namespace misc { + +/// Finds ``cnd_wait`` or `wait` function calls in an ``IfStmt`` and tries to +/// replace it with ``WhileStm``. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/misc-spuriously-wake-up-functions.html +class SpuriouslyWakeUpFunctionsCheck : public ClangTidyCheck { +public: + SpuriouslyWakeUpFunctionsCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace misc +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SPURIOUSLYWAKEUPFUNCTIONSCHECK_H Index: clang-tools-extra/clang-tidy/misc/SpuriouslyWakeUpFunctionsCheck.cpp =================================================================== --- /dev/null +++ clang-tools-extra/clang-tidy/misc/SpuriouslyWakeUpFunctionsCheck.cpp @@ -0,0 +1,87 @@ +//===--- SpuriouslyWakeUpFunctionsCheck.cpp - clang-tidy ------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "SpuriouslyWakeUpFunctionsCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace misc { + +void SpuriouslyWakeUpFunctionsCheck::registerMatchers(MatchFinder *Finder) { + + if (getLangOpts().CPlusPlus) { + // Check for `CON54-CPP` + auto hasUniqueLock = hasDescendant(declRefExpr(hasDeclaration( + varDecl(hasType(asString("std::unique_lock")))))); + + auto hasWaitDescendantCPP = hasDescendant( + cxxMemberCallExpr( + anyOf( + allOf(hasDescendant(memberExpr(hasDeclaration(functionDecl( + allOf(hasName("std::condition_variable::wait"), + parameterCountIs(1)))))), + onImplicitObjectArgument(declRefExpr(to(varDecl( + hasType(asString("std::condition_variable &")))))), + hasUniqueLock), + allOf(hasDescendant(memberExpr(hasDeclaration(functionDecl( + allOf(hasName("std::condition_variable::wait_for"), + parameterCountIs(2)))))), + onImplicitObjectArgument(declRefExpr(to(varDecl( + hasType(asString("std::condition_variable &")))))), + hasUniqueLock), + allOf(hasDescendant(memberExpr(hasDeclaration(functionDecl( + allOf(hasName("std::condition_variable::wait_until"), + parameterCountIs(2)))))), + onImplicitObjectArgument(declRefExpr(to(varDecl( + hasType(asString("std::condition_variable &")))))), + hasUniqueLock) + + )) + .bind("wait")); + + Finder->addMatcher( + ifStmt( + allOf(hasWaitDescendantCPP, + unless(anyOf(hasDescendant(ifStmt(hasWaitDescendantCPP)), + hasDescendant(whileStmt(hasWaitDescendantCPP)), + hasDescendant(forStmt(hasWaitDescendantCPP)), + hasDescendant(doStmt(hasWaitDescendantCPP)))))), + this); + } else { + // Check for `CON36-C` + auto hasWaitDescendantC = + hasDescendant(callExpr(callee(functionDecl(allOf(hasName("cnd_wait"), + parameterCountIs(2))))) + .bind("wait")); + Finder->addMatcher( + ifStmt(allOf(hasWaitDescendantC, + unless(anyOf(hasDescendant(ifStmt(hasWaitDescendantC)), + hasDescendant(whileStmt(hasWaitDescendantC)), + hasDescendant(forStmt(hasWaitDescendantC)), + hasDescendant(doStmt(hasWaitDescendantC)))))), + this); + } +} + +void SpuriouslyWakeUpFunctionsCheck::check( + const MatchFinder::MatchResult &Result) { + + const auto *MatchedWait = Result.Nodes.getNodeAs("wait"); + diag( + MatchedWait->getExprLoc(), + "'%0' should be placed inside a while statement or used with a condition " + "parameter") + << MatchedWait->getDirectCallee()->getName(); +} +} // namespace misc +} // namespace tidy +} // namespace clang Index: clang-tools-extra/docs/ReleaseNotes.rst =================================================================== --- clang-tools-extra/docs/ReleaseNotes.rst +++ clang-tools-extra/docs/ReleaseNotes.rst @@ -94,6 +94,16 @@ Without the null terminator it can result in undefined behaviour when the string is read. +- New alias :doc:`cert-con36-c + ` to + :doc:`misc-spuriously-wake-up-functions + ` was added. + +- New alias :doc:`cert-con54-cpp + ` to + :doc:`misc-spuriously-wake-up-functions + ` was added. + - New :doc:`cert-mem57-cpp ` check. @@ -179,6 +189,13 @@ Finds non-static member functions that can be made ``const`` because the functions don't use ``this`` in a non-const way. +- New :doc:`misc-spuriously-wake-up-functions + ` check. + + Finds ``cnd_wait`` or ``wait`` function calls when the function is not + invoked from a loop that checks whether a condition predicate holds or the + function has a condition parameter. + - Improved :doc:`modernize-use-override ` check. Index: clang-tools-extra/docs/clang-tidy/checks/cert-con36-c.rst =================================================================== --- /dev/null +++ clang-tools-extra/docs/clang-tidy/checks/cert-con36-c.rst @@ -0,0 +1,10 @@ +.. title:: clang-tidy - cert-con36-c +.. meta:: +:http-equiv=refresh: 5;URL=misc-spuriously-wake-up-functions.html + +cert-con36-c +============ + +The cert-con36-c check is an alias, please see +`misc-spuriously-wake-up-functions `_ +for more information. Index: clang-tools-extra/docs/clang-tidy/checks/cert-con54-cpp.rst =================================================================== --- /dev/null +++ clang-tools-extra/docs/clang-tidy/checks/cert-con54-cpp.rst @@ -0,0 +1,10 @@ +.. title:: clang-tidy - cert-con54-cpp +.. meta:: +:http-equiv=refresh: 5;URL=misc-spuriously-wake-up-functions.html + +cert-con54-cpp +============== + +The cert-con54-cpp check is an alias, please see +`misc-spuriously-wake-up-functions `_ +for more information. Index: clang-tools-extra/docs/clang-tidy/checks/list.rst =================================================================== --- clang-tools-extra/docs/clang-tidy/checks/list.rst +++ clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -84,6 +84,8 @@ bugprone-unused-return-value bugprone-use-after-move bugprone-virtual-near-miss + cert-con36-c (redirects to misc-spuriously-wake-up-functions) + cert-con54-cpp (redirects to misc-spuriously-wake-up-functions) cert-dcl03-c (redirects to misc-static-assert) cert-dcl16-c (redirects to readability-uppercase-literal-suffix) cert-dcl21-cpp @@ -290,6 +292,7 @@ misc-non-copyable-objects misc-non-private-member-variables-in-classes misc-redundant-expression + misc-spuriously-wake-up-functions misc-static-assert misc-throw-by-value-catch-by-reference misc-unconventional-assign-operator Index: clang-tools-extra/docs/clang-tidy/checks/misc-spuriously-wake-up-functions.rst =================================================================== --- /dev/null +++ clang-tools-extra/docs/clang-tidy/checks/misc-spuriously-wake-up-functions.rst @@ -0,0 +1,27 @@ +.. title:: clang-tidy - misc-spuriously-wake-up-functions + +misc-spuriously-wake-up-functions +================================= + +Finds ``cnd_wait`` or ``wait`` function calls in an ``IfStmt`` and tries to +replace it with ``WhileStmt``. + +.. code-block: c++ + + if (condition_predicate) { + condition.wait(lk); + } + +.. code-block: c + + if (condition_predicate) { + if (thrd_success != cnd_wait(&condition, &lock)) { + } + } + +This check corresponds to the CERT C++ Coding Standard rule +`CON54-CPP. Wrap functions that can spuriously wake up in a loop +`_. +and CERT C Coding Standard rule +`CON36-C. Wrap functions that can spuriously wake up in a loop +`_. Index: clang-tools-extra/test/clang-tidy/misc-spuriously-wake-up-functions.cpp =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/misc-spuriously-wake-up-functions.cpp @@ -0,0 +1,50 @@ +// RUN: %check_clang_tidy %s misc-spuriously-wake-up-functions %t -- -- -I %S/../../../libcxx/include/ + +#include "chrono" +#include "condition_variable" +#include "mutex" + +struct Node1 { + void *Node1; + struct Node1 *next; +}; + +static Node1 list; +static std::mutex m; +static std::condition_variable condition; + +void consume_list_element(std::condition_variable &condition) { + std::unique_lock lk(m); + + if (list.next == nullptr) { + condition.wait(lk); + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'wait' should be placed inside a while statement or used with a condition parameter [misc-spuriously-wake-up-functions] + } + + while (list.next == nullptr) { + condition.wait(lk); + } + + if (list.next == nullptr) { + while (list.next == nullptr) { + condition.wait(lk); + } + } + using durtype = std::chrono::duration; + durtype dur = std::chrono::duration(); + if (list.next == nullptr) { + condition.wait_for(lk, dur); + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'wait_for' should be placed inside a while statement or used with a condition parameter [misc-spuriously-wake-up-functions] + } + if (list.next == nullptr) { + condition.wait_for(lk, dur, [] { return 1; }); + } + auto now = std::chrono::system_clock::now(); + if (list.next == nullptr) { + condition.wait_until(lk, now + dur); + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'wait_until' should be placed inside a while statement or used with a condition parameter [misc-spuriously-wake-up-functions] + } + if (list.next == nullptr) { + condition.wait_until(lk, now + dur, [] { return 1; }); + } +}