[clang-tools-extra] r365007 - [clang-tidy] new check: bugprone-posix-return
Dmitri Gribenko via cfe-commits
cfe-commits at lists.llvm.org
Wed Jul 3 02:20:19 PDT 2019
Author: gribozavr
Date: Wed Jul 3 02:20:18 2019
New Revision: 365007
URL: http://llvm.org/viewvc/llvm-project?rev=365007&view=rev
Log:
[clang-tidy] new check: bugprone-posix-return
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.
Reviewers: JonasToth, gribozavr, alexfh, hokein
Reviewed By: gribozavr
Subscribers: Eugene.Zelenko, lebedev.ri, llozano, george.burgess.iv, xazax.hun, srhines, mgorny, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D63623
Patch by Jian Cai.
Added:
clang-tools-extra/trunk/clang-tidy/bugprone/PosixReturnCheck.cpp
clang-tools-extra/trunk/clang-tidy/bugprone/PosixReturnCheck.h
clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-posix-return.rst
clang-tools-extra/trunk/test/clang-tidy/bugprone-posix-return.cpp
Modified:
clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
clang-tools-extra/trunk/docs/ReleaseNotes.rst
clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
Modified: clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp?rev=365007&r1=365006&r2=365007&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp Wed Jul 3 02:20:18 2019
@@ -31,6 +31,7 @@
#include "MoveForwardingReferenceCheck.h"
#include "MultipleStatementMacroCheck.h"
#include "ParentVirtualCallCheck.h"
+#include "PosixReturnCheck.h"
#include "SizeofContainerCheck.h"
#include "SizeofExpressionCheck.h"
#include "StringConstructorCheck.h"
@@ -104,6 +105,8 @@ public:
"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>(
Modified: clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt?rev=365007&r1=365006&r2=365007&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt Wed Jul 3 02:20:18 2019
@@ -23,6 +23,7 @@ add_clang_library(clangTidyBugproneModul
MoveForwardingReferenceCheck.cpp
MultipleStatementMacroCheck.cpp
ParentVirtualCallCheck.cpp
+ PosixReturnCheck.cpp
SizeofContainerCheck.cpp
SizeofExpressionCheck.cpp
StringConstructorCheck.cpp
Added: clang-tools-extra/trunk/clang-tidy/bugprone/PosixReturnCheck.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/bugprone/PosixReturnCheck.cpp?rev=365007&view=auto
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/bugprone/PosixReturnCheck.cpp (added)
+++ clang-tools-extra/trunk/clang-tidy/bugprone/PosixReturnCheck.cpp Wed Jul 3 02:20:18 2019
@@ -0,0 +1,82 @@
+//===--- 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 "PosixReturnCheck.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 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(
+ 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, "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<BinaryOperator>("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<BinaryOperator>("binop");
+ diag(BinOp->getOperatorLoc(), "%0 only returns non-negative values")
+ << getFunctionSpelling(Result, "binop");
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
Added: clang-tools-extra/trunk/clang-tidy/bugprone/PosixReturnCheck.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/bugprone/PosixReturnCheck.h?rev=365007&view=auto
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/bugprone/PosixReturnCheck.h (added)
+++ clang-tools-extra/trunk/clang-tidy/bugprone/PosixReturnCheck.h Wed Jul 3 02:20:18 2019
@@ -0,0 +1,30 @@
+//===--- 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
Modified: clang-tools-extra/trunk/docs/ReleaseNotes.rst
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/ReleaseNotes.rst?rev=365007&r1=365006&r2=365007&view=diff
==============================================================================
--- clang-tools-extra/trunk/docs/ReleaseNotes.rst (original)
+++ clang-tools-extra/trunk/docs/ReleaseNotes.rst Wed Jul 3 02:20:18 2019
@@ -125,6 +125,12 @@ Improvements to clang-tidy
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:`fuchsia-default-arguments-calls
<clang-tidy/checks/fuchsia-default-arguments-calls>` check.
Added: clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-posix-return.rst
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-posix-return.rst?rev=365007&view=auto
==============================================================================
--- clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-posix-return.rst (added)
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-posix-return.rst Wed Jul 3 02:20:18 2019
@@ -0,0 +1,20 @@
+.. 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) {
Modified: clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst?rev=365007&r1=365006&r2=365007&view=diff
==============================================================================
--- clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst (original)
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst Wed Jul 3 02:20:18 2019
@@ -58,6 +58,7 @@ Clang-Tidy Checks
bugprone-move-forwarding-reference
bugprone-multiple-statement-macro
bugprone-parent-virtual-call
+ bugprone-posix-return
bugprone-sizeof-container
bugprone-sizeof-expression
bugprone-string-constructor
Added: clang-tools-extra/trunk/test/clang-tidy/bugprone-posix-return.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/bugprone-posix-return.cpp?rev=365007&view=auto
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/bugprone-posix-return.cpp (added)
+++ clang-tools-extra/trunk/test/clang-tidy/bugprone-posix-return.cpp Wed Jul 3 02:20:18 2019
@@ -0,0 +1,127 @@
+// 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 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
+}
+
+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
+}
+
+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:
+}
+
+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:
+}
+
+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) {}
+ }
+};
More information about the cfe-commits
mailing list