[clang-tidy] new check: bugprone-posix-return

[jcai19]

Summary

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.

[lebedev.ri]

Why is this in android module?
Is that android-specific behavior, or posix?

[george.burgess.iv]

just a few drive-by nits/comments from me. as usual, not super familiar with clang-tidy, so i won't be able to stamp this.

thanks!

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

23 ↗

(On Diff #205897)

should we also try to catch <= ${negative_value} (or < ${negative_value})?

29 ↗

(On Diff #205897)

similarly, should we complain about != ${negative_value}?

36 ↗

(On Diff #205897)

nit: please clang-format

39 ↗

(On Diff #205897)

would it be helpful to add fixits for simple cases? e.g. we can probably offer to replace posix_whatever() < 0 with posix_whatever() > 0 or != 0

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

57 ↗

(On Diff #205897)

nit: no space in Macro (), please

[jcai19]

In D63623#1552679, @lebedev.ri wrote:

Why is this in android module?
Is that android-specific behavior, or posix?

I implemented it for Andriod as requested, but it would be completely fine to move it if it is better to place the check at a different location. Where do you suggest we should move this check to? Thanks.

[Eugene.Zelenko]

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

1 ↗

(On Diff #205897)

Please use - before *- C++ -* to fill line. See other checks as example.

clang-tools-extra/docs/ReleaseNotes.rst

205

Please move into new checks list (in alphabetical order).

208

calls? posix -> POSIX. Please enclose posix_openpt in double back-ticks.

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

4 ↗

(On Diff #205897)

Please make same length as name.

6 ↗

(On Diff #205897)

Please make same changes as in Release Notes.

7 ↗

(On Diff #205897)

Please enclose 0 and errno into double back-ticks.

16 ↗

(On Diff #205897)

Please add colon at end.

[jcai19]

Fix typos and formatting issues.

[lebedev.ri]

In D63623#1552716, @jcai19 wrote:

In D63623#1552679, @lebedev.ri wrote:

Why is this in android module?
Is that android-specific behavior, or posix?

I implemented it for Andriod as requested, but it would be completely fine to move it if it is better to place the check at a different location. Where do you suggest we should move this check to? Thanks.

Either misc or bugprone or a new posix module.

[jcai19]

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

23 ↗

(On Diff #205897)

Sounds good. Will update the patch to catch these two cases as well.

29 ↗

(On Diff #205897)

Sounds good. Will update the patch to catch these additional cases as well.

39 ↗

(On Diff #205897)

While this fix is handy, I am not sure whether it will be safe enough under all circumstances. For example, is it possible in the code block following the check, the program calls another POSIX function and alter the errno before its value it checked? In that case, maybe the proper fix should be something as follows and fixing it by changing the binary operator may obscure it:

int ret = posix_whatever();
if (ret != 0)

[jcai19]

In D63623#1556161, @lebedev.ri wrote:

In D63623#1552716, @jcai19 wrote:

In D63623#1552679, @lebedev.ri wrote:

Why is this in android module?
Is that android-specific behavior, or posix?

I implemented it for Andriod as requested, but it would be completely fine to move it if it is better to place the check at a different location. Where do you suggest we should move this check to? Thanks.

Either misc or bugprone or a new posix module.

Sounds good. Thanks for the clarification. The check has been moved to 'bugprone' module.

[gribozavr]

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

21 ↗

(On Diff #206273)

why not if (posix_fadvise() != 0) ?

Otherwise it looks like adding a variable is necessary for a fix.

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

8 ↗

(On Diff #206273)

typedef decltype(sizeof(char)) size_t;

67 ↗

(On Diff #206273)

What about

if (posix_fadvise() >= 0) { ... } else { ... }

?

[jcai19]

Move the check from android to bugprone.

[jcai19]

Update a test case.

[jcai19]

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

39 ↗

(On Diff #205897)

After some offline discussion, I agree fixing the simple cases should be fine as programers should verify if the fixes proposed by clang-tidy are correct.

[gribozavr]

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

50

Please add tests for fix-its.

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

97

Shouldn't there be a warning in the else branch?

[jcai19]

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

50

The fix-it is triggered when cases like posix_*(...) < 0 are detected (except posix_openpt) , which are currently covered in the unit test. The test runtime will make sure fix-its are applied.

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

97

This check only matches calls to the POSIX functions in the global scope, not member functions or functions defined within namespaces. Although in the global scope, this particular case will still pass as there won't be a `<` binary operator for the else branch so no matching will happen.

[gribozavr]

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

50

Could you add CHECK-FIXES lines to validate the fixit directly?

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

97

Sorry, yes, that's what I meant -- if we warn on < 0, then we should warn on the else branch of >=. I just commented on the wrong test -- sorry about that.

[jcai19]

Update the check to catch redundant code and update the warning message to be more speicific.

[jcai19]

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

97

Thanks for the clarification. Actually >= 0 is always true since the call return 0 on success and a positive value on error, so the else branch will never be reached. I have update my check and the warning message to reflect this. Good catch!

[jcai19]

Update test names.

[gribozavr]

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

62

80 columns, please.

[jcai19]

Update warning messages.

[gribozavr]

Good improvements!

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

22

Please clang-format (the whole file).

57

"non-negative"

"the comparison always evaluates to false because %0 always returns non-negative values"

63

I'd suggest "the comparison always evaluates to true because %0 always returns a non-negative value"

[jcai19]

clang-format

[jcai19]

Update warning messages.

[gribozavr]

Thanks! Do you have commit access? Do you want me to commit this patch for you?

[jcai19]

Update CHECK-MESSAGES messages accordingly.

[jcai19]

In D63623#1565377, @gribozavr wrote:

Thanks! Do you have commit access? Do you want me to commit this patch for you?

Totally missed your message yesterday. Sorry about that. I do not have the access so it will be great if you can commit the change. Thanks a lot.

[gribozavr]

Committed revision 365007.

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

 #include "LambdaFunctionNameCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MisplacedOperatorInStrlenInAllocCheck.h"
 #include "MisplacedWideningCastCheck.h"
 #include "MoveForwardingReferenceCheck.h"
 #include "MultipleStatementMacroCheck.h"
 #include "ParentVirtualCallCheck.h"
+#include "PosixReturnCheck.h"
 #include "SizeofContainerCheck.h"
 #include "SizeofExpressionCheck.h"
 #include "StringConstructorCheck.h"
 #include "StringIntegerAssignmentCheck.h"
 #include "StringLiteralWithEmbeddedNulCheck.h"
 #include "SuspiciousEnumUsageCheck.h"
 #include "SuspiciousMemsetUsageCheck.h"
 #include "SuspiciousMissingCommaCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<MoveForwardingReferenceCheck>(
         "bugprone-move-forwarding-reference");
     CheckFactories.registerCheck<MultipleStatementMacroCheck>(
         "bugprone-multiple-statement-macro");
     CheckFactories.registerCheck<cppcoreguidelines::NarrowingConversionsCheck>(
         "bugprone-narrowing-conversions");
     CheckFactories.registerCheck<ParentVirtualCallCheck>(
         "bugprone-parent-virtual-call");
+    CheckFactories.registerCheck<PosixReturnCheck>(
+        "bugprone-posix-return");
     CheckFactories.registerCheck<SizeofContainerCheck>(
         "bugprone-sizeof-container");
     CheckFactories.registerCheck<SizeofExpressionCheck>(
         "bugprone-sizeof-expression");
     CheckFactories.registerCheck<StringConstructorCheck>(
         "bugprone-string-constructor");
     CheckFactories.registerCheck<StringIntegerAssignmentCheck>(
         "bugprone-string-integer-assignment");

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

add_clang_library(clangTidyBugproneModule
   LambdaFunctionNameCheck.cpp
   MacroParenthesesCheck.cpp
   MacroRepeatedSideEffectsCheck.cpp
   MisplacedOperatorInStrlenInAllocCheck.cpp
   MisplacedWideningCastCheck.cpp
   MoveForwardingReferenceCheck.cpp
   MultipleStatementMacroCheck.cpp
   ParentVirtualCallCheck.cpp
+  PosixReturnCheck.cpp
   SizeofContainerCheck.cpp
   SizeofExpressionCheck.cpp
   StringConstructorCheck.cpp
   StringIntegerAssignmentCheck.cpp
   StringLiteralWithEmbeddedNulCheck.cpp
   SuspiciousEnumUsageCheck.cpp
   SuspiciousMemsetUsageCheck.cpp
   SuspiciousMissingCommaCheck.cpp

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

+//===--- 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
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_POSIX_RETURN_CHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+class PosixReturnCheck: public ClangTidyCheck {
+public:
+  PosixReturnCheck(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_POSIX_RETURN_CHECK_H

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

+//===--- PosixReturnCheck.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 "../utils/Matchers.h"
+#include "PosixReturnCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+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("binop"),
+      this);
+  Finder->addMatcher(
+      binaryOperator(
+          anyOf(hasOperatorName("=="),
+                hasOperatorName("!="),
+                hasOperatorName("<="),
+                hasOperatorName("<")
+               ),
+          hasLHS(callExpr(callee(functionDecl(matchesName("^::posix_"), unless(hasName("::posix_openpt")))))),
+          hasRHS(unaryOperator(hasOperatorName("-"), hasUnaryOperand(integerLiteral())))).bind("binop"),
+      this);
+}
+
+void PosixReturnCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *LessThanZeroOp = Result.Nodes.getNodeAs<BinaryOperator>("ltzop")) {
+    SourceLocation OperatorLoc = LessThanZeroOp->getOperatorLoc();
+    diag(OperatorLoc, "POSIX functions (except posix_openpt) never return negative values")
+        << FixItHint::CreateReplacement(OperatorLoc, Twine(">").str());
+    return;
+  }
+  const auto *BinOp = Result.Nodes.getNodeAs<BinaryOperator>("binop");
+  diag(BinOp->getOperatorLoc(), "POSIX functions (except posix_openpt) never return negative values");
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

 
 - New :doc:`bugprone-branch-clone
   <clang-tidy/checks/bugprone-branch-clone>` check.
 
   Checks for repeated branches in ``if/else if/else`` chains, consecutive
   repeated branches in ``switch`` statements and indentical true and false
   branches in conditional operators.
 
+- New :doc:`bugprone-posix-return
+  <clang-tidy/checks/android-posix-return>` check.
+
+  Checks if any calls to POSIX functions (except ``posix_openpt``) expect negative
+  return values
+
 - New :doc:`google-readability-avoid-underscore-in-googletest-name
   <clang-tidy/checks/google-readability-avoid-underscore-in-googletest-name>`
   check.
 
   Checks whether there are underscores in googletest test and test case names in
   test macros, which is prohibited by the Googletest FAQ.
 
 - New :doc:`google-objc-avoid-nsobject-new
- New :doc:`modernize-use-trailing-return-type
 
   Rewrites function signatures to use a trailing return type.
 
 - The :doc:`misc-throw-by-value-catch-by-reference
   <clang-tidy/checks/misc-throw-by-value-catch-by-reference.rst>` now supports
   `WarnOnLargeObject` and `MaxSize` options to warn on any large trivial
   object caught by value.
 
 - Added `UseAssignment` option to :doc:`cppcoreguidelines-pro-type-member-init`
 
   If set to true, the check will provide fix-its with literal initializers
   (``int i = 0;``) instead of curly braces (``int i{};``).
 
 Improvements to include-fixer
 -----------------------------
 
 - New :doc:`openmp-exception-escape
   <clang-tidy/checks/openmp-exception-escape>` check.
 
   Analyzes OpenMP Structured Blocks and checks that no exception escapes

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.
+
+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/docs/clang-tidy/checks/list.rst

.. toctree::
    bugprone-lambda-function-name
    bugprone-macro-parentheses
    bugprone-macro-repeated-side-effects
    bugprone-misplaced-operator-in-strlen-in-alloc
    bugprone-misplaced-widening-cast
    bugprone-move-forwarding-reference
    bugprone-multiple-statement-macro
    bugprone-parent-virtual-call
+   bugprone-posix-return
    bugprone-sizeof-container
    bugprone-sizeof-expression
    bugprone-string-constructor
    bugprone-string-integer-assignment
    bugprone-string-literal-with-embedded-nul
    bugprone-suspicious-enum-usage
    bugprone-suspicious-memset-usage
    bugprone-suspicious-missing-comma

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 long off_t;
+typedef decltype(sizeof(int)) size_t;
+typedef int pid_t;
+typedef struct __posix_spawn_file_actions* posix_spawn_file_actions_t;
+typedef struct __posix_spawnattr* posix_spawnattr_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[]);
+
+void warningLEZero() {
+  if (posix_fadvise(0, 0, 0, 0) < 0) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning:
+  if (posix_fadvise(0, 0, 0, 0) <= 0) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning:
+  if (posix_fallocate(0, 0, 0) < 0) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:32: warning:
+  if (posix_madvise(NULL, 0, 0) < 0) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning:
+  if (posix_memalign(NULL, 0, 0) < 0) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning:
+  if (posix_spawn(NULL, NULL, NULL, NULL, {NULL}, {NULL}) < 0) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:59: warning:
+  if (posix_spawnp(NULL, NULL, NULL, NULL, {NULL}, {NULL}) < 0) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:60: warning:
+}
+
+void warningEqualsNegative() {
+  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_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:
+}
+
+void WarningWithMacro() {
+  if (posix_fadvise(0, 0, 0, 0) < ZERO) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning:
+  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:
+}
+
+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);
+
+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) {}
+}
+
+} // namespace i
+
+class G {
+ public:
+  int posix_fadvise(int fd, off_t offset, off_t len, int advice);
+
+  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) {}
+  }
+};