[clang-tidy] bugprone-infinite-loop: Fix false positives with volatile addresses.

[NoQ]

Summary

Low-level code may occasionally deal with direct access by concrete addresses such as 0x1234. Values at these addresses act like globals: they can change at any time. They typically wear volatile qualifiers.

Suppress all warnings on loops with conditions that involve casting anything to a pointer-to-...-pointer-to-volatile type.

We should probably suppress loops that dereference concrete addresses regardless of volatile qualifiers but it's pretty difficult to figure out whether the address is indeed concrete (say, it may have an unknown offset, eg. (char *)(0x1234 + i), and now it can be interpreted as either ((char *)0x1234)[i] or ((char *)i)[0x1234] so it's unclear whether this should be suppressed). I also suspect that such code's behavior is undefined so this isn't a very important case to support. So for now i'm sticking to supporting the explicitly volatile-qualified case which seems straightforward.

The closely related bugprone-redundant-branch-condition check doesn't seem to be affected. I added a test just in case.

[whisperity]

What happens for something like the following? Maybe it's also worth a test case?

bool wait_analogue_pin(volatile int* address, int threshold) {
  while (*address < threshold) {}
  return true;
}

[gribozavr2]

What happens for something like the following? Maybe it's also worth a test case?

WDYT about the test on line 614?

[whisperity]

In D108808#2968742, @gribozavr2 wrote:

In D108808#2968721, @whisperity wrote:

What happens for something like the following? Maybe it's also worth a test case?

WDYT about the test on line 614?

Ah! The fact that the actual value was there misled me.

[NoQ]

In D108808#2968721, @whisperity wrote:

What happens for something like the following? Maybe it's also worth a test case?

bool wait_analogue_pin(volatile int* address, int threshold) {
  while (*address < threshold) {}
  return true;
}

These checkers dodge these cases by noticing that the loop condition relies on pointer-type variables which, regardless of volatileness, may potentially alias with other things in the program and therefore be mutated at any moment without us noticing. The problem with concrete addresses wasn't that the check didn't know pointers cause problems, but that there weren't any variables in the expression to pay attention to. So I taught this sub-system to recognize these non-variables.

Banning pointer defererence operations entirely may still be a nicer solution that'd also allow us to simplify some code. I'll take a look.

clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp

if (const auto *Var = dyn_cast<VarDecl>(DRE->getDecl())) {
       return hasPtrOrReferenceInFunc(Func, Var) ||
              isChanged(LoopStmt, Var, Context);
       // FIXME: Track references.
     }
   } else if (isa<MemberExpr, CallExpr,
                  ObjCIvarRefExpr, ObjCPropertyRefExpr, ObjCMessageExpr>(Cond)) {
     // FIXME: Handle MemberExpr.
     return true;
+  } else if (const auto *CE = dyn_cast<CastExpr>(Cond)) {
+    QualType T = CE->getType();
+    while (true) {
+      if (T.isVolatileQualified())
+        return true;
+
+      if (!T->isAnyPointerType() && !T->isReferenceType())
+        break;
+
+      T = T->getPointeeType();
+    }
   }
 
   return false;
 }
 
 /// Return whether at least one variable of `Cond` changed in `LoopStmt`.
 static bool isAtLeastOneCondVarChanged(const Decl *Func, const Stmt *LoopStmt,
                                        const Stmt *Cond, ASTContext *Context) {

clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp

 void test_structured_bindings_bad() {
   int x = 0;
   AggregateWithValue val { x };
   auto &[y] = val;
   for (; x < 10; ++y) {
       // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (x) are updated in the loop body [bugprone-infinite-loop]
   }
 }
+
+void test_volatile_cast() {
+  // This is a no-op cast. Clang ignores the qualifier, we should too.
+  for (int i = 0; (volatile int)i < 10;) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (i) are updated in the loop body [bugprone-infinite-loop]
+  }
+}
+
+void test_volatile_concrete_address(int i, int size) {
+  // No warning. The value behind the volatile concrete address
+  // is beyond our control. It may change at any time.
+  for (; *((volatile int *)0x1234) < size;) {
+  }
+
+  for (; *((volatile int *)(0x1234 + i)) < size;) {
+  }
+
+  for (; **((volatile int **)0x1234) < size;) {
+  }
+
+  volatile int *x = (volatile int *)0x1234;
+  for (; *x < 10;) {
+  }
+
+  // FIXME: This one should probably also be suppressed.
+  // Whatever the developer is doing here, they can do that again anywhere else
+  // which basically makes it a global.
+  for (; *(int *)0x1234 < size;) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (size) are updated in the loop body [bugprone-infinite-loop]
+  }
+}

clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp

 
 void relational_operator_reversed() {
   if (peopleInTheBuilding > 1) {
     if (1 < peopleInTheBuilding) {
       doSomething();
     }
   }
 }
+
+void volatile_concrete_address() {
+  // No warning. The value behind the volatile concrete address
+  // is beyond our control. It may change at any time.
+  if (*(volatile int *)0x1234) {
+    if (*(volatile int *)0x1234) {
+      doSomething();
+    }
+  }
+}