[clang-tidy] add checks to bugprone-posix-return

[jcai19]

Summary

This check now also checks if any calls to pthread_* functions expect negative return values. These functions return either 0 on success or an errno on failure, which is positive only.

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/bugprone/PthreadReturnCheck.cpp

1 ↗

(On Diff #216748)

Please make it single line, 80 characters long.

23 ↗

(On Diff #216748)

You could use const auto * here, because type is spelled in same statement.

clang-tools-extra/clang-tidy/bugprone/PthreadReturnCheck.h

1 ↗

(On Diff #216748)

Please make line 80 characters long.

clang-tools-extra/docs/ReleaseNotes.rst

88

Please keep new checks list sorted alphabetically.

[jcai19]

Fix format.

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/bugprone/PthreadReturnCheck.cpp

1 ↗

(On Diff #216765)

Please put space after clang-tidy. Same in header.

2 ↗

(On Diff #216765)

Please remove this line.

[jcai19]

Fix typos.

[jcai19]

clang-tools-extra/clang-tidy/bugprone/PthreadReturnCheck.cpp

2 ↗

(On Diff #216765)

Good catch! Thanks.

[gribozavr]

Thanks for the contribution! In abstract, I think it is a good checker, however, the implementation largely duplicates clang-tidy/bugprone/PosixReturnCheck.cpp -- do you think you could factor out the common parts? I see at least two ways:

• Factor out a library, use it in both checkers.

• Put everything into one checker, if necessary, add configuration options to turn on/off POSIX and PThread parts. However, I don't even think configuration options would be necessary -- it is unlikely that someone would want one but not the other -- or is there a use case?

clang-tools-extra/test/clang-tidy/bugprone-pthread-return.cpp

101 ↗

(On Diff #216768)

It does not seem like most of these functions are used in the test below. Could you leave only representative functions for each case in the code?

[jcai19]

In D66627#1644775, @gribozavr wrote:

Thanks for the contribution! In abstract, I think it is a good checker, however, the implementation largely duplicates clang-tidy/bugprone/PosixReturnCheck.cpp -- do you think you could factor out the common parts? I see at least two ways:

• Factor out a library, use it in both checkers.

• Put everything into one checker, if necessary, add configuration options to turn on/off POSIX and PThread parts. However, I don't even think configuration options would be necessary -- it is unlikely that someone would want one but not the other -- or is there a use case?

That's a good suggestion. I will try to combine it with POSIX check. Thanks!

[jcai19]

Add pthread function calls return values check to bugprone-posix-return.

[Eugene.Zelenko]

It'll be reasonable to mention changes in Release Notes and check documentation.

[jcai19]

Update Release Notes and check documentation.

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

70

Check is not new, just modified. Such check should be after new checks.

71

Please insert empty line below.

72

Please omit This check. Please enclose pthread_* in double back-ticks.

clang-tools-extra/docs/clang-tidy/checks/bugprone-posix-return.rst

6–8

Please enclose pthread_* and posix_* in double back-ticks.

[gribozavr]

LGTM with all comments addressed.

clang-tools-extra/test/clang-tidy/bugprone-posix-return.cpp

169

Maybe pick a simpler function for this test, like pthread_yield?

191

Ditto, pthread_yield would be simpler to declare.

[jcai19]

Fix format and test cases.

[jcai19]

clang-tools-extra/docs/ReleaseNotes.rst

70

Is Modified the right keyword for modification? Thanks

[gribozavr]

clang-tools-extra/docs/ReleaseNotes.rst

70

I'd say "improved" instead of modified.

[jcai19]

Thanks for the confirmation!

clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.h

-//===--- 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.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_POSIX_RETURN_CHECK_H

clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.cpp

const CallExpr *MatchedCall = cast<CallExpr>(
       (Result.Nodes.getNodeAs<BinaryOperator>(BindingStr))->getLHS());
   const SourceManager &SM = *Result.SourceManager;
   return Lexer::getSourceText(CharSourceRange::getTokenRange(
                                   MatchedCall->getCallee()->getSourceRange()),
                               SM, Result.Context->getLangOpts());
 }
 
 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("<"),
+              anyOf(matchesName("^::posix_"), matchesName("^::pthread_")),
+  Finder->addMatcher(
+      binaryOperator(
+          hasOperatorName(">="),
+              anyOf(matchesName("^::posix_"), matchesName("^::pthread_")),
   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"),
       this);
 }
 
 void PosixReturnCheck::check(const MatchFinder::MatchResult &Result) {
   if (const auto *LessThanZeroOp =

clang-tools-extra/docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
 - New :doc:`bugprone-dynamic-static-initializers
   <clang-tidy/checks/bugprone-dynamic-static-initializers>` check.
 
   Finds instances where variables with static storage are initialized
   dynamically in header files.
 
 - New :doc:`linuxkernel-must-use-errs
   <clang-tidy/checks/linuxkernel-must-use-errs>` check.
 
   Checks Linux kernel code to see if it uses the results from the functions in
   ``linux/err.h``.
 
 - New :doc:`google-upgrade-googletest-case
   <clang-tidy/checks/google-upgrade-googletest-case>` check.
 
   Finds uses of deprecated Googletest APIs with names containing ``case`` and
   replaces them with equivalent APIs with ``suite``.
 
 - New :doc:`llvm-prefer-register-over-unsigned
   <clang-tidy/checks/llvm-prefer-register-over-unsigned>` check.
 
   Finds historical use of ``unsigned`` to hold vregs and physregs and rewrites
   them to use ``Register``
 
+- Improved :doc:`bugprone-posix-return
+  <clang-tidy/checks/bugprone-posix-return>` check.
+
+  Now also checks if any calls to ``pthread_*`` functions expect negative return
+  values.
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to clang-include-fixer
 -----------------------------------

clang-tools-extra/docs/clang-tidy/checks/bugprone-posix-return.rst

 .. title:: clang-tidy - bugprone-posix-return
 
 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:
 
 .. code-block:: c
 
   if (posix_fadvise(...) < 0) {
 
 This will never happen as the return value is always non-negative. A simple fix could be:
 
 .. code-block:: c
 
   if (posix_fadvise(...) > 0) {

clang-tools-extra/test/clang-tidy/bugprone-posix-return.cpp

 // RUN: %check_clang_tidy %s bugprone-posix-return %t
 
 #define NULL nullptr
 #define ZERO 0
 #define NEGATIVE_ONE -1
 
 typedef int pid_t;
 typedef long off_t;
 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);
 extern "C" int posix_madvise(void *addr, size_t len, int advice);
 extern "C" int posix_memalign(void **memptr, size_t alignment, size_t size);
 extern "C" int posix_openpt(int flags);
 extern "C" int posix_spawn(pid_t *pid, const char *path,
                 const posix_spawn_file_actions_t *file_actions,
                 const posix_spawnattr_t *attrp,
                 char *const argv[], char *const envp[]);
 extern "C" int posix_spawnp(pid_t *pid, const char *file,
                  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) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: the comparison always evaluates to false because posix_fadvise always returns non-negative values
   // CHECK-FIXES: posix_fadvise(0, 0, 0, 0) > 0
   if (posix_fallocate(0, 0, 0) < 0) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:32: warning:
   // CHECK-FIXES: posix_fallocate(0, 0, 0) > 0
   if (posix_madvise(NULL, 0, 0) < 0) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:33: warning:
   // CHECK-FIXES: posix_madvise(NULL, 0, 0) > 0
   if (posix_memalign(NULL, 0, 0) < 0) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:34: warning:
   // CHECK-FIXES: posix_memalign(NULL, 0, 0) > 0
   if (posix_spawn(NULL, NULL, NULL, NULL, {NULL}, {NULL}) < 0) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:59: warning:
   // CHECK-FIXES: posix_spawn(NULL, NULL, NULL, NULL, {NULL}, {NULL}) > 0
   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() {
   if (posix_fadvise(0, 0, 0, 0) == -1) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: posix_fadvise
   if (posix_fadvise(0, 0, 0, 0) != -1) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:33: warning:
   if (posix_fadvise(0, 0, 0, 0) <= -1) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:33: warning:
   if (posix_fadvise(0, 0, 0, 0) < -1) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:33: warning:
   if (posix_fallocate(0, 0, 0) == -1) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:32: warning:
   if (posix_madvise(NULL, 0, 0) == -1) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:33: warning:
   if (posix_memalign(NULL, 0, 0) == -1) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:34: warning:
   if (posix_spawn(NULL, NULL, NULL, NULL, {NULL}, {NULL}) == -1) {}
   // 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() {
   if (posix_fadvise(0, 0, 0, 0) < ZERO) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:33: warning:
   // CHECK-FIXES: posix_fadvise(0, 0, 0, 0) > ZERO
   if (posix_fadvise(0, 0, 0, 0) >= ZERO) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:33: warning:
   if (posix_fadvise(0, 0, 0, 0) == NEGATIVE_ONE) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:33: warning:
   if (posix_fadvise(0, 0, 0, 0) != NEGATIVE_ONE) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:33: warning:
   if (posix_fadvise(0, 0, 0, 0) <= NEGATIVE_ONE) {}
   // 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() {
   if (posix_openpt(0) < 0) {}
   if (posix_openpt(0) <= 0) {}
   if (posix_openpt(0) == -1) {}
   if (posix_openpt(0) != -1) {}
   if (posix_openpt(0) <= -1) {}
   if (posix_openpt(0) < -1) {}
   if (posix_fadvise(0, 0, 0, 0) <= 0) {}
   if (posix_fadvise(0, 0, 0, 0) == 1) {}
 }
 
 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) {}
   if (posix_fadvise(0, 0, 0, 0) >= 0) {}
   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 (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
 
 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) {}
     if (posix_fadvise(0, 0, 0, 0) >= 0) {}
     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 (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) {}
   }
 };