Index: clang-tidy/bugprone/BugproneTidyModule.cpp =================================================================== --- clang-tidy/bugprone/BugproneTidyModule.cpp +++ clang-tidy/bugprone/BugproneTidyModule.cpp @@ -13,6 +13,7 @@ #include "ArgumentCommentCheck.h" #include "AssertSideEffectCheck.h" #include "BoolPointerImplicitConversionCheck.h" +#include "ComparisonInTempFailureRetryCheck.h" #include "CopyConstructorInitCheck.h" #include "DanglingHandleCheck.h" #include "FoldInitTypeCheck.h" @@ -60,6 +61,8 @@ "bugprone-assert-side-effect"); CheckFactories.registerCheck( "bugprone-bool-pointer-implicit-conversion"); + CheckFactories.registerCheck( + "bugprone-comparison-in-temp-failure-retry"); CheckFactories.registerCheck( "bugprone-copy-constructor-init"); CheckFactories.registerCheck( Index: clang-tidy/bugprone/CMakeLists.txt =================================================================== --- clang-tidy/bugprone/CMakeLists.txt +++ clang-tidy/bugprone/CMakeLists.txt @@ -5,6 +5,7 @@ AssertSideEffectCheck.cpp BoolPointerImplicitConversionCheck.cpp BugproneTidyModule.cpp + ComparisonInTempFailureRetryCheck.cpp CopyConstructorInitCheck.cpp DanglingHandleCheck.cpp FoldInitTypeCheck.cpp Index: clang-tidy/bugprone/ComparisonInTempFailureRetryCheck.h =================================================================== --- /dev/null +++ clang-tidy/bugprone/ComparisonInTempFailureRetryCheck.h @@ -0,0 +1,34 @@ +//===--- ComparisonInTempFailureRetryCheck.h - clang-tidy--------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_COMPARISONINTEMPFAILURERETRYCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_COMPARISONINTEMPFAILURERETRYCHECK_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace bugprone { + +/// Attempts to catch calls to TEMP_FAILURE_RETRY with a top-level comparison +/// operation, like `TEMP_FAILURE_RETRY(read(...) != N)`. In these cases, the +/// comparison should go outside of the TEMP_FAILURE_RETRY. +class ComparisonInTempFailureRetryCheck : public ClangTidyCheck { +public: + ComparisonInTempFailureRetryCheck(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_COMPARISONINTEMPFAILURERETRYCHECK_H Index: clang-tidy/bugprone/ComparisonInTempFailureRetryCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/bugprone/ComparisonInTempFailureRetryCheck.cpp @@ -0,0 +1,85 @@ +//===--- ComparisonInTempFailureRetryCheck.cpp - clang-tidy----------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "ComparisonInTempFailureRetryCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Lex/Lexer.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace bugprone { + +namespace { +AST_MATCHER(Expr, isBinOpComparison) { + const auto *BinOp = dyn_cast(Node.IgnoreParenCasts()); + return BinOp && BinOp->isComparisonOp(); +} + +AST_MATCHER(BinaryOperator, isRHSATempFailureRetryArg) { + if (!Node.getLocStart().isMacroID()) + return false; + + const SourceManager &SM = Finder->getASTContext().getSourceManager(); + if (!SM.isMacroArgExpansion(Node.getRHS()->IgnoreParenCasts()->getLocStart())) + return false; + + const LangOptions &Opts = Finder->getASTContext().getLangOpts(); + SourceLocation LocStart = Node.getLocStart(); + while (LocStart.isMacroID()) { + SourceLocation Invocation = SM.getImmediateMacroCallerLoc(LocStart); + Token Tok; + if (!Lexer::getRawToken(SM.getSpellingLoc(Invocation), Tok, SM, Opts, + /*IgnoreWhiteSpace=*/true)) { + if (Tok.getKind() == tok::raw_identifier && + Tok.getRawIdentifier() == "TEMP_FAILURE_RETRY") + return true; + } + + LocStart = Invocation; + } + return false; +} +} + + +void ComparisonInTempFailureRetryCheck::registerMatchers(MatchFinder *Finder) { + // Both glibc's and Bionic's TEMP_FAILURE_RETRY macros structurally look like: + // + // #define TEMP_FAILURE_RETRY(x) ({ \ + // typeof(x) y; \ + // do y = (x); \ + // while (y == -1 && errno != EINTR); \ + // y; \ + // }) + // + // It's unclear how to walk up the AST from inside the expansion of `x`, and + // we need to not complain about things like TEMP_FAILURE_RETRY(foo(x == 1)), + // so we just match the assignment of `y = (x)` and inspect `x` from there. + Finder->addMatcher(binaryOperator(hasOperatorName("="), + hasRHS(isBinOpComparison()), + isRHSATempFailureRetryArg()) + .bind("assign"), + this); +} + +void ComparisonInTempFailureRetryCheck::check(const MatchFinder::MatchResult &Result) { + const auto &Assign = *Result.Nodes.getNodeAs("assign"); + const auto &RHS = *cast(Assign.getRHS()->IgnoreParenCasts()); + assert(RHS.isComparisonOp()); + + diag(RHS.getOperatorLoc(), + "Top-level comparisons should be moved out of TEMP_FAILURE_RETRY"); +} + +} // namespace bugprone +} // namespace tidy +} // namespace clang Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -57,6 +57,12 @@ Improvements to clang-tidy -------------------------- +- New `bugprone-comparison-in-temp-failure-retry + `_ check + + Diagnoses comparisons that appear to be incorrectly placed in the argument to + the `TEMP_FAILURE_RETRY` macro. + - New module ``portability``. - New module ``zircon`` for checks related to Fuchsia's Zircon kernel. Index: docs/clang-tidy/checks/bugprone-comparison-in-temp-failure-retry.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/bugprone-comparison-in-temp-failure-retry.rst @@ -0,0 +1,30 @@ +.. title:: clang-tidy - bugprone-comparison-in-temp-failure-retry + +bugprone-comparison-in-temp-failure-retry +========================================= + +Checks whether a use of ``TEMP_FAILURE_RETRY`` has a top-level comparison in its +argument. Having such a use is incorrect in the vast majority of cases, and +will often silently defeat the purpose of the ``TEMP_FAILURE_RETRY`` macro. + +Example buggy usage looks like: + +.. code-block:: c + + char cs[1]; + while (TEMP_FAILURE_RETRY(read(STDIN_FILENO, cs, sizeof(cs)) != 0)) { + // Do something with cs. + } + +Since TEMP_FAILURE_RETRY will check for whether the result *of the comparison* +is ``-1``, and retry if so. + +If you encounter this, the fix is simple: lift the comparison out of the +``TEMP_FAILURE_RETRY`` argument, like so: + +.. code-block:: c + + char cs[1]; + while (TEMP_FAILURE_RETRY(read(STDIN_FILENO, cs, sizeof(cs))) != 0) { + // Do something with cs. + } Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -21,6 +21,7 @@ bugprone-argument-comment bugprone-assert-side-effect bugprone-bool-pointer-implicit-conversion + bugprone-comparison-in-temp-failure-retry bugprone-copy-constructor-init bugprone-dangling-handle bugprone-fold-init-type Index: test/clang-tidy/bugprone-comparison-in-temp-failure-retry.c =================================================================== --- /dev/null +++ test/clang-tidy/bugprone-comparison-in-temp-failure-retry.c @@ -0,0 +1,109 @@ +// RUN: %check_clang_tidy %s bugprone-comparison-in-temp-failure-retry %t + +#define TEMP_FAILURE_RETRY(x) \ + ({ \ + typeof(x) __z; \ + do \ + __z = (x); \ + while (__z == -1); \ + __z; \ + }) + +int foo(); +int bar(int a); + +void test() { + int i; + TEMP_FAILURE_RETRY((i = foo())); + TEMP_FAILURE_RETRY(foo()); + TEMP_FAILURE_RETRY((foo())); + + TEMP_FAILURE_RETRY(foo() == 1); + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: Top-level comparisons should be moved out of TEMP_FAILURE_RETRY [bugprone-comparison-in-temp-failure-retry] + TEMP_FAILURE_RETRY((foo() == 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: Top-level comparisons should be moved out of TEMP_FAILURE_RETRY + TEMP_FAILURE_RETRY((int)(foo() == 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: Top-level comparisons should be moved out of TEMP_FAILURE_RETRY + + TEMP_FAILURE_RETRY(bar(foo() == 1)); + TEMP_FAILURE_RETRY((bar(foo() == 1))); + TEMP_FAILURE_RETRY((bar(foo() == 1)) == 1); + // CHECK-MESSAGES: :[[@LINE-1]]:40: warning: Top-level comparisons should be moved out of TEMP_FAILURE_RETRY + TEMP_FAILURE_RETRY(((bar(foo() == 1)) == 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:41: warning: Top-level comparisons should be moved out of TEMP_FAILURE_RETRY + TEMP_FAILURE_RETRY((int)((bar(foo() == 1)) == 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:46: warning: Top-level comparisons should be moved out of TEMP_FAILURE_RETRY + +#define INDIRECT TEMP_FAILURE_RETRY + INDIRECT(foo()); + INDIRECT((foo() == 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: Top-level comparisons should be moved out of TEMP_FAILURE_RETRY + INDIRECT(bar(foo() == 1)); + INDIRECT((int)((bar(foo() == 1)) == 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:36: warning: Top-level comparisons should be moved out of TEMP_FAILURE_RETRY + +#define TFR(x) TEMP_FAILURE_RETRY(x) + TFR(foo()); + TFR((foo() == 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: Top-level comparisons should be moved out of TEMP_FAILURE_RETRY + TFR(bar(foo() == 1)); + TFR((int)((bar(foo() == 1)) == 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: Top-level comparisons should be moved out of TEMP_FAILURE_RETRY + +#define ADD_TFR(x) (1 + TEMP_FAILURE_RETRY(x) + 1) + ADD_TFR(foo()); + ADD_TFR(foo() == 1); + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: Top-level comparisons should be moved out of TEMP_FAILURE_RETRY + + ADD_TFR(bar(foo() == 1)); + ADD_TFR((int)((bar(foo() == 1)) == 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: Top-level comparisons should be moved out of TEMP_FAILURE_RETRY + +#define ADDP_TFR(x) (1 + TEMP_FAILURE_RETRY((x)) + 1) + ADDP_TFR(foo()); + ADDP_TFR((foo() == 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: Top-level comparisons should be moved out of TEMP_FAILURE_RETRY + + ADDP_TFR(bar(foo() == 1)); + ADDP_TFR((int)((bar(foo() == 1)) == 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:36: warning: Top-level comparisons should be moved out of TEMP_FAILURE_RETRY + +#define MACRO TEMP_FAILURE_RETRY(foo() == 1) + MACRO; + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Top-level comparisons should be moved out of TEMP_FAILURE_RETRY + + // Be sure that being a macro arg doesn't mess with this. +#define ID(x) (x) + ID(ADDP_TFR(bar(foo() == 1))); + ID(ADDP_TFR(bar(foo() == 1) == 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: Top-level comparisons should be moved out of TEMP_FAILURE_RETRY + ID(MACRO); + // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: Top-level comparisons should be moved out of TEMP_FAILURE_RETRY + +#define CMP(x) x == 1 + TEMP_FAILURE_RETRY(CMP(foo())); + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: Top-level comparisons should be moved out of TEMP_FAILURE_RETRY +} + +// I can't find a case where TEMP_FAILURE_RETRY is implemented like this, but if +// we can cheaply support it, I don't see why not. +void obscured_temp_failure_retry() { +#undef TEMP_FAILURE_RETRY +#define IMPL(x) \ + ({ \ + typeof(x) __z; \ + do \ + __z = (x); \ + while (__z == -1); \ + __z; \ + }) + +#define IMPL2(x) IMPL(x) +#define TEMP_FAILURE_RETRY(x) IMPL2(x) + TEMP_FAILURE_RETRY((foo())); + TEMP_FAILURE_RETRY((int)(foo() == 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: Top-level comparisons should be moved out of TEMP_FAILURE_RETRY + TEMP_FAILURE_RETRY((bar(foo() == 1))); + TEMP_FAILURE_RETRY((int)((bar(foo() == 1)) == 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:46: warning: Top-level comparisons should be moved out of TEMP_FAILURE_RETRY +}