diff --git a/clang-tools-extra/clang-tidy/android/ComparisonInTempFailureRetryCheck.h b/clang-tools-extra/clang-tidy/android/ComparisonInTempFailureRetryCheck.h --- a/clang-tools-extra/clang-tidy/android/ComparisonInTempFailureRetryCheck.h +++ b/clang-tools-extra/clang-tidy/android/ComparisonInTempFailureRetryCheck.h @@ -10,6 +10,9 @@ #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_COMPARISONINTEMPFAILURERETRYCHECK_H #include "../ClangTidyCheck.h" +#include "llvm/ADT/SmallVector.h" +#include "llvm/ADT/StringRef.h" +#include namespace clang { namespace tidy { @@ -22,10 +25,14 @@ /// 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) {} + ComparisonInTempFailureRetryCheck(StringRef Name, ClangTidyContext *Context); + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + const std::string RawRetryList; + SmallVector RetryMacros; }; } // namespace android diff --git a/clang-tools-extra/clang-tidy/android/ComparisonInTempFailureRetryCheck.cpp b/clang-tools-extra/clang-tidy/android/ComparisonInTempFailureRetryCheck.cpp --- a/clang-tools-extra/clang-tidy/android/ComparisonInTempFailureRetryCheck.cpp +++ b/clang-tools-extra/clang-tidy/android/ComparisonInTempFailureRetryCheck.cpp @@ -18,32 +18,17 @@ namespace tidy { namespace android { -namespace { -AST_MATCHER(BinaryOperator, isRHSATempFailureRetryArg) { - if (!Node.getBeginLoc().isMacroID()) - return false; - - const SourceManager &SM = Finder->getASTContext().getSourceManager(); - if (!SM.isMacroArgExpansion(Node.getRHS()->IgnoreParenCasts()->getBeginLoc())) - return false; - - const LangOptions &Opts = Finder->getASTContext().getLangOpts(); - SourceLocation LocStart = Node.getBeginLoc(); - 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; - } +ComparisonInTempFailureRetryCheck::ComparisonInTempFailureRetryCheck( + StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + RawRetryList(Options.get("RetryMacros", "TEMP_FAILURE_RETRY")) { + StringRef(RawRetryList).split(RetryMacros, ",", -1, false); +} - LocStart = Invocation; - } - return false; +void ComparisonInTempFailureRetryCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "RetryMacros", RawRetryList); } -} // namespace void ComparisonInTempFailureRetryCheck::registerMatchers(MatchFinder *Finder) { // Both glibc's and Bionic's TEMP_FAILURE_RETRY macros structurally look like: @@ -63,15 +48,43 @@ Finder->addMatcher( binaryOperator(hasOperatorName("="), hasRHS(ignoringParenCasts( - binaryOperator(isComparisonOperator()).bind("binop"))), - isRHSATempFailureRetryArg()), + binaryOperator(isComparisonOperator()).bind("inner")))) + .bind("outer"), 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"); + StringRef RetryMacroName; + const auto &Node = *Result.Nodes.getNodeAs("outer"); + if (!Node.getBeginLoc().isMacroID()) + return; + + const SourceManager &SM = *Result.SourceManager; + if (!SM.isMacroArgExpansion(Node.getRHS()->IgnoreParenCasts()->getBeginLoc())) + return; + + const LangOptions &Opts = Result.Context->getLangOpts(); + SourceLocation LocStart = Node.getBeginLoc(); + 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 && + llvm::is_contained(RetryMacros, Tok.getRawIdentifier())) { + RetryMacroName = Tok.getRawIdentifier(); + break; + } + } + + LocStart = Invocation; + } + if (RetryMacroName.empty()) + return; + + const auto &Inner = *Result.Nodes.getNodeAs("inner"); + diag(Inner.getOperatorLoc(), "top-level comparison in %0") << RetryMacroName; // FIXME: FixIts would be nice, but potentially nontrivial when nested macros // happen, e.g. `TEMP_FAILURE_RETRY(IS_ZERO(foo()))` diff --git a/clang-tools-extra/docs/clang-tidy/checks/android-comparison-in-temp-failure-retry.rst b/clang-tools-extra/docs/clang-tidy/checks/android-comparison-in-temp-failure-retry.rst --- a/clang-tools-extra/docs/clang-tidy/checks/android-comparison-in-temp-failure-retry.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/android-comparison-in-temp-failure-retry.rst @@ -34,3 +34,10 @@ while (TEMP_FAILURE_RETRY(read(STDIN_FILENO, cs, sizeof(cs))) != 0) { // Do something with cs. } + +Options +------- + +.. option:: RetryMacros + + A comma-separated list of the names of retry macros to be checked. diff --git a/clang-tools-extra/test/clang-tidy/checkers/android-comparison-in-temp-failure-retry-custom-macro.c b/clang-tools-extra/test/clang-tidy/checkers/android-comparison-in-temp-failure-retry-custom-macro.c new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/android-comparison-in-temp-failure-retry-custom-macro.c @@ -0,0 +1,46 @@ +// RUN: %check_clang_tidy %s android-comparison-in-temp-failure-retry %t -- -config="{CheckOptions: [{key: android-comparison-in-temp-failure-retry.RetryMacros, value: 'MY_TEMP_FAILURE_RETRY,MY_OTHER_TEMP_FAILURE_RETRY'}]}" + +#define MY_TEMP_FAILURE_RETRY(x) \ + ({ \ + typeof(x) __z; \ + do \ + __z = (x); \ + while (__z == -1); \ + __z; \ + }) + +#define MY_OTHER_TEMP_FAILURE_RETRY(x) \ + ({ \ + typeof(x) __z; \ + do \ + __z = (x); \ + while (__z == -1); \ + __z; \ + }) + +int foo(); +int bar(int a); + +void with_custom_macro() { + MY_TEMP_FAILURE_RETRY(foo()); + MY_TEMP_FAILURE_RETRY(foo() == 1); + // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: top-level comparison in MY_TEMP_FAILURE_RETRY + MY_TEMP_FAILURE_RETRY((foo())); + MY_TEMP_FAILURE_RETRY((int)(foo() == 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: top-level comparison in MY_TEMP_FAILURE_RETRY + MY_TEMP_FAILURE_RETRY((bar(foo() == 1))); + MY_TEMP_FAILURE_RETRY((int)((bar(foo() == 1)) == 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:49: warning: top-level comparison in MY_TEMP_FAILURE_RETRY +} + +void with_other_custom_macro() { + MY_OTHER_TEMP_FAILURE_RETRY(foo()); + MY_OTHER_TEMP_FAILURE_RETRY(foo() == 1); + // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: top-level comparison in MY_OTHER_TEMP_FAILURE_RETRY + MY_OTHER_TEMP_FAILURE_RETRY((foo())); + MY_OTHER_TEMP_FAILURE_RETRY((int)(foo() == 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:43: warning: top-level comparison in MY_OTHER_TEMP_FAILURE_RETRY + MY_OTHER_TEMP_FAILURE_RETRY((bar(foo() == 1))); + MY_OTHER_TEMP_FAILURE_RETRY((int)((bar(foo() == 1)) == 1)); + // CHECK-MESSAGES: :[[@LINE-1]]:55: warning: top-level comparison in MY_OTHER_TEMP_FAILURE_RETRY +}