Index: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp =================================================================== --- clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -38,6 +38,7 @@ #include "PosixReturnCheck.h" #include "SizeofContainerCheck.h" #include "SizeofExpressionCheck.h" +#include "SpuriouslyWakeUpFunctionsCheck.h" #include "StringConstructorCheck.h" #include "StringIntegerAssignmentCheck.h" #include "StringLiteralWithEmbeddedNulCheck.h" @@ -123,6 +124,8 @@ "bugprone-sizeof-container"); CheckFactories.registerCheck( "bugprone-sizeof-expression"); + CheckFactories.registerCheck( + "bugprone-spuriously-wake-up-functions"); CheckFactories.registerCheck( "bugprone-string-constructor"); CheckFactories.registerCheck( Index: clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt =================================================================== --- clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -30,6 +30,7 @@ PosixReturnCheck.cpp SizeofContainerCheck.cpp SizeofExpressionCheck.cpp + SpuriouslyWakeUpFunctionsCheck.cpp StringConstructorCheck.cpp StringIntegerAssignmentCheck.cpp StringLiteralWithEmbeddedNulCheck.cpp Index: clang-tools-extra/clang-tidy/bugprone/SpuriouslyWakeUpFunctionsCheck.h =================================================================== --- /dev/null +++ clang-tools-extra/clang-tidy/bugprone/SpuriouslyWakeUpFunctionsCheck.h @@ -0,0 +1,36 @@ +//===--- 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_BUGPRONE_SPURIOUSLYWAKEUPFUNCTIONSCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SPURIOUSLYWAKEUPFUNCTIONSCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang { +namespace tidy { +namespace bugprone { + +/// Finds ``cnd_wait``, ``wait``, ``wait_for``, or ``wait_until`` 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. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-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 bugprone +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SPURIOUSLYWAKEUPFUNCTIONSCHECK_H Index: clang-tools-extra/clang-tidy/bugprone/SpuriouslyWakeUpFunctionsCheck.cpp =================================================================== --- /dev/null +++ clang-tools-extra/clang-tidy/bugprone/SpuriouslyWakeUpFunctionsCheck.cpp @@ -0,0 +1,84 @@ +//===--- 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 bugprone { + +void SpuriouslyWakeUpFunctionsCheck::registerMatchers(MatchFinder *Finder) { + + 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")); + + auto hasWaitDescendantC = hasDescendant( + callExpr(callee(functionDecl( + anyOf(hasName("cnd_wait"), hasName("cnd_timedwait"))))) + .bind("wait")); + Finder->addMatcher( + ifStmt(anyOf( + // Check for `CON54-CPP` + allOf(hasWaitDescendantCPP, + unless(anyOf(hasDescendant(ifStmt(hasWaitDescendantCPP)), + hasDescendant(whileStmt(hasWaitDescendantCPP)), + hasDescendant(forStmt(hasWaitDescendantCPP)), + hasDescendant(doStmt(hasWaitDescendantCPP))))), + // Check for `CON36-C` + hasDescendant(compoundStmt(allOf( + hasWaitDescendantC, + unless(anyOf(hasDescendant(ifStmt(hasDescendant( + compoundStmt(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"); + SmallString<128> Buffer; + llvm::raw_svector_ostream Str(Buffer); + Str << "'%0' should be placed inside a while statement"; + auto waitName = MatchedWait->getDirectCallee()->getName(); + if (waitName != "cnd_wait" && waitName != "cnd_timedwait") + Str << " or used with a condition parameter"; + diag(MatchedWait->getExprLoc(), Str.str().str()) << waitName; +} +} // namespace bugprone +} // namespace tidy +} // namespace clang 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 @@ -11,6 +11,7 @@ #include "../ClangTidyModuleRegistry.h" #include "../bugprone/UnhandledSelfAssignmentCheck.h" #include "../bugprone/BadSignalToKillThreadCheck.h" +#include "../bugprone/SpuriouslyWakeUpFunctionsCheck.h" #include "../google/UnnamedNamespaceInHeaderCheck.h" #include "../misc/NewDeleteOverloadsCheck.h" #include "../misc/NonCopyableObjects.h" @@ -40,6 +41,9 @@ public: void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { // C++ checkers + // CON + CheckFactories.registerCheck( + "cert-con54-cpp"); // DCL CheckFactories.registerCheck( "cert-dcl21-cpp"); @@ -74,6 +78,9 @@ "cert-oop58-cpp"); // C checkers + // CON + CheckFactories.registerCheck( + "cert-con36-c"); // DCL CheckFactories.registerCheck("cert-dcl03-c"); CheckFactories.registerCheck( Index: clang-tools-extra/docs/ReleaseNotes.rst =================================================================== --- clang-tools-extra/docs/ReleaseNotes.rst +++ clang-tools-extra/docs/ReleaseNotes.rst @@ -94,6 +94,13 @@ Without the null terminator it can result in undefined behaviour when the string is read. +- New :doc:`bugprone-spuriously-wake-up-functions + ` check. + + Finds ``cnd_wait``, ``wait``, ``wait_for``, or ``wait_until`` 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. + - New :doc:`cert-mem57-cpp ` check. @@ -216,6 +223,16 @@ added to address situations where the existing fix-it logic would sometimes generate code that no longer compiles. +- New alias :doc:`cert-con36-c + ` to + :doc:`bugprone-spuriously-wake-up-functions + ` was added. + +- New alias :doc:`cert-con54-cpp + ` to + :doc:`bugprone-spuriously-wake-up-functions + ` was added. + Improvements to include-fixer ----------------------------- Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-spuriously-wake-up-functions.rst =================================================================== --- /dev/null +++ clang-tools-extra/docs/clang-tidy/checks/bugprone-spuriously-wake-up-functions.rst @@ -0,0 +1,28 @@ +.. title:: clang-tidy - bugprone-spuriously-wake-up-functions + +bugprone-spuriously-wake-up-functions +===================================== + +Finds ``cnd_wait``, ``wait``, ``wait_for``, or ``wait_until`` 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. + +.. 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/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=bugprone-spuriously-wake-up-functions.html + +cert-con36-c +============ + +The cert-con36-c check is an alias, please see +`bugprone-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=bugprone-spuriously-wake-up-functions.html + +cert-con54-cpp +============== + +The cert-con54-cpp check is an alias, please see +`bugprone-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 @@ -72,6 +72,7 @@ `bugprone-posix-return `_, "Yes", "" `bugprone-sizeof-container `_, , "high" `bugprone-sizeof-expression `_, , "high" + `bugprone-spuriously-wake-up-functions `_, , "" `bugprone-string-constructor `_, "Yes", "high" `bugprone-string-integer-assignment `_, "Yes", "low" `bugprone-string-literal-with-embedded-nul `_, , "medium" @@ -292,6 +293,8 @@ :header: "Name", "Redirect", "Offers fixes", "Severity" :widths: 50, 50, 10, 10 + `cert-con36-c `_, `bugprone-spuriously-wake-up-functions `_, , "" + `cert-con54-cpp `_, `bugprone-spuriously-wake-up-functions `_, , "" `cert-dcl03-c `_, `misc-static-assert `_, "Yes", "medium" `cert-dcl16-c `_, `readability-uppercase-literal-suffix `_, "Yes", "style" `cert-dcl54-cpp `_, `misc-new-delete-overloads `_, , "medium" Index: clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/chrono.h =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/chrono.h @@ -0,0 +1,48 @@ +#include "ratio.h" + +namespace std { +namespace chrono { + +template > +class duration { +public: + using rep = Rep; + using period = Period; + +public: + constexpr duration() = default; + template + constexpr explicit duration(const Rep2 &r); + template + constexpr duration(const duration &d); + ~duration() = default; + duration(const duration &) = default; +}; + +template +class time_point { +public: + using clock = Clock; + using duration = Duration; + +public: + constexpr time_point(); + constexpr explicit time_point(const duration &d); + template + constexpr time_point(const time_point &t); +}; + +using milliseconds = duration; + +class system_clock { +public: + typedef milliseconds duration; + typedef duration::rep rep; + typedef duration::period period; + typedef chrono::time_point time_point; + + static time_point now() noexcept; +}; +} // namespace chrono + +} // namespace std \ No newline at end of file Index: clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/condition_variable.h =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/condition_variable.h @@ -0,0 +1,40 @@ +#include "chrono.h" +#include "mutex.h" + +namespace std { + +enum class cv_status { + no_timeout, + timeout +}; + +class condition_variable { +public: + condition_variable(); + ~condition_variable(); + condition_variable(const condition_variable &) = delete; + condition_variable &operator=(const condition_variable &) = delete; + + void notify_one() noexcept; + void notify_all() noexcept; + void wait(unique_lock &lock); + template + void wait(unique_lock &lock, Predicate pred); + template + cv_status wait_until(unique_lock &lock, + const chrono::time_point &abs_time){}; + template + bool wait_until(unique_lock &lock, + const chrono::time_point &abs_time, + Predicate pred){}; + template + cv_status wait_for(unique_lock &lock, + const chrono::duration &rel_time){}; + template + bool wait_for(unique_lock &lock, + const chrono::duration &rel_time, + Predicate pred){}; +}; +class condition_variable_any; + +} // namespace std \ No newline at end of file Index: clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/cstdint.h =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/cstdint.h @@ -0,0 +1,3 @@ +namespace std { +using intmax_t = int; +} \ No newline at end of file Index: clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/mutex.h =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/mutex.h @@ -0,0 +1,19 @@ +namespace std { +class mutex; +template +class unique_lock { +public: + typedef Mutex mutex_type; + + unique_lock() noexcept; + explicit unique_lock(mutex_type &m); +}; + +class mutex { +public: + constexpr mutex() noexcept; + ~mutex(); + mutex(const mutex &) = delete; + mutex &operator=(const mutex &) = delete; +}; +} // namespace std \ No newline at end of file Index: clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/ratio.h =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/ratio.h @@ -0,0 +1,28 @@ +#include "cstdint.h" + +namespace std { + +template +class ratio { +public: + static constexpr intmax_t num = 0; + static constexpr intmax_t den = 0; + typedef ratio type; +}; + +template +struct ratio_equal; +template +struct ratio_not_equal; +template +struct ratio_less; +template +struct ratio_less_equal; +template +struct ratio_greater; +template +struct ratio_greater_equal; + +typedef ratio<1, 1000> milli; + +} // namespace std \ No newline at end of file Index: clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp =================================================================== --- /dev/null +++ clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp @@ -0,0 +1,99 @@ +// RUN: %check_clang_tidy %s bugprone-spuriously-wake-up-functions %t -- -- -I %S/Inputs/bugprone-spuriously-wake-up-functions/ +#include "condition_variable.h" +#define NULL 0 + +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 [bugprone-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 [bugprone-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); + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'wait_until' should be placed inside a while statement or used with a condition parameter [bugprone-spuriously-wake-up-functions] + } + if (list.next == nullptr) { + condition.wait_until(lk, now, [] { return 1; }); + } +} + +typedef struct mtx_t { +} mtx_t; +typedef struct cnd_t { +} cnd_t; +struct timespec {}; + +int cnd_wait(cnd_t *cond, mtx_t *mutex){}; +int cnd_timedwait(cnd_t *cond, mtx_t *mutex, + const struct timespec *time_point){}; + +struct Node1 list_c; +static mtx_t lock; +static cnd_t condition_c; +struct timespec ts; + +void consume_list_element(void) { + + if (list_c.next == NULL) { + if (0 != cnd_wait(&condition_c, &lock)) { + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 'cnd_wait' should be placed inside a while statement [bugprone-spuriously-wake-up-functions] + } + } + while (list_c.next == NULL) { + if (0 != cnd_wait(&condition_c, &lock)) { + } + } + if (list_c.next == NULL) { + cnd_wait(&condition_c, &lock); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'cnd_wait' should be placed inside a while statement [bugprone-spuriously-wake-up-functions] + } + while (list.next == NULL) { + cnd_wait(&condition_c, &lock); + } + if (list_c.next == NULL) { + if (0 != cnd_timedwait(&condition_c, &lock, &ts)) { + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 'cnd_timedwait' should be placed inside a while statement [bugprone-spuriously-wake-up-functions] + } + } + while (list_c.next == NULL) { + if (0 != cnd_timedwait(&condition_c, &lock, &ts)) { + } + } + if (list_c.next == NULL) { + cnd_timedwait(&condition_c, &lock, &ts); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'cnd_timedwait' should be placed inside a while statement [bugprone-spuriously-wake-up-functions] + } + while (list.next == NULL) { + cnd_timedwait(&condition_c, &lock, &ts); + } +}