Index: clang-tools-extra/trunk/clang-tidy/android/AndroidTidyModule.cpp =================================================================== --- clang-tools-extra/trunk/clang-tidy/android/AndroidTidyModule.cpp +++ clang-tools-extra/trunk/clang-tidy/android/AndroidTidyModule.cpp @@ -22,6 +22,7 @@ #include "CloexecMemfdCreateCheck.h" #include "CloexecOpenCheck.h" #include "CloexecSocketCheck.h" +#include "ComparisonInTempFailureRetryCheck.h" using namespace clang::ast_matchers; @@ -50,6 +51,8 @@ "android-cloexec-memfd-create"); CheckFactories.registerCheck("android-cloexec-open"); CheckFactories.registerCheck("android-cloexec-socket"); + CheckFactories.registerCheck( + "android-comparison-in-temp-failure-retry"); } }; Index: clang-tools-extra/trunk/clang-tidy/android/CMakeLists.txt =================================================================== --- clang-tools-extra/trunk/clang-tidy/android/CMakeLists.txt +++ clang-tools-extra/trunk/clang-tidy/android/CMakeLists.txt @@ -15,6 +15,7 @@ CloexecMemfdCreateCheck.cpp CloexecOpenCheck.cpp CloexecSocketCheck.cpp + ComparisonInTempFailureRetryCheck.cpp LINK_LIBS clangAST Index: clang-tools-extra/trunk/clang-tidy/android/ComparisonInTempFailureRetryCheck.h =================================================================== --- clang-tools-extra/trunk/clang-tidy/android/ComparisonInTempFailureRetryCheck.h +++ clang-tools-extra/trunk/clang-tidy/android/ComparisonInTempFailureRetryCheck.h @@ -0,0 +1,36 @@ +//===--- 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_ANDROID_COMPARISONINTEMPFAILURERETRYCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_COMPARISONINTEMPFAILURERETRYCHECK_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace android { + +/// 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. +/// +/// TEMP_FAILURE_RETRY is a macro provided by both glibc and Bionic. +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 android +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_COMPARISONINTEMPFAILURERETRYCHECK_H Index: clang-tools-extra/trunk/clang-tidy/android/ComparisonInTempFailureRetryCheck.cpp =================================================================== --- clang-tools-extra/trunk/clang-tidy/android/ComparisonInTempFailureRetryCheck.cpp +++ clang-tools-extra/trunk/clang-tidy/android/ComparisonInTempFailureRetryCheck.cpp @@ -0,0 +1,84 @@ +//===--- 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 "../utils/Matchers.h" +#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 android { + +namespace { +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; +} +} // namespace + +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; \ + // }) + // + // (glibc uses `long int` instead of `typeof(x)` for the type of 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(ignoringParenCasts( + binaryOperator(matchers::isComparisonOperator()).bind("binop"))), + isRHSATempFailureRetryArg()), + this); +} + +void ComparisonInTempFailureRetryCheck::check( + const MatchFinder::MatchResult &Result) { + const auto &BinOp = *Result.Nodes.getNodeAs("binop"); + diag(BinOp.getOperatorLoc(), "top-level comparison in TEMP_FAILURE_RETRY"); + + // FIXME: FixIts would be nice, but potentially nontrivial when nested macros + // happen, e.g. `TEMP_FAILURE_RETRY(IS_ZERO(foo()))` +} + +} // namespace android +} // namespace tidy +} // namespace clang Index: clang-tools-extra/trunk/docs/ReleaseNotes.rst =================================================================== --- clang-tools-extra/trunk/docs/ReleaseNotes.rst +++ clang-tools-extra/trunk/docs/ReleaseNotes.rst @@ -69,6 +69,12 @@ - New module ``zircon`` for checks related to Fuchsia's Zircon kernel. +- New :doc:`android-comparison-in-temp-failure-retry + ` check + + Diagnoses comparisons that appear to be incorrectly placed in the argument to + the ``TEMP_FAILURE_RETRY`` macro. + - New :doc:`bugprone-parent-virtual-call ` check Index: clang-tools-extra/trunk/docs/clang-tidy/checks/android-comparison-in-temp-failure-retry.rst =================================================================== --- clang-tools-extra/trunk/docs/clang-tidy/checks/android-comparison-in-temp-failure-retry.rst +++ clang-tools-extra/trunk/docs/clang-tidy/checks/android-comparison-in-temp-failure-retry.rst @@ -0,0 +1,36 @@ +.. title:: clang-tidy - android-comparison-in-temp-failure-retry + +android-comparison-in-temp-failure-retry +======================================== + +Diagnoses comparisons that appear to be incorrectly placed in the argument to +the ``TEMP_FAILURE_RETRY`` macro. 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. + +For context, ``TEMP_FAILURE_RETRY`` is `a convenience macro +`_ +provided by both glibc and Bionic. Its purpose is to repeatedly run a syscall +until it either succeeds, or fails for reasons other than being interrupted. + +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. + } + +Because 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: clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst =================================================================== --- clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst +++ clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst @@ -17,6 +17,7 @@ android-cloexec-memfd-create android-cloexec-open android-cloexec-socket + android-comparison-in-temp-failure-retry boost-use-to-string bugprone-argument-comment bugprone-assert-side-effect Index: clang-tools-extra/trunk/test/clang-tidy/android-comparison-in-temp-failure-retry.c =================================================================== --- clang-tools-extra/trunk/test/clang-tidy/android-comparison-in-temp-failure-retry.c +++ clang-tools-extra/trunk/test/clang-tidy/android-comparison-in-temp-failure-retry.c @@ -0,0 +1,148 @@ +// RUN: %check_clang_tidy %s android-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 comparison in TEMP_FAILURE_RETRY [android-comparison-in-temp-failure-retry] + TEMP_FAILURE_RETRY((foo() == 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: top-level comparison in TEMP_FAILURE_RETRY + TEMP_FAILURE_RETRY((int)(foo() == 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: top-level comparison in 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 comparison in TEMP_FAILURE_RETRY + TEMP_FAILURE_RETRY(((bar(foo() == 1)) == 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:41: warning: top-level comparison in TEMP_FAILURE_RETRY + TEMP_FAILURE_RETRY((int)((bar(foo() == 1)) == 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:46: warning: top-level comparison in TEMP_FAILURE_RETRY + +#define INDIRECT TEMP_FAILURE_RETRY + INDIRECT(foo()); + INDIRECT((foo() == 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: top-level comparison in TEMP_FAILURE_RETRY + INDIRECT(bar(foo() == 1)); + INDIRECT((int)((bar(foo() == 1)) == 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:36: warning: top-level comparison in TEMP_FAILURE_RETRY + +#define TFR(x) TEMP_FAILURE_RETRY(x) + TFR(foo()); + TFR((foo() == 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: top-level comparison in TEMP_FAILURE_RETRY + TFR(bar(foo() == 1)); + TFR((int)((bar(foo() == 1)) == 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: top-level comparison in 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 comparison in TEMP_FAILURE_RETRY + + ADD_TFR(bar(foo() == 1)); + ADD_TFR((int)((bar(foo() == 1)) == 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: top-level comparison in 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 comparison in TEMP_FAILURE_RETRY + + ADDP_TFR(bar(foo() == 1)); + ADDP_TFR((int)((bar(foo() == 1)) == 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:36: warning: top-level comparison in TEMP_FAILURE_RETRY + +#define MACRO TEMP_FAILURE_RETRY(foo() == 1) + MACRO; + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: top-level comparison in 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 comparison in TEMP_FAILURE_RETRY + ID(MACRO); + // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: top-level comparison in TEMP_FAILURE_RETRY + +#define CMP(x) x == 1 + TEMP_FAILURE_RETRY(CMP(foo())); + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: top-level comparison in TEMP_FAILURE_RETRY +} + +// Be sure that it works inside of things like loops, if statements, etc. +void control_flow() { + do { + if (TEMP_FAILURE_RETRY(foo())) { + } + + if (TEMP_FAILURE_RETRY(foo() == 1)) { + // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: top-level comparison in TEMP_FAILURE_RETRY + } + + if (TEMP_FAILURE_RETRY(bar(foo() == 1))) { + } + + if (TEMP_FAILURE_RETRY(bar(foo() == 1) == 1)) { + // CHECK-MESSAGES: :[[@LINE-1]]:44: warning: top-level comparison in TEMP_FAILURE_RETRY + } + } while (TEMP_FAILURE_RETRY(foo() == 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: top-level comparison in TEMP_FAILURE_RETRY +} + +void with_nondependent_variable_type() { +#undef TEMP_FAILURE_RETRY +#define TEMP_FAILURE_RETRY(x) \ + ({ \ + long int __z; \ + do \ + __z = (x); \ + while (__z == -1); \ + __z; \ + }) + + TEMP_FAILURE_RETRY((foo())); + TEMP_FAILURE_RETRY((int)(foo() == 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: top-level comparison in 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 comparison in 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 comparison in 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 comparison in TEMP_FAILURE_RETRY +}