Allow to specify macro names for android-comparison-in-temp-failure-retry.

[fmayer]

Summary

Some projects do not use the TEMP_FAILURE_RETRY macro but define their own
one, as not to depend on glibc / Bionic details. By allowing the user to override the
list of macros, these projects can also benefit from this check.

[george.burgess.iv]

Concept and implementation LGTM. Please wait for comment from +alexfh before landing, since I think they have more ownership over clang-tidy in general than I do :)

[fmayer]

Hey, any updates on this? :)

[srhines]

clang-tools-extra/test/clang-tidy/checkers/android-comparison-in-temp-failure-retry.c

1 ↗

(On Diff #275450)

Ideally there should either be another RUN macro here without your new options, or there should be a separate test/file for checking here, since we'd like to make sure that the default behavior still works.

28 ↗

(On Diff #275450)

Don't you want a check for MY_TEMP_FAILURE_RETRY for cases like this too (to make sure that it is detecting them)?

[fmayer]

Added new file for the tests. This way, we keep the tests for the normal case as well.

clang-tools-extra/test/clang-tidy/checkers/android-comparison-in-temp-failure-retry.c

1 ↗

(On Diff #275450)

Left this file alone, and added a new file for custom macros.

28 ↗

(On Diff #275450)

Checking for more cases in the other file.

[srhines]

Thanks for creating the new test, and for making this more flexible. Everything else looks good here.

[fmayer]

Thanks for the review! Should I wait for Alex to take a look, or is it fine like this?

[srhines]

In D83144#2268760, @fmayer wrote:

Thanks for the review! Should I wait for Alex to take a look, or is it fine like this?

George added @alexfh a while ago, but if it's ok with you, maybe give him until Tuesday or Wednesday next week in case he wants to respond?

[alexfh]

Looks good modulo comment.

clang-tools-extra/test/clang-tidy/checkers/android-comparison-in-temp-failure-retry-custom-macro.c

25

Are the extra parentheses required for the macro to work? Can it be just MY_TEMP_FAILURE_RETRY(foo()); ?

[fmayer]

Added more simple cases to the test.

clang-tools-extra/test/clang-tidy/checkers/android-comparison-in-temp-failure-retry-custom-macro.c

25

Added the more simple case as well.

[fmayer]

I am not very experienced with Phabricator, but I think this needs another approval for the latest revision :) Thanks in advance!

[fmayer]

I do not have commit access. Could someone submit this patch please?

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

 //===--- ComparisonInTempFailureRetryCheck.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_ANDROID_COMPARISONINTEMPFAILURERETRYCHECK_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_COMPARISONINTEMPFAILURERETRYCHECK_H
 
 #include "../ClangTidyCheck.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringRef.h"
+#include <string>
 
 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) {}
+  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<StringRef, 5> RetryMacros;
 };
 
 } // namespace android
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_COMPARISONINTEMPFAILURERETRYCHECK_H

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

 #include "clang/Lex/Lexer.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 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;
-}
-} // namespace
+void ComparisonInTempFailureRetryCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "RetryMacros", RawRetryList);
 
 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(isComparisonOperator()).bind("binop"))),
-                     isRHSATempFailureRetryArg()),
+                         binaryOperator(isComparisonOperator()).bind("inner"))))
+          .bind("outer"),
       this);
 }
 
 void ComparisonInTempFailureRetryCheck::check(
     const MatchFinder::MatchResult &Result) {
-  const auto &BinOp = *Result.Nodes.getNodeAs<BinaryOperator>("binop");
-  diag(BinOp.getOperatorLoc(), "top-level comparison in TEMP_FAILURE_RETRY");
+  StringRef RetryMacroName;
+  const auto &Node = *Result.Nodes.getNodeAs<BinaryOperator>("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<BinaryOperator>("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()))`
 }
 
 } // namespace android
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/clang-tidy/checks/android-comparison-in-temp-failure-retry.rst

 ``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.
   }
+
+Options
+-------
+
+.. option:: RetryMacros
+
+   A comma-separated list of the names of retry macros to be checked.

clang-tools-extra/test/clang-tidy/checkers/android-comparison-in-temp-failure-retry-custom-macro.c

+// 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((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((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
+}