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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: alexfh, gribozavr2, aaron.ballman, xazax.hun.
Herald added subscribers: martong, mgehre, rnkovacs.
NoQ requested review of this revision.
Herald added a project: clang-tools-extra.

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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D108808

Files:
  clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
@@ -1387,3 +1387,13 @@
     }
   }
 }
+
+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();
+    }
+  }
+}
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp
@@ -591,3 +591,35 @@
       // 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]
+  }
+}
+
Index: clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
@@ -65,6 +65,17 @@
                  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;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D108808.369040.patch
Type: text/x-patch
Size: 2982 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20210827/514097b3/attachment-0001.bin>