[clang-tidy] Add check to catch comparisons in TEMP_FAILURE_RETRY

[george.burgess.iv]

Summary

TEMP_FAILURE_RETRY(x) is a macro that executes x until (x) != -1 || errno != EINTR, and evaluates to the result of the last execution of x. It is provided by both glibc and Bionic, and sometimes is defined outside of libc.

I've been told that a somewhat common mistake people make with this is to include a condition in TEMP_FAILURE_RETRY's argument, e.g.

// Wrote
TEMP_FAILURE_RETRY(foo() == 1)
// Meant
TEMP_FAILURE_RETRY(foo()) == 1

...In the former case, the TEMP_FAILURE_RETRY is useless, since (x == 1) != -1 is always true. It will appear to work, though, since TEMP_FAILURE_RETRY will execute foo() at least once, and it'll evaluate to the value of the condition.

We do have a warning in clang to catch things like (x == y) == -1 (-Wtautological-constant-out-of-range-compare). This warns about these comparisons in C++ on Bionic. It doesn't catch them in C, since typeof(x == y) is an int in C, and it doesn't catch them when using glibc, since glibc just uses a long int to store the result of (x).

[Eugene.Zelenko]

clang-tidy/bugprone/ComparisonInTempFailureRetryCheck.cpp

51 ↗

(On Diff #140312)

Please add // namespace and remove one empty line below.

docs/ReleaseNotes.rst

60 ↗

(On Diff #140312)

Please use alphabetical order in new checks list. Also rebase from trunk and use :doc: and adjust link.

docs/clang-tidy/checks/bugprone-comparison-in-temp-failure-retry.rst

6 ↗

(On Diff #140312)

Please make first statement same as in Release Notes.

[Eugene.Zelenko]

Will be good idea to clarify where TEMP_FAILURE_RETRY come from.

[george.burgess.iv]

Addressed feedback.

[george.burgess.iv]

Thanks!

Will be good idea to clarify where TEMP_FAILURE_RETRY come from.

Updated the CL summary and added "TEMP_FAILURE_RETRY is a macro provided by both glibc and Bionic." to ComparisonInTempFailureRetryCheck.h. Did you have anything else in mind?

docs/clang-tidy/checks/bugprone-comparison-in-temp-failure-retry.rst

6 ↗

(On Diff #140312)

Interpreted "statement" as "sentence". Please let me know if you meant to include "Having such a use...", as well

[JonasToth]

You noted, that the C++ warning would catch this case, but does not get triggered in C. Would it be wort to generalize the concern and have a warning that catch the comparison, regardless of the macro?

Do other major libraries define a similar macro but name it differently? If so, including there names would be worthwhile.

clang-tidy/bugprone/ComparisonInTempFailureRetryCheck.cpp

22 ↗

(On Diff #140324)

I think this matcher already exists.

https://github.com/JonasToth/clang-tools-extra/blob/master/clang-tidy/utils/Matchers.h

isEqualityOperator would work? See the other matchers there too. If the matchers there would not do it, maybe add one there.

[george.burgess.iv]

Addressed feedback

[george.burgess.iv]

Thanks for the feedback!

You noted, that the C++ warning would catch this case, but does not get triggered in C. Would it be wort to generalize the concern and have a warning that catch the comparison, regardless of the macro?

I'm not quite sure how we would go about that with confidence. The code we'd need to catch looks something like:

typeof(foo() == y) x;
/* snip */
bar(x == -1); // warning: comparison is pointless

If we could tell that x's type was inferred from a comparison op, we could warn here. However, if the user used x as anything but a C++ bool in /* snip */, the warning would be incorrect. We could maybe do some sort of range analysis by walking the AST, but that sounds like more like a static analyzer thing.

...And none of that would catch glibc's TEMP_FAILURE_RETRY macro, unless we wanted to apply said analysis to all integral variables.

Do other major libraries define a similar macro but name it differently? If so, including there names would be worthwhile.

Dunno. A quick search for alternatives on MacOS said "no; #define it yourself," and I know so little about Windows that I have no clue if a TEMP_FAILURE_RETRY-like macro would even be reasonable there. :)

If you have anything in mind, I'm happy to add it.

[JonasToth]

I'm not quite sure how we would go about that with confidence. The code we'd need to catch looks something like:

typeof(foo() == y) x;
/* snip */
bar(x == -1); // warning: comparison is pointless

If we could tell that x's type was inferred from a comparison op, we could warn here. However, if the user used x as anything but a C++ bool in /* snip */, the warning would be incorrect. We could maybe do some sort of range analysis by walking the AST, but that sounds like more like a static analyzer thing.

...And none of that would catch glibc's TEMP_FAILURE_RETRY macro, unless we wanted to apply said analysis to all integral variables.

Yes that sounds reasonable. As i am no good C programer i have little insight in these issues. Please correct me if i am wrong!

C89 has no bool type and no stdbool.h but it has been added to later standards? That means the generalization could theoretically be done for later C standards, because we could check if the bool typedef had been used? If yes, would the check benefit?

I do not want to overthink the whole issue, because your check addresses a valid usecase and has general appliance. But if we can do more/better, we should do it :)

If you have anything in mind, I'm happy to add it.

No clue :D I think @aaron.ballman knows C good and might be a better reviewer for this code in general :)

[george.burgess.iv]

C89 has no bool type and no stdbool.h but it has been added to later standards? That means the generalization could theoretically be done for later C standards, because we could check if the bool typedef had been used? If yes, would the check benefit?

Sure, and we do complain about x == -1 if the type of x is _Bool, but the type of x == y appears to be an int in all of {c89, c99, c11}: https://godbolt.org/g/BciGZT :)

[JonasToth]

Alright. Thank you for clarification :)

clang-tidy/bugprone/ComparisonInTempFailureRetryCheck.cpp

75 ↗

(On Diff #140459)

You can match RHS in your matcher by adding another bind for the binaryOperator.

76 ↗

(On Diff #140459)

this assertion is redundant with the new matcher.

78 ↗

(On Diff #140459)

You could even provide a fixit to do this. But this can be done in later patches, too.

test/clang-tidy/bugprone-comparison-in-temp-failure-retry.c

1 ↗

(On Diff #140459)

Could you please add a test with control structures, like loops you used in your example?

5 ↗

(On Diff #140459)

I think you could add one test, that shows using long int instead of typeof is diagnosed, given glibc uses this approach (or even copy there macro)

[george.burgess.iv]

Addressed feedback

clang-tidy/bugprone/ComparisonInTempFailureRetryCheck.cpp

78 ↗

(On Diff #140459)

I initially tried this, but a complete solution appears to be nontrivial when nested macros start to happen. Added a FIXME nonetheless. :)

test/clang-tidy/bugprone-comparison-in-temp-failure-retry.c

1 ↗

(On Diff #140459)

Added the control_flow function

5 ↗

(On Diff #140459)

Added a variant with long int near the bottom.

[JonasToth]

LGTM, but @aaron.ballman, @alexfh or someone else should review it before comitting.

[alexfh]

The TEMP_FAILURE_RETRY macro is specific to the GNU C library (and environments that attempt to mimic it). The generic bugprone- module is not the best place for this check. I suggest moving it to android- as other similar checks are already there (and I suspect that your interest in this check is also related to android?). Alternatively, if there are other checks specific to the GNU C library planned, we could add a new module for it.

clang-tidy/bugprone/ComparisonInTempFailureRetryCheck.cpp

77 ↗

(On Diff #140675)

Diagnostic messages are not full sentences and should not start with a capital letter.

docs/clang-tidy/checks/bugprone-comparison-in-temp-failure-retry.rst

7 ↗

(On Diff #140675)

The documentation should provide some context w.r.t what the TEMP_FAILURE_RETRY macro is and where it comes from (maybe also link to its documentation).

[george.burgess.iv]

Addressed feedback

[george.burgess.iv]

Thanks for the feedback!

and I suspect that your interest in this check is also related to android?

Yup :)

Alternatively, if there are other checks specific to the GNU C library planned, we could add a new module for it.

I have nothing planned, so I'm happy with this sitting under android-.

docs/clang-tidy/checks/bugprone-comparison-in-temp-failure-retry.rst

7 ↗

(On Diff #140675)

Added a paragraph after this one.

[aaron.ballman]

LGTM with a few minor nits to be fixed.

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

4 ↗

(On Diff #141687)

Underlining is off here.

25 ↗

(On Diff #141687)

s/Since/Because

[george.burgess.iv]

Address feedback

[george.burgess.iv]

Thanks!

I plan to commit this tomorrow, to give time for any last-minute comments.

Thanks again to everyone for the review. :)

[alexfh]

LG. Thanks!

clang-tools-extra/trunk/clang-tidy/android/AndroidTidyModule.cpp

 #include "CloexecEpollCreate1Check.h"
 #include "CloexecEpollCreateCheck.h"
 #include "CloexecFopenCheck.h"
 #include "CloexecInotifyInit1Check.h"
 #include "CloexecInotifyInitCheck.h"
 #include "CloexecMemfdCreateCheck.h"
 #include "CloexecOpenCheck.h"
 #include "CloexecSocketCheck.h"
+#include "ComparisonInTempFailureRetryCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace android {
 
 /// This module is for Android specific checks.
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<CloexecInotifyInitCheck>(
         "android-cloexec-inotify-init");
     CheckFactories.registerCheck<CloexecInotifyInit1Check>(
         "android-cloexec-inotify-init1");
     CheckFactories.registerCheck<CloexecMemfdCreateCheck>(
         "android-cloexec-memfd-create");
     CheckFactories.registerCheck<CloexecOpenCheck>("android-cloexec-open");
     CheckFactories.registerCheck<CloexecSocketCheck>("android-cloexec-socket");
+    CheckFactories.registerCheck<ComparisonInTempFailureRetryCheck>(
+        "android-comparison-in-temp-failure-retry");
   }
 };
 
 // Register the AndroidTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<AndroidModule>
     X("android-module", "Adds Android platform checks.");
 
 } // namespace android
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the AndroidModule.
 volatile int AndroidModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tools-extra/trunk/clang-tidy/android/CMakeLists.txt

add_clang_library(clangTidyAndroidModule
   CloexecEpollCreateCheck.cpp
   CloexecDupCheck.cpp
   CloexecFopenCheck.cpp
   CloexecInotifyInit1Check.cpp
   CloexecInotifyInitCheck.cpp
   CloexecMemfdCreateCheck.cpp
   CloexecOpenCheck.cpp
   CloexecSocketCheck.cpp
+  ComparisonInTempFailureRetryCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

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

+//===--- 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

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

+//===--- 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<BinaryOperator>("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

clang-tools-extra/trunk/docs/ReleaseNotes.rst

 
 - New module `abseil` for checks related to the `Abseil <https://abseil.io>`_
   library.
 
 - New module ``portability``.
 
 - New module ``zircon`` for checks related to Fuchsia's Zircon kernel.
 
+- New :doc:`android-comparison-in-temp-failure-retry
+  <clang-tidy/checks/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
   <clang-tidy/checks/bugprone-parent-virtual-call>` check
 
   Detects and fixes calls to grand-...parent virtual methods instead of calls
   to overridden parent's virtual methods.
 
 - New :doc:`bugprone-throw-keyword-missing
   <clang-tidy/checks/bugprone-throw-keyword-missing>` check

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

+.. 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
+<https://www.gnu.org/software/libc/manual/html_node/Interrupted-Primitives.html>`_
+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.
+  }

clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

.. toctree::
    android-cloexec-epoll-create
    android-cloexec-epoll-create1
    android-cloexec-fopen
    android-cloexec-inotify-init
    android-cloexec-inotify-init1
    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
    bugprone-bool-pointer-implicit-conversion
    bugprone-copy-constructor-init
    bugprone-dangling-handle
    bugprone-fold-init-type
    bugprone-forward-declaration-namespace

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

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