diff --git a/clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.h b/clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.h --- a/clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.h @@ -1,4 +1,4 @@ -//===--- PosixReturnCheck.h - clang-tidy-------------------------*- C++ -*-===// +//===--- PosixReturnCheck.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. diff --git a/clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.cpp --- a/clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.cpp @@ -28,26 +28,31 @@ } void PosixReturnCheck::registerMatchers(MatchFinder *Finder) { - Finder->addMatcher(binaryOperator(hasOperatorName("<"), - hasLHS(callExpr(callee(functionDecl( - matchesName("^::posix_"), - unless(hasName("::posix_openpt")))))), - hasRHS(integerLiteral(equals(0)))) - .bind("ltzop"), - this); - Finder->addMatcher(binaryOperator(hasOperatorName(">="), - hasLHS(callExpr(callee(functionDecl( - matchesName("^::posix_"), - unless(hasName("::posix_openpt")))))), - hasRHS(integerLiteral(equals(0)))) - .bind("atop"), - this); + Finder->addMatcher( + binaryOperator( + hasOperatorName("<"), + hasLHS(callExpr(callee(functionDecl( + anyOf(matchesName("^::posix_"), matchesName("^::pthread_")), + unless(hasName("::posix_openpt")))))), + hasRHS(integerLiteral(equals(0)))) + .bind("ltzop"), + this); + Finder->addMatcher( + binaryOperator( + hasOperatorName(">="), + hasLHS(callExpr(callee(functionDecl( + anyOf(matchesName("^::posix_"), matchesName("^::pthread_")), + unless(hasName("::posix_openpt")))))), + hasRHS(integerLiteral(equals(0)))) + .bind("atop"), + this); Finder->addMatcher( binaryOperator( anyOf(hasOperatorName("=="), hasOperatorName("!="), hasOperatorName("<="), hasOperatorName("<")), hasLHS(callExpr(callee(functionDecl( - matchesName("^::posix_"), unless(hasName("::posix_openpt")))))), + anyOf(matchesName("^::posix_"), matchesName("^::pthread_")), + unless(hasName("::posix_openpt")))))), hasRHS(unaryOperator(hasOperatorName("-"), hasUnaryOperand(integerLiteral())))) .bind("binop"), 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 @@ -91,6 +91,11 @@ Finds historical use of ``unsigned`` to hold vregs and physregs and rewrites them to use ``Register`` +- Improved :doc:`bugprone-posix-return + ` check. + + Now also checks if any calls to ``pthread_*`` functions expect negative return + values. Improvements to include-fixer ----------------------------- diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone-posix-return.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone-posix-return.rst --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone-posix-return.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone-posix-return.rst @@ -3,9 +3,9 @@ bugprone-posix-return ===================== -Checks if any calls to POSIX functions (except ``posix_openpt``) expect negative -return values. These functions return either ``0`` on success or an ``errno`` on failure, -which is positive only. +Checks if any calls to ``pthread_*`` or ``posix_*`` functions +(except ``posix_openpt``) 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: diff --git a/clang-tools-extra/test/clang-tidy/bugprone-posix-return.cpp b/clang-tools-extra/test/clang-tidy/bugprone-posix-return.cpp --- a/clang-tools-extra/test/clang-tidy/bugprone-posix-return.cpp +++ b/clang-tools-extra/test/clang-tidy/bugprone-posix-return.cpp @@ -9,6 +9,16 @@ typedef decltype(sizeof(int)) size_t; typedef struct __posix_spawn_file_actions* posix_spawn_file_actions_t; typedef struct __posix_spawnattr* posix_spawnattr_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 struct _opaque_pthread_t *__darwin_pthread_t; +typedef __darwin_pthread_t pthread_t; +typedef struct pthread_attr_t_ *pthread_attr_t; extern "C" int posix_fadvise(int fd, off_t offset, off_t len, int advice); extern "C" int posix_fallocate(int fd, off_t offset, off_t len); @@ -23,6 +33,12 @@ const posix_spawn_file_actions_t *file_actions, const posix_spawnattr_t *attrp, char *const argv[], char *const envp[]); +extern "C" int pthread_create(pthread_t *thread, const pthread_attr_t *attr, void *(*start_routine)(void *), void *arg); +extern "C" int pthread_attr_setaffinity_np(pthread_attr_t *attr, size_t cpusetsize, const cpu_set_t *cpuset); +extern "C" int pthread_attr_setschedpolicy(pthread_attr_t *attr, int policy); +extern "C" int pthread_attr_init(pthread_attr_t *attr); +extern "C" int pthread_yield(void); + void warningLessThanZero() { if (posix_fadvise(0, 0, 0, 0) < 0) {} @@ -43,11 +59,38 @@ if (posix_spawnp(NULL, NULL, NULL, NULL, {NULL}, {NULL}) < 0) {} // CHECK-MESSAGES: :[[@LINE-1]]:60: warning: // CHECK-FIXES: posix_spawnp(NULL, NULL, NULL, NULL, {NULL}, {NULL}) > 0 + 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 (posix_fadvise(0, 0, 0, 0) >= 0) {} // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: the comparison always evaluates to true because posix_fadvise always returns non-negative values + 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() { @@ -69,6 +112,15 @@ // CHECK-MESSAGES: :[[@LINE-1]]:59: warning: if (posix_spawnp(NULL, NULL, NULL, NULL, {NULL}, {NULL}) == -1) {} // CHECK-MESSAGES: :[[@LINE-1]]:60: warning: + 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() { @@ -85,6 +137,20 @@ // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: if (posix_fadvise(0, 0, 0, 0) < NEGATIVE_ONE) {} // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: + 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: + } void noWarning() { @@ -100,6 +166,7 @@ namespace i { int posix_fadvise(int fd, off_t offset, off_t len, int advice); +int pthread_yield(void); void noWarning() { if (posix_fadvise(0, 0, 0, 0) < 0) {} @@ -108,6 +175,12 @@ if (posix_fadvise(0, 0, 0, 0) != -1) {} if (posix_fadvise(0, 0, 0, 0) <= -1) {} if (posix_fadvise(0, 0, 0, 0) < -1) {} + if (pthread_yield() < 0) {} + if (pthread_yield() >= 0) {} + if (pthread_yield() == -1) {} + if (pthread_yield() != -1) {} + if (pthread_yield() <= -1) {} + if (pthread_yield() < -1) {} } } // namespace i @@ -115,6 +188,7 @@ class G { public: int posix_fadvise(int fd, off_t offset, off_t len, int advice); + int pthread_yield(void); void noWarning() { if (posix_fadvise(0, 0, 0, 0) < 0) {} @@ -123,5 +197,11 @@ if (posix_fadvise(0, 0, 0, 0) != -1) {} if (posix_fadvise(0, 0, 0, 0) <= -1) {} if (posix_fadvise(0, 0, 0, 0) < -1) {} + if (pthread_yield() < 0) {} + if (pthread_yield() >= 0) {} + if (pthread_yield() == -1) {} + if (pthread_yield() != -1) {} + if (pthread_yield() <= -1) {} + if (pthread_yield() < -1) {} } };