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;
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;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
