diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -32,6 +32,7 @@ #include "MultipleStatementMacroCheck.h" #include "ParentVirtualCallCheck.h" #include "PosixReturnCheck.h" +#include "PthreadReturnCheck.h" #include "SizeofContainerCheck.h" #include "SizeofExpressionCheck.h" #include "StringConstructorCheck.h" @@ -107,6 +108,8 @@ "bugprone-parent-virtual-call"); CheckFactories.registerCheck( "bugprone-posix-return"); + CheckFactories.registerCheck( + "bugprone-pthread-return"); CheckFactories.registerCheck( "bugprone-sizeof-container"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -24,6 +24,7 @@ MultipleStatementMacroCheck.cpp ParentVirtualCallCheck.cpp PosixReturnCheck.cpp + PthreadReturnCheck.cpp SizeofContainerCheck.cpp SizeofExpressionCheck.cpp StringConstructorCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/PthreadReturnCheck.h b/clang-tools-extra/clang-tidy/bugprone/PthreadReturnCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/PthreadReturnCheck.h @@ -0,0 +1,30 @@ +//===--- PthreadReturnCheck.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_PTHREAD_RETURN_CHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_PTHREAD_RETURN_CHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang { +namespace tidy { +namespace bugprone { + +class PthreadReturnCheck: public ClangTidyCheck { +public: + PthreadReturnCheck(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_PTHREAD_RETURN_CHECK_H diff --git a/clang-tools-extra/clang-tidy/bugprone/PthreadReturnCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/PthreadReturnCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/PthreadReturnCheck.cpp @@ -0,0 +1,82 @@ +//===--- PthreadReturnCheck.cpp - clang-tidy-------------------------------===// +//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 "PthreadReturnCheck.h" +#include "../utils/Matchers.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace bugprone { + +static StringRef getFunctionSpelling(const MatchFinder::MatchResult &Result, + const char *BindingStr) { + const auto *MatchedCall = cast( + (Result.Nodes.getNodeAs(BindingStr))->getLHS()); + const SourceManager &SM = *Result.SourceManager; + return Lexer::getSourceText(CharSourceRange::getTokenRange( + MatchedCall->getCallee()->getSourceRange()), + SM, Result.Context->getLangOpts()); +} + +void PthreadReturnCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + binaryOperator( + hasOperatorName("<"), + hasLHS(callExpr(callee(functionDecl(matchesName("^::pthread_"))))), + hasRHS(integerLiteral(equals(0)))) + .bind("ltzop"), + this); + Finder->addMatcher( + binaryOperator( + hasOperatorName(">="), + hasLHS(callExpr(callee(functionDecl(matchesName("^::pthread_"))))), + hasRHS(integerLiteral(equals(0)))) + .bind("atop"), + this); + Finder->addMatcher( + binaryOperator( + anyOf(hasOperatorName("=="), hasOperatorName("!="), + hasOperatorName("<="), hasOperatorName("<")), + hasLHS(callExpr(callee(functionDecl(matchesName("^::pthread_"))))), + hasRHS(unaryOperator(hasOperatorName("-"), + hasUnaryOperand(integerLiteral())))) + .bind("binop"), + this); +} + +void PthreadReturnCheck::check(const MatchFinder::MatchResult &Result) { + if (const auto *LessThanZeroOp = + Result.Nodes.getNodeAs("ltzop")) { + SourceLocation OperatorLoc = LessThanZeroOp->getOperatorLoc(); + diag(OperatorLoc, "the comparison always evaluates to false because %0 " + "always returns non-negative values") + << getFunctionSpelling(Result, "ltzop") + << FixItHint::CreateReplacement(OperatorLoc, Twine(">").str()); + return; + } + if (const auto *AlwaysTrueOp = + Result.Nodes.getNodeAs("atop")) { + diag(AlwaysTrueOp->getOperatorLoc(), + "the comparison always evaluates to true because %0 always returns " + "non-negative values") + << getFunctionSpelling(Result, "atop"); + return; + } + const auto *BinOp = Result.Nodes.getNodeAs("binop"); + diag(BinOp->getOperatorLoc(), "%0 only returns non-negative values") + << getFunctionSpelling(Result, "binop"); +} + +} // namespace bugprone +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -67,6 +67,11 @@ Improvements to clang-tidy -------------------------- +- New :doc:`bugprone-pthread-return + ` check. + + Checks if any calls to POSIX thread functions expect negative return values. + - New :doc:`linuxkernel-must-use-errs ` check. @@ -79,7 +84,6 @@ Finds uses of deprecated Googletest APIs with names containing ``case`` and replaces them with equivalent APIs with ``suite``. - Improvements to include-fixer ----------------------------- diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone-pthread-return.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone-pthread-return.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone-pthread-return.rst @@ -0,0 +1,20 @@ +.. title:: clang-tidy - bugprone-pthread-return + +bugprone-posix-return +===================== + +Checks if any calls to POSIX thread functions expect negative return values. +These functions return either ``0`` on success or an ``errno`` on failure, +which is positive only. + +Example buggy usage looks like: + +.. code-block:: c + + if (posix_create(...) < 0) { + +This will never happen as the return value is always non-negative. A simple fix could be: + +.. code-block:: c + + if (posix_create(...) > 0) { diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -60,6 +60,7 @@ bugprone-multiple-statement-macro bugprone-parent-virtual-call bugprone-posix-return + bugprone-pthread-return bugprone-sizeof-container bugprone-sizeof-expression bugprone-string-constructor diff --git a/clang-tools-extra/test/clang-tidy/bugprone-pthread-return.cpp b/clang-tools-extra/test/clang-tidy/bugprone-pthread-return.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/bugprone-pthread-return.cpp @@ -0,0 +1,187 @@ +// RUN: %check_clang_tidy %s bugprone-pthread-return %t + +#define NULL nullptr +#define ZERO 0 +#define NEGATIVE_ONE -1 + +typedef decltype(sizeof(int)) size_t; +# define __CPU_SETSIZE 1024 +# define __NCPUBITS (8 * sizeof (__cpu_mask)) +typedef unsigned long int __cpu_mask; +typedef struct +{ + __cpu_mask __bits[__CPU_SETSIZE / __NCPUBITS]; +} cpu_set_t; +typedef int clockid_t; +# define _SIGSET_NWORDS (1024 / (8 * sizeof (unsigned long int))) +typedef struct +{ + unsigned long int __val[_SIGSET_NWORDS]; +} sigset_t; +typedef struct _opaque_pthread_t *__darwin_pthread_t; +typedef __darwin_pthread_t pthread_t; +typedef struct pthread_attr_t_ *pthread_attr_t; +typedef struct pthread_mutex_t_ * pthread_mutex_t; +typedef struct pthread_mutexattr_t_ * pthread_mutexattr_t; +typedef struct pthread_rwlockattr_t_ * pthread_rwlockattr_t; +typedef struct pthread_spinlock_t_ * pthread_spinlock_t; + + +extern "C" void pthread_cleanup_push(void (*routine)(void *), void *arg); +extern "C" void pthread_cleanup_pop(int execute); +extern "C" void pthread_cleanup_push_defer_np(void (*routine)(void *), void *arg); +extern "C" void pthread_cleanup_pop_restore_np(int execute); +extern "C" void pthread_exit(void *retval); +extern "C" void pthread_kill_other_threads_np(void); +extern "C" void pthread_testcancel(void); + +extern "C" pthread_t pthread_self(void); + +extern "C" int pthread_attr_init(pthread_attr_t *attr); +extern "C" int pthread_attr_destroy(pthread_attr_t *attr); +extern "C" int pthread_attr_setaffinity_np(pthread_attr_t *attr, size_t cpusetsize, const cpu_set_t *cpuset); +extern "C" int pthread_attr_getaffinity_np(const pthread_attr_t *attr, size_t cpusetsize, cpu_set_t *cpuset); +extern "C" int pthread_attr_setdetachstate(pthread_attr_t *attr, int detachstate); +extern "C" int pthread_attr_getdetachstate(const pthread_attr_t *attr, int *detachstate); +extern "C" int pthread_attr_setguardsize(pthread_attr_t *attr, size_t guardsize); +extern "C" int pthread_attr_getguardsize(const pthread_attr_t *attr, size_t *guardsize); +extern "C" int pthread_attr_setinheritsched(pthread_attr_t *attr, int inheritsched); +extern "C" int pthread_attr_getinheritsched(const pthread_attr_t *attr, int *inheritsched); +extern "C" int pthread_attr_setschedparam(pthread_attr_t *attr, const struct sched_param *param); +extern "C" int pthread_attr_getschedparam(const pthread_attr_t *attr, struct sched_param *param); +extern "C" int pthread_attr_setschedpolicy(pthread_attr_t *attr, int policy); +extern "C" int pthread_attr_getschedpolicy(const pthread_attr_t *attr, int *policy); +extern "C" int pthread_attr_setscope(pthread_attr_t *attr, int scope); +extern "C" int pthread_attr_getscope(const pthread_attr_t *attr, int *scope); +extern "C" int pthread_attr_setstack(pthread_attr_t *attr, void *stackaddr, size_t stacksize); +extern "C" int pthread_attr_getstack(const pthread_attr_t *attr, void **stackaddr, size_t *stacksize); +extern "C" int pthread_attr_setstackaddr(pthread_attr_t *attr, void *stackaddr); +extern "C" int pthread_attr_getstackaddr(const pthread_attr_t *attr, void **stackaddr); +extern "C" int pthread_attr_setstacksize(pthread_attr_t *attr, size_t stacksize); +extern "C" int pthread_attr_getstacksize(const pthread_attr_t *attr, size_t *stacksize); +extern "C" int pthread_cancel(pthread_t thread); +extern "C" int pthread_create(pthread_t *thread, const pthread_attr_t *attr, void *(*start_routine)(void *), void *arg); +extern "C" int pthread_detach(pthread_t thread); +extern "C" int pthread_equal(pthread_t t1, pthread_t t2); +extern "C" int pthread_setaffinity_np(pthread_t thread, size_t cpusetsize, const cpu_set_t *cpuset); +extern "C" int pthread_getaffinity_np(pthread_t thread, size_t cpusetsize, cpu_set_t *cpuset); +extern "C" int pthread_getattr_default_np(pthread_attr_t *attr); +extern "C" int pthread_setattr_default_np(pthread_attr_t *attr); +extern "C" int pthread_getattr_np(pthread_t thread, pthread_attr_t *attr); +extern "C" int pthread_setconcurrency(int new_level); +extern "C" int pthread_getconcurrency(void); +extern "C" int pthread_getcpuclockid(pthread_t thread, clockid_t *clock_id); +extern "C" int pthread_setname_np(pthread_t thread, const char *name); +extern "C" int pthread_getname_np(pthread_t thread, char *name, size_t len); +extern "C" int pthread_setschedparam(pthread_t thread, int policy, const struct sched_param *param); +extern "C" int pthread_getschedparam(pthread_t thread, int *policy, struct sched_param *param); +extern "C" int pthread_join(pthread_t thread, void **retval); +extern "C" int pthread_kill(pthread_t thread, int sig); +extern "C" int pthread_mutexattr_getpshared(const pthread_mutexattr_t *attr, int *pshared); +extern "C" int pthread_mutexattr_setpshared(pthread_mutexattr_t *attr, int pshared); +extern "C" int pthread_mutexattr_getrobust(const pthread_mutexattr_t *attr, int *robustness); +extern "C" int pthread_mutexattr_setrobust(const pthread_mutexattr_t *attr, int robustness); +extern "C" int pthread_mutexattr_getrobust_np(const pthread_mutexattr_t *attr, int *robustness); +extern "C" int pthread_mutexattr_setrobust_np(const pthread_mutexattr_t *attr, int robustness); +extern "C" int pthread_mutex_consistent(pthread_mutex_t *mutex); +extern "C" int pthread_rwlockattr_setkind_np(pthread_rwlockattr_t *attr, int pref); +extern "C" int pthread_rwlockattr_getkind_np(const pthread_rwlockattr_t *attr, int *pref); +extern "C" int pthread_setcancelstate(int state, int *oldstate); +extern "C" int pthread_setcanceltype(int type, int *oldtype); +extern "C" int pthread_setschedprio(pthread_t thread, int prio); +extern "C" int pthread_sigmask(int how, const sigset_t *set, sigset_t *oldset); +extern "C" int pthread_sigqueue(pthread_t thread, int sig, const union sigval value); +extern "C" int pthread_spin_init(pthread_spinlock_t *lock, int pshared); +extern "C" int pthread_spin_destroy(pthread_spinlock_t *lock); +extern "C" int pthread_spin_lock(pthread_spinlock_t *lock); +extern "C" int pthread_spin_trylock(pthread_spinlock_t *lock); +extern "C" int pthread_spin_unlock(pthread_spinlock_t *lock); +extern "C" int pthread_tryjoin_np(pthread_t thread, void **retval); +extern "C" int pthread_timedjoin_np(pthread_t thread, void **retval, const struct timespec *abstime); +extern "C" int pthread_yield(void); + +void warningLessThanZero() { + if (pthread_create(NULL, NULL, NULL, NULL) < 0) {} + // CHECK-MESSAGES: :[[@LINE-1]]:46: warning: the comparison always evaluates to false because pthread_create always returns non-negative values + // CHECK-FIXES: pthread_create(NULL, NULL, NULL, NULL) > 0 + if (pthread_attr_setaffinity_np(NULL, 0, NULL) < 0) {} + // CHECK-MESSAGES: :[[@LINE-1]]:50: warning: + // CHECK-FIXES: pthread_attr_setaffinity_np(NULL, 0, NULL) > 0 + if (pthread_attr_setschedpolicy(NULL, 0) < 0) {} + // CHECK-MESSAGES: :[[@LINE-1]]:44: warning: + // CHECK-FIXES: pthread_attr_setschedpolicy(NULL, 0) > 0) + if (pthread_attr_init(NULL) < 0) {} + // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: + // CHECK-FIXES: pthread_attr_init(NULL) > 0 + if (pthread_yield() < 0) {} + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: + // CHECK-FIXES: pthread_yield() > 0 +} + +void warningAlwaysTrue() { + if (pthread_create(NULL, NULL, NULL, NULL) >= 0) {} + // CHECK-MESSAGES: :[[@LINE-1]]:46: warning: the comparison always evaluates to true because pthread_create always returns non-negative values + if (pthread_attr_setaffinity_np(NULL, 0, NULL) >= 0) {} + // CHECK-MESSAGES: :[[@LINE-1]]:50: warning: + if (pthread_attr_setschedpolicy(NULL, 0) >= 0) {} + // CHECK-MESSAGES: :[[@LINE-1]]:44: warning: + if (pthread_attr_init(NULL) >= 0) {} + // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: + if (pthread_yield() >= 0) {} + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: +} + +void warningEqualsNegative() { + if (pthread_create(NULL, NULL, NULL, NULL) == -1) {} + // CHECK-MESSAGES: :[[@LINE-1]]:46: warning: pthread_create + if (pthread_create(NULL, NULL, NULL, NULL) != -1) {} + // CHECK-MESSAGES: :[[@LINE-1]]:46: warning: + if (pthread_create(NULL, NULL, NULL, NULL) <= -1) {} + // CHECK-MESSAGES: :[[@LINE-1]]:46: warning: + if (pthread_create(NULL, NULL, NULL, NULL) < -1) {} + // CHECK-MESSAGES: :[[@LINE-1]]:46: warning: +} + +void WarningWithMacro() { + if (pthread_create(NULL, NULL, NULL, NULL) < ZERO) {} + // CHECK-MESSAGES: :[[@LINE-1]]:46: warning: + // CHECK-FIXES: pthread_create(NULL, NULL, NULL, NULL) > ZERO + if (pthread_create(NULL, NULL, NULL, NULL) >= ZERO) {} + // CHECK-MESSAGES: :[[@LINE-1]]:46: warning: + if (pthread_create(NULL, NULL, NULL, NULL) == NEGATIVE_ONE) {} + // CHECK-MESSAGES: :[[@LINE-1]]:46: warning: + if (pthread_create(NULL, NULL, NULL, NULL) != NEGATIVE_ONE) {} + // CHECK-MESSAGES: :[[@LINE-1]]:46: warning: + if (pthread_create(NULL, NULL, NULL, NULL) <= NEGATIVE_ONE) {} + // CHECK-MESSAGES: :[[@LINE-1]]:46: warning: + if (pthread_create(NULL, NULL, NULL, NULL) < NEGATIVE_ONE) {} + // CHECK-MESSAGES: :[[@LINE-1]]:46: warning: +} + +namespace i { +int pthread_create(pthread_t *thread, const pthread_attr_t *attr, void *(*start_routine)(void *), void *arg); + +void noWarning() { + if (pthread_create(NULL, NULL, NULL, NULL) < 0) {} + if (pthread_create(NULL, NULL, NULL, NULL) >= 0) {} + if (pthread_create(NULL, NULL, NULL, NULL) == -1) {} + if (pthread_create(NULL, NULL, NULL, NULL) != -1) {} + if (pthread_create(NULL, NULL, NULL, NULL) <= -1) {} + if (pthread_create(NULL, NULL, NULL, NULL) < -1) {} +} + +} // namespace i + +class G { +public: + int pthread_create(pthread_t *thread, const pthread_attr_t *attr, void *(*start_routine)(void *), void *arg); + + void noWarning() { + if (pthread_create(NULL, NULL, NULL, NULL) < 0) {} + if (pthread_create(NULL, NULL, NULL, NULL) >= 0) {} + if (pthread_create(NULL, NULL, NULL, NULL) == -1) {} + if (pthread_create(NULL, NULL, NULL, NULL) != -1) {} + if (pthread_create(NULL, NULL, NULL, NULL) <= -1) {} + if (pthread_create(NULL, NULL, NULL, NULL) < -1) {} + } +};