[PATCH] D40937: [clang-tidy] Infinite loop checker

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.

Could you run the check on llvm sources and post a summary of results here?

A few more inline comments.




Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:102
+  if (std::unique_ptr TheCFG =
+  CFG::buildCFG(nullptr, FunctionBody, , Options))
+Sequence = llvm::make_unique(TheCFG.get(), );

What does `nullptr` mean here? Please add an argument comment 
(`/*ArgumentName=*/nullptr, ...`).



Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:109-112
+  const auto *ContainingLambda =
+  Result.Nodes.getNodeAs("containing-lambda");
+  const auto *ContainingFunc =
+  Result.Nodes.getNodeAs("containing-func");

nit: Let's put these variable definitions into the corresponding `if` 
conditions to limit their scope. I'd also move the `if`s to the start of the 
function.



Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:122-123
+FunctionBody = ContainingFunc->getBody();
+  else
+return;
+

Instead I'd check that FunctionBody is not nullptr.



Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:148
+// (excluding the init stmt).
+if (const auto ForLoop = dyn_cast(LoopStmt)) {
+  if (ForLoop->getInc())

nit: `const auto *`



Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:150
+  if (ForLoop->getInc())
+Match = match(potentiallyModifyVarStmt(CondVar), *ForLoop->getInc(),
+  ASTCtx);

Any reason to store the result of the matching instead of returning early? Same 
below. Please also move the definition of the Match variable to where it's 
actually needed first time.



Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:175
+
+for (const auto  : Match) {
+  if (Sequence->potentiallyAfter(LoopStmt, ES.getNodeAs("escStmt")))

Please use the specific type instead of `auto`.



Comment at: clang-tidy/bugprone/InfiniteLoopCheck.h:37
+private:
+  bool updateSequence(Stmt *FunctionBody, ASTContext );
+  const Stmt *PrevFunctionBody;

You seem to have forgotten to update the header.


https://reviews.llvm.org/D40937



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40937: [clang-tidy] Infinite loop checker

[Peter Szecsi via Phabricator via cfe-commits]

szepet updated this revision to Diff 145160.
szepet marked 2 inline comments as done.
szepet added a comment.

Changes based on comments.


https://reviews.llvm.org/D40937

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/InfiniteLoopCheck.cpp
  clang-tidy/bugprone/InfiniteLoopCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-infinite-loop.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-infinite-loop.cpp

Index: test/clang-tidy/bugprone-infinite-loop.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-infinite-loop.cpp
@@ -0,0 +1,121 @@
+// RUN: %check_clang_tidy %s bugprone-infinite-loop %t
+
+void simple_infinite_loop1() {
+  int i = 0;
+  int j = 0;
+  while (i < 10) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the condition variable (i) is not updated in the loop body [bugprone-infinite-loop]
+j++;
+  }
+
+  do {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the condition variable (i) is not updated in the loop body
+j++;
+  } while (i < 10);
+
+  for (i = 0; i < 10; ++j) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the condition variable (i) is not updated in the loop body
+  }
+}
+
+void simple_infinite_loop2() {
+  int i = 0;
+  int j = 0;
+  int Limit = 10;
+  while (i < Limit) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: none of the condition variables (i, Limit) are updated in the loop body [bugprone-infinite-loop]
+j++;
+  }
+
+  do {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: none of the condition variables (i, Limit) are updated in the loop body
+j++;
+  } while (i < Limit);
+
+  for (i = 0; i < Limit; ++j) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: none of the condition variables (i, Limit) are updated in the loop body
+  }
+}
+
+void simple_not_infinite() {
+  int i = 0;
+  int Limit = 100;
+  while (i < Limit) { // Not an error since 'Limit' is updated
+Limit--;
+  }
+  do {
+Limit--;
+  } while (i < Limit);
+
+  for (i = 0; i < Limit; Limit--) {
+  }
+}
+
+void escape_before1() {
+  int i = 0;
+  int Limit = 100;
+  int *p = 
+  while (i < Limit) { // Not an error, since p is alias of i.
+*++p;
+  }
+
+  do {
+*++p;
+  } while (i < Limit);
+
+  for (i = 0; i < Limit; *++p) {
+;
+  }
+}
+
+void escape_before2() {
+  int i = 0;
+  int Limit = 100;
+  int *p = 
+  while (i < Limit) { // We do not warn since the var 'i' is escaped but it is
+  // an actual error, since the pointer 'p' is increased.
+*(p++);
+  }
+}
+
+void escape_after() {
+  int i = 0;
+  int j = 0;
+  int Limit = 10;
+
+  while (i < Limit) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: none of the condition variables (i, Limit) are updated in the loop body
+  }
+  int *p = 
+}
+
+int glob;
+void glob_var(int ) {
+  int i = 0, Limit = 100;
+  while (x < Limit) { // Not an error since 'x' can be an alias of glob.
+glob++;
+  }
+}
+
+void glob_var2() {
+  int i = 0, Limit = 100;
+  while (glob < Limit) { // Since 'glob' is declared out of the function we do not warn.
+i++;
+  }
+}
+
+struct X {
+  void memberExpr_test(int i) {
+while (i < m) { // False negative: No warning, since skipping the case where
+// a memberExpr can be found in the condition.
+  ;
+}
+  }
+
+  void memberExpr_test2(int i) {
+while (i < m) {
+  --m;
+}
+  }
+  int m;
+};
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -29,6 +29,7 @@
bugprone-forwarding-reference-overload
bugprone-inaccurate-erase
bugprone-incorrect-roundings
+   bugprone-infinite-loop
bugprone-integer-division
bugprone-lambda-function-name
bugprone-macro-parentheses
Index: docs/clang-tidy/checks/bugprone-infinite-loop.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/bugprone-infinite-loop.rst
@@ -0,0 +1,30 @@
+.. title:: clang-tidy - bugprone-infinite-loop
+
+bugprone-infinite-loop
+==
+
+Finds loops where none of the condition variables are updated in the body. This
+performs a very conservative check in order to avoid false positives and work
+only on integer types at the moment.
+
+Examples:
+
+.. code-block:: c++
+
+  void simple_infinite_loop() {
+int i = 0;
+int j = 0;
+int Limit = 10;
+while (i < Limit) { // Error, since none of the variables are updated.
+  j++;
+}
+  }
+
+  void escape_before() {
+int i = 0;
+int Limit = 100;
+int *p = 
+while (i < Limit) { // Not an error, since p is alias of i.
+  *++p;
+}
+  }
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -70,6 +70,12 @@
   Diagnoses 

[PATCH] D40937: [clang-tidy] Infinite loop checker

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.



Comment at: clang-tidy/bugprone/InfiniteLoopCheck.h:37
+private:
+  bool updateSequence(Stmt *FunctionBody, ASTContext );
+  const Stmt *PrevFunctionBody;

Why bool? The return value is not used anywhere.



Comment at: docs/ReleaseNotes.rst:68
+- New :doc:`bugprone-infinite-loop
+  ` 
check
+

There should be a trailing period in each of these `New ... check` items. I've 
updated the script in r331460 and the existing release notes in r331461. Please 
rebase.


https://reviews.llvm.org/D40937



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40937: [clang-tidy] Infinite loop checker

[Peter Szecsi via Phabricator via cfe-commits]

szepet updated this revision to Diff 143949.
szepet marked 2 inline comments as done.
szepet added a comment.

Changes made based on comments.
The CFG recreating problem is handled the following (only for this check):
Always store the last visited function and its CFG* (in form of the Sequence*) 
and check if we are visiting it again. If so, then the check reuses the 
previous one, if not, then replaces them. As far as I know the AST traverse 
done by the tidy fits this model (at least for this check, since it not uses 
narrowing matchers to other functions).
Sure, it would be better to find a general solution to this problem, and make 
the CFG reusable by every check which needs it, but I would left it for a 
follow-up (and a change like this probably would worth an own patch/review 
anyway).


https://reviews.llvm.org/D40937

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/InfiniteLoopCheck.cpp
  clang-tidy/bugprone/InfiniteLoopCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-infinite-loop.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-infinite-loop.cpp

Index: test/clang-tidy/bugprone-infinite-loop.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-infinite-loop.cpp
@@ -0,0 +1,121 @@
+// RUN: %check_clang_tidy %s bugprone-infinite-loop %t
+
+void simple_infinite_loop1() {
+  int i = 0;
+  int j = 0;
+  while (i < 10) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the condition variable (i) is not updated in the loop body [bugprone-infinite-loop]
+j++;
+  }
+
+  do {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the condition variable (i) is not updated in the loop body
+j++;
+  } while (i < 10);
+
+  for (i = 0; i < 10; ++j) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the condition variable (i) is not updated in the loop body
+  }
+}
+
+void simple_infinite_loop2() {
+  int i = 0;
+  int j = 0;
+  int Limit = 10;
+  while (i < Limit) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: none of the condition variables (i, Limit) are updated in the loop body [bugprone-infinite-loop]
+j++;
+  }
+
+  do {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: none of the condition variables (i, Limit) are updated in the loop body
+j++;
+  } while (i < Limit);
+
+  for (i = 0; i < Limit; ++j) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: none of the condition variables (i, Limit) are updated in the loop body
+  }
+}
+
+void simple_not_infinite() {
+  int i = 0;
+  int Limit = 100;
+  while (i < Limit) { // Not an error since 'Limit' is updated
+Limit--;
+  }
+  do {
+Limit--;
+  } while (i < Limit);
+
+  for (i = 0; i < Limit; Limit--) {
+  }
+}
+
+void escape_before1() {
+  int i = 0;
+  int Limit = 100;
+  int *p = 
+  while (i < Limit) { // Not an error, since p is alias of i.
+*++p;
+  }
+
+  do {
+*++p;
+  } while (i < Limit);
+
+  for (i = 0; i < Limit; *++p) {
+;
+  }
+}
+
+void escape_before2() {
+  int i = 0;
+  int Limit = 100;
+  int *p = 
+  while (i < Limit) { // We do not warn since the var 'i' is escaped but it is
+  // an actual error, since the pointer 'p' is increased.
+*(p++);
+  }
+}
+
+void escape_after() {
+  int i = 0;
+  int j = 0;
+  int Limit = 10;
+
+  while (i < Limit) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: none of the condition variables (i, Limit) are updated in the loop body
+  }
+  int *p = 
+}
+
+int glob;
+void glob_var(int ) {
+  int i = 0, Limit = 100;
+  while (x < Limit) { // Not an error since 'x' can be an alias of glob.
+glob++;
+  }
+}
+
+void glob_var2() {
+  int i = 0, Limit = 100;
+  while (glob < Limit) { // Since 'glob' is declared out of the function we do not warn.
+i++;
+  }
+}
+
+struct X {
+  void memberExpr_test(int i) {
+while (i < m) { // False negative: No warning, since skipping the case where
+// a memberExpr can be found in the condition.
+  ;
+}
+  }
+
+  void memberExpr_test2(int i) {
+while (i < m) {
+  --m;
+}
+  }
+  int m;
+};
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -28,6 +28,7 @@
bugprone-forwarding-reference-overload
bugprone-inaccurate-erase
bugprone-incorrect-roundings
+   bugprone-infinite-loop
bugprone-integer-division
bugprone-lambda-function-name
bugprone-macro-parentheses
Index: docs/clang-tidy/checks/bugprone-infinite-loop.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/bugprone-infinite-loop.rst
@@ -0,0 +1,30 @@
+.. title:: clang-tidy - bugprone-infinite-loop
+
+bugprone-infinite-loop
+==
+
+Finds loops where none of the condition variables are updated in the body. This
+performs a very 

[PATCH] D40937: [clang-tidy] Infinite loop checker

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.



Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:94-99
+  std::unique_ptr TheCFG =
+  CFG::buildCFG(nullptr, FunctionBody, , Options);
+  if (!TheCFG)
+return std::unique_ptr();
+
+  return llvm::make_unique(TheCFG.get(), );

A little suggestion:

  if (std::unique_ptr TheCFG = ...)
return llvm::make_unique(TheCFG.get(), );
  return {};



Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:128
+return;
+  std::unique_ptr Sequence = createSequence(FunctionBody, 
ASTCtx);
+

The main concern I have now is that the check may end up creating the CFG 
multiple times for the same function, which may be rather expensive in certain 
cases (consider a large function consisting of loops that trigger the matcher). 
It's hard to predict how common is this situation in real code. Do you see how 
this could be restructured to avoid unnecessary work?



Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:179
+  }
+  CondVarNames.resize(CondVarNames.size() - 2);
+

I suggest a more straightforward version:

​  std::string CondVarNames;
  for (const auto  : CondVarMatches) {
if (!CondVarNames.empty())
  CondVarNames.append(", ");
CondVarNames.append(CVM.getNodeAs("condvar")->getNameAsString());
​  }



https://reviews.llvm.org/D40937



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40937: [clang-tidy] Infinite loop checker

[Peter Szecsi via Phabricator via cfe-commits]

szepet updated this revision to Diff 141322.
szepet marked 4 inline comments as done.
szepet added a comment.

Addressed comments and readded the lost fixes.


https://reviews.llvm.org/D40937

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/InfiniteLoopCheck.cpp
  clang-tidy/bugprone/InfiniteLoopCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-infinite-loop.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-infinite-loop.cpp

Index: test/clang-tidy/bugprone-infinite-loop.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-infinite-loop.cpp
@@ -0,0 +1,121 @@
+// RUN: %check_clang_tidy %s bugprone-infinite-loop %t
+
+void simple_infinite_loop1() {
+  int i = 0;
+  int j = 0;
+  while (i < 10) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: The condition variable (i) is not updated in the loop body [bugprone-infinite-loop]
+j++;
+  }
+
+  do {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: The condition variable (i) is not updated in the loop body
+j++;
+  } while (i < 10);
+
+  for (i = 0; i < 10; ++j) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: The condition variable (i) is not updated in the loop body
+  }
+}
+
+void simple_infinite_loop2() {
+  int i = 0;
+  int j = 0;
+  int Limit = 10;
+  while (i < Limit) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: None of the condition variables (i, Limit) are updated in the loop body [bugprone-infinite-loop]
+j++;
+  }
+
+  do {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: None of the condition variables (i, Limit) are updated in the loop body
+j++;
+  } while (i < Limit);
+
+  for (i = 0; i < Limit; ++j) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: None of the condition variables (i, Limit) are updated in the loop body
+  }
+}
+
+void simple_not_infinite() {
+  int i = 0;
+  int Limit = 100;
+  while (i < Limit) { // Not an error since 'Limit' is updated
+Limit--;
+  }
+  do {
+Limit--;
+  } while (i < Limit);
+
+  for (i = 0; i < Limit; Limit--) {
+  }
+}
+
+void escape_before1() {
+  int i = 0;
+  int Limit = 100;
+  int *p = 
+  while (i < Limit) { // Not an error, since p is alias of i.
+*++p;
+  }
+
+  do {
+*++p;
+  } while (i < Limit);
+
+  for (i = 0; i < Limit; *++p) {
+;
+  }
+}
+
+void escape_before2() {
+  int i = 0;
+  int Limit = 100;
+  int *p = 
+  while (i < Limit) { // We do not warn since the var 'i' is escaped but it is
+  // an actual error, since the pointer 'p' is increased.
+*(p++);
+  }
+}
+
+void escape_after() {
+  int i = 0;
+  int j = 0;
+  int Limit = 10;
+
+  while (i < Limit) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: None of the condition variables (i, Limit) are updated in the loop body
+  }
+  int *p = 
+}
+
+int glob;
+void glob_var(int ) {
+  int i = 0, Limit = 100;
+  while (x < Limit) { // Not an error since 'x' can be an alias of glob.
+glob++;
+  }
+}
+
+void glob_var2() {
+  int i = 0, Limit = 100;
+  while (glob < Limit) { // Since 'glob' is declared out of the function we do not warn.
+i++;
+  }
+}
+
+struct X {
+  void memberExpr_test(int i) {
+while (i < m) { // False negative: No warning, since skipping the case where
+// a memberExpr can be found in the condition.
+  ;
+}
+  }
+
+  void memberExpr_test2(int i) {
+while (i < m) {
+  --m;
+}
+  }
+  int m;
+};
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -28,6 +28,7 @@
bugprone-forwarding-reference-overload
bugprone-inaccurate-erase
bugprone-incorrect-roundings
+   bugprone-infinite-loop
bugprone-integer-division
bugprone-lambda-function-name
bugprone-macro-parentheses
Index: docs/clang-tidy/checks/bugprone-infinite-loop.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/bugprone-infinite-loop.rst
@@ -0,0 +1,30 @@
+.. title:: clang-tidy - bugprone-infinite-loop
+
+bugprone-infinite-loop
+==
+
+Finds loops where none of the condition variables are updated in the body. This
+performs a very conservative check in order to avoid false positives and work
+only on integer types at the moment.
+
+Examples:
+
+.. code-block:: c++
+
+  void simple_infinite_loop() {
+int i = 0;
+int j = 0;
+int Limit = 10;
+while (i < Limit) { // Error, since none of the variables are updated.
+  j++;
+}
+  }
+
+  void escape_before() {
+int i = 0;
+int Limit = 100;
+int *p = 
+while (i < Limit) { // Not an error, since p is alias of i.
+  *++p;
+}
+  }
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -64,6 

[PATCH] D40937: [clang-tidy] Infinite loop checker

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.



Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:23-25
+static internal::Matcher loopEndingStmt() {
+  return stmt(anyOf(breakStmt(), returnStmt(), gotoStmt(), cxxThrowExpr()));
+}

This doesn't have to be a function. It can be a local variable or can be 
inlined into the callsite.



Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:32
+void InfiniteLoopCheck::registerMatchers(MatchFinder *Finder) {
+  const auto loopCondition = allOf(hasCondition(expr().bind("condition")),
+  anyOf(hasAncestor(lambdaExpr().bind("containing-lambda")),

http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

> Variable names should be nouns (as they represent state). The name should be 
> camel case, and start with an upper case letter (e.g. Leader or Boats).



Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:105
+  return llvm::make_unique(
+  *(new ExprSequence(TheCFG.get(), )));
+}

Why not just `llvm::make_unique(TheCFG.get(), )`?

It also may be that you somehow lost a bunch of fixes, since this comment has 
already been addressed at some point: 
https://reviews.llvm.org/D40937?id=125870#inline-357357



Comment at: clang-tidy/bugprone/InfiniteLoopCheck.cpp:186
+  diag(LoopStmt->getLocStart(),
+   "%plural{1:The condition variable|:None of the condition variables}0 "
+   "(%1) %plural{1:is not|:are}0 updated in the loop body")

Clang diagnostics (and clang-tidy warnings) are not complete sentences and 
shouldn't start with a capital letter.


https://reviews.llvm.org/D40937



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40937: [clang-tidy] Infinite loop checker

[Peter Szecsi via Phabricator via cfe-commits]

szepet added inline comments.



Comment at: clang-tidy/misc/InfiniteLoopCheck.cpp:121
+
+  Stmt *FunctionBody = nullptr;
+  if (ContainingLambda)

xazax.hun wrote:
> This could be pointer to const, right?
Since the createSequence uses it as a parameter for buildCFG which expects a 
Stmt*, I believe this is fine. (Or I could const cast is away on the call)


https://reviews.llvm.org/D40937



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40937: [clang-tidy] Infinite loop checker

[Peter Szecsi via Phabricator via cfe-commits]

szepet updated this revision to Diff 141132.
szepet marked 3 inline comments as done.
szepet added a comment.

Removed to bugprone category,
skipping memberExpr cases for now in order to avoid false positives,
other small changes based on review comments.


https://reviews.llvm.org/D40937

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/InfiniteLoopCheck.cpp
  clang-tidy/bugprone/InfiniteLoopCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-infinite-loop.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-infinite-loop.cpp

Index: test/clang-tidy/bugprone-infinite-loop.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-infinite-loop.cpp
@@ -0,0 +1,121 @@
+// RUN: %check_clang_tidy %s bugprone-infinite-loop %t
+
+void simple_infinite_loop1() {
+  int i = 0;
+  int j = 0;
+  while (i < 10) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: The condition variable (i) is not updated in the loop body [bugprone-infinite-loop]
+j++;
+  }
+
+  do {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: The condition variable (i) is not updated in the loop body
+j++;
+  } while (i < 10);
+
+  for (i = 0; i < 10; ++j) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: The condition variable (i) is not updated in the loop body
+  }
+}
+
+void simple_infinite_loop2() {
+  int i = 0;
+  int j = 0;
+  int Limit = 10;
+  while (i < Limit) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: None of the condition variables (i, Limit) are updated in the loop body [bugprone-infinite-loop]
+j++;
+  }
+
+  do {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: None of the condition variables (i, Limit) are updated in the loop body
+j++;
+  } while (i < Limit);
+
+  for (i = 0; i < Limit; ++j) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: None of the condition variables (i, Limit) are updated in the loop body
+  }
+}
+
+void simple_not_infinite() {
+  int i = 0;
+  int Limit = 100;
+  while (i < Limit) { // Not an error since 'Limit' is updated
+Limit--;
+  }
+  do {
+Limit--;
+  } while (i < Limit);
+
+  for (i = 0; i < Limit; Limit--) {
+  }
+}
+
+void escape_before1() {
+  int i = 0;
+  int Limit = 100;
+  int *p = 
+  while (i < Limit) { // Not an error, since p is alias of i.
+*++p;
+  }
+
+  do {
+*++p;
+  } while (i < Limit);
+
+  for (i = 0; i < Limit; *++p) {
+;
+  }
+}
+
+void escape_before2() {
+  int i = 0;
+  int Limit = 100;
+  int *p = 
+  while (i < Limit) { // We do not warn since the var 'i' is escaped but it is
+  // an actual error, since the pointer 'p' is increased.
+*(p++);
+  }
+}
+
+void escape_after() {
+  int i = 0;
+  int j = 0;
+  int Limit = 10;
+
+  while (i < Limit) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: None of the condition variables (i, Limit) are updated in the loop body
+  }
+  int *p = 
+}
+
+int glob;
+void glob_var(int ) {
+  int i = 0, Limit = 100;
+  while (x < Limit) { // Not an error since 'x' can be an alias of glob.
+glob++;
+  }
+}
+
+void glob_var2() {
+  int i = 0, Limit = 100;
+  while (glob < Limit) { // Since 'glob' is declared out of the function we do not warn.
+i++;
+  }
+}
+
+struct X {
+  void memberExpr_test(int i) {
+while(i < m) { // False negative: No warning, since skipping the case where
+   // a memberExpr can be found in the condition.
+  ;
+}
+  }
+
+  void memberExpr_test2(int i) {
+while(i < m) {
+  --m;
+}
+  }
+  int m;
+};
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -28,6 +28,7 @@
bugprone-forwarding-reference-overload
bugprone-inaccurate-erase
bugprone-incorrect-roundings
+   bugprone-infinite-loop
bugprone-integer-division
bugprone-lambda-function-name
bugprone-macro-parentheses
Index: docs/clang-tidy/checks/bugprone-infinite-loop.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/bugprone-infinite-loop.rst
@@ -0,0 +1,30 @@
+.. title:: clang-tidy - bugprone-infinite-loop
+
+bugprone-infinite-loop
+==
+
+Finds loops where none of the condition variables are updated in the body. This
+performs a very conservative check in order to avoid false positives and work
+only on integer types at the moment.
+
+Examples:
+
+.. code-block:: c++
+
+  void simple_infinite_loop() {
+int i = 0;
+int j = 0;
+int Limit = 10;
+while (i < Limit) { // Error, since none of the variables are updated.
+  j++;
+}
+  }
+
+  void escape_before() {
+int i = 0;
+int Limit = 100;
+int *p = 
+while (i < Limit) { // Not an error, since p is alias of i.
+  *++p;
+}
+  }
Index: docs/ReleaseNotes.rst

[PATCH] D40937: [clang-tidy] Infinite loop checker

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.

Please move this check to "bugprone-", since it seems to be an appropriate 
category for this check.


https://reviews.llvm.org/D40937



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40937: [clang-tidy] Infinite loop checker

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

I think, while the analyzer is more suitable for certain kinds of checks that 
require deep analysis, it is still useful to have quicker syntactic checks that 
can easily identify problems that are the results of typos or incorrectly 
modified copy and pasted code. I think this check is in that category.  Also, 
the original warning Peter mentioned does something similar but has some 
shortcomings.

The current implementation is not path sensitive. It uses flow sensitivity to 
check for escaping values.
If we would try to port this check to the static analyzer, the questions we 
would ask from the analyzer are universally quantified (e.g. for all path this 
variable does not escape and does not change). Unfortunately, it is not that 
easy with the current analyzer to answer such questions. The static analyzer is 
better with existential questions (e.g. there is a path such that the condition 
variables are not escaped and are unchanged in the loop). Using the latter 
formulation we might have a larger number of false positives because the 
analyzer sometimes hit infeasible paths.  In the first approach, the infeasible 
paths are less of a problem (they might cause false negatives but not false 
positives), but we need to be careful with all the peculiarities of the 
analyzer because it does not guarantee to discover all possible paths.

Hopefully, Devin will correct me if I'm wrong :)


https://reviews.llvm.org/D40937



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40937: [clang-tidy] Infinite loop checker

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: dcoughlin.
aaron.ballman added a subscriber: dcoughlin.
aaron.ballman added a comment.

I share the concerns that @Eugene.Zelenko brought up regarding this not being 
in the analyzer. This is a path sensitive problem that I'm not certain 
clang-tidy is the best home for. You cover many of the simple cases, but fail 
to cover cases like nested loops, unsigned integer wrapping, non-integral 
types, function calls, globals, etc. I'm not certain this check will generalize 
over time as part of clang-tidy in the way it would in the analyzer. I'd be 
curious to hear what @alexfh and @dcoughlin think, though.


https://reviews.llvm.org/D40937



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40937: [clang-tidy] Infinite loop checker

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang-tidy/misc/InfiniteLoopCheck.cpp:121
+
+  Stmt *FunctionBody = nullptr;
+  if (ContainingLambda)

This could be pointer to const, right?


https://reviews.llvm.org/D40937



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40937: [clang-tidy] Infinite loop checker

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

This does not support memberExprs as condition variables right now.

What happens if you have something like this:

  struct X {
  void f(int a) {
while(a < i) {
  --i;
} 
  }
  int i;
  };

I think you could extend the test cases with some classes.




Comment at: clang-tidy/misc/InfiniteLoopCheck.cpp:28
+void InfiniteLoopCheck::registerMatchers(MatchFinder *Finder) {
+  const auto loopCondition = []() {
+return allOf(hasCondition(expr().bind("condition")),

Do you need this to be a lambda? Can't you just use a local variable?



Comment at: clang-tidy/misc/InfiniteLoopCheck.cpp:31
+ anyOf(hasAncestor(lambdaExpr().bind("containing-lambda")),
+   hasAncestor(functionDecl().bind("containing-func"))),
+ unless(hasBody(hasDescendant(loopEndingStmt();

Maybe this is not too important, but you might also want to check for blocks 
here.



Comment at: clang-tidy/misc/InfiniteLoopCheck.cpp:48
+declRefExpr(to(varDecl(VarNodeMatcher)),
+  binaryOperator(anyOf(hasOperatorName("="), hasOperatorName("+="),
+   hasOperatorName("/="), hasOperatorName("*="),

This can be greatly simplified once https://reviews.llvm.org/D38921 is 
accepted. Maybe you could use that as a dependent revision? It should be close 
to be accepted.


https://reviews.llvm.org/D40937



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40937: [clang-tidy] Infinite loop checker

[Peter Szecsi via Phabricator via cfe-commits]

szepet added a comment.

In https://reviews.llvm.org/D40937#947760, @JVApen wrote:

> How does this check deal with atomic members?
>  ...


This patch only works on integer types. So, since the atomic is a non-supported 
type the check will skip that `while` loop.


https://reviews.llvm.org/D40937



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40937: [clang-tidy] Infinite loop checker

[Peter Szecsi via Phabricator via cfe-commits]

szepet updated this revision to Diff 125965.
szepet marked 9 inline comments as done.
szepet added a comment.

Updates based on comments.


https://reviews.llvm.org/D40937

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/InfiniteLoopCheck.cpp
  clang-tidy/misc/InfiniteLoopCheck.h
  clang-tidy/misc/MiscTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-infinite-loop.rst
  test/clang-tidy/misc-infinite-loop.cpp

Index: test/clang-tidy/misc-infinite-loop.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-infinite-loop.cpp
@@ -0,0 +1,114 @@
+// RUN: %check_clang_tidy %s misc-infinite-loop %t
+
+void simple_infinite_loop1() {
+  int i = 0;
+  int j = 0;
+  while (i < 10) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: The condition variable (i) is not updated in the loop body [misc-infinite-loop]
+j++;
+  }
+
+  do {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: The condition variable (i) is not updated in the loop body
+j++;
+  } while (i < 10);
+
+  for (i = 0; i < 10; ++j) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: The condition variable (i) is not updated in the loop body
+  }
+}
+
+void simple_infinite_loop2() {
+  int i = 0;
+  int j = 0;
+  int Limit = 10;
+  while (i < Limit) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: None of the condition variables (i, Limit) are updated in the loop body [misc-infinite-loop]
+j++;
+  }
+
+  do {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: None of the condition variables (i, Limit) are updated in the loop body
+j++;
+  } while (i < Limit);
+
+  for (i = 0; i < Limit; ++j) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: None of the condition variables (i, Limit) are updated in the loop body
+  }
+}
+
+void simple_not_infinite() {
+  int i = 0;
+  int Limit = 100;
+  while (i < Limit) { // Not an error since 'Limit' is updated
+Limit--;
+  }
+  do {
+Limit--;
+  } while (i < Limit);
+
+  for (i = 0; i < Limit; Limit--) {
+  }
+}
+
+void escape_before1() {
+  int i = 0;
+  int Limit = 100;
+  int *p = 
+  while (i < Limit) { // Not an error, since p is alias of i.
+*++p;
+  }
+
+  do {
+*++p;
+  } while (i < Limit);
+
+  for (i = 0; i < Limit; *++p) {
+  }
+}
+
+void escape_before2() {
+  int i = 0;
+  int Limit = 100;
+  int *p = 
+  while (i < Limit) { // We do not warn since the var 'i' is escaped but it is
+  // an actual error, since the pointer 'p' is increased.
+*(p++);
+  }
+}
+
+void escape_after() {
+  int i = 0;
+  int j = 0;
+  int Limit = 10;
+
+  while (i < Limit) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: None of the condition variables (i, Limit) are updated in the loop body
+  }
+  int *p = 
+}
+
+int glob;
+void glob_var(int ) {
+  int i = 0;
+  int Limit = 100;
+  while (x < Limit) { // Not an error since 'x' can be an alias of glob.
+glob++;
+  }
+}
+
+void glob_var2() {
+  int i = 0, Limit = 100;
+  while (glob < Limit) { // Since 'glob' is declared out of the function we do not warn.
+i++;
+  }
+}
+
+bool foo(int n);
+void fun_call() {
+  int i = 0;
+  int Limit = 100;
+  while (foo(i)) { // Do not warn, since foo can have state.
+Limit--;
+  }
+}
Index: docs/clang-tidy/checks/misc-infinite-loop.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-infinite-loop.rst
@@ -0,0 +1,30 @@
+.. title:: clang-tidy - misc-infinite-loop
+
+misc-infinite-loop
+==
+
+The check finds loops where none of the condition variables are updated in the
+body. This performs a very conservative check in order to avoid false positives
+and work only on integer types at the moment.
+
+Examples:
+
+.. code-block:: c++
+
+  void simple_infinite_loop() {
+int i = 0;
+int j = 0;
+int Limit = 10;
+while (i < Limit) { // Error, since none of the variables are updated.
+  j++;
+}
+  }
+
+  void escape_before() {
+int i = 0;
+int Limit = 100;
+int *p = 
+while (i < Limit) { // Not an error, since p is alias of i.
+  *++p;
+}
+  }
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -120,6 +120,7 @@
misc-definitions-in-headers
misc-forwarding-reference-overload
misc-incorrect-roundings
+   misc-infinite-loop
misc-lambda-function-name
misc-macro-parentheses
misc-macro-repeated-side-effects
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -158,6 +158,11 @@
   Finds uses of bitwise operations on signed integer types, which may lead to 
   undefined or implementation defined behaviour.
 
+- New `misc-infinite-loop
+  

[PATCH] D40937: [clang-tidy] Infinite loop checker

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang-tidy/misc/InfiniteLoopCheck.cpp:105
+  return llvm::make_unique(
+  *(new ExprSequence(TheCFG.get(), )));
+}

`make_unique` is a forwarding function, therefore there is no need to create an 
object and then move it. Instead you can simply forward the arguments to the 
constructor:
`return llvm::make_unique(TheCFG.get(), )`


https://reviews.llvm.org/D40937



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40937: [clang-tidy] Infinite loop checker

[JVApen via Phabricator via cfe-commits]

JVApen added a comment.

How does this check deal with atomic members?

struct A {
 std:: atomic a;

void f() const { while (a); }
};


https://reviews.llvm.org/D40937



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40937: [clang-tidy] Infinite loop checker

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/misc/InfiniteLoopCheck.cpp:94
+
+std::unique_ptr createSequence(Stmt *FunctionBody,
+ ASTContext ) {

Missing static?



Comment at: clang-tidy/misc/InfiniteLoopCheck.cpp:102
+  if (!TheCFG)
+return std::unique_ptr();
+

May be return {}?



Comment at: clang-tidy/misc/InfiniteLoopCheck.cpp:125
+
+  auto CondVarMatches =
+  match(findAll(declRefExpr(to(varDecl().bind("condvar", *Cond, 
ASTCtx);

const auto?



Comment at: clang-tidy/misc/InfiniteLoopCheck.cpp:130
+
+  for (auto  : CondVarMatches) {
+const VarDecl *CondVar = E.getNodeAs("condvar");

const auto?



Comment at: clang-tidy/misc/InfiniteLoopCheck.cpp:142
+// (excluding the init stmt).
+if (auto ForLoop = dyn_cast(LoopStmt)) {
+  if (ForLoop->getInc())

const auto?



Comment at: clang-tidy/misc/InfiniteLoopCheck.cpp:168
+*FunctionBody, ASTCtx);
+for (auto  : Match) {
+  if (Sequence->potentiallyAfter(LoopStmt, ES.getNodeAs("escStmt")))

const auto?



Comment at: clang-tidy/misc/InfiniteLoopCheck.cpp:176
+  std::string CondVarNames = "";
+  for (auto  : CondVarMatches) {
+CondVarNames += CVM.getNodeAs("condvar")->getNameAsString() + ", 
";

const auto?


https://reviews.llvm.org/D40937



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40937: [clang-tidy] Infinite loop checker

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In https://reviews.llvm.org/D40937#947658, @szepet wrote:

> Other note: If somebody would came up with an approach which can benefit from 
> the symbolic execution, these solutions could still live happily in the 
> different tools eg. UseAfterMove (tidy) and MisusedMovedObjectChecker 
> (analyzer).


I'm more concerned with path sensitivity and reporting. From my point of view 
UseAfterMove also belongs to Analyzer realm.

Actually //bugprone// module may be better choice then //misc//.




Comment at: docs/ReleaseNotes.rst:63
+
+  The check finds loops where none of the condition variables are updated in 
the
+  body.

Please remove //The check//. See other checks release notes and documentation 
as style examples.

Please rebase from trunk, since I sorted Clang-tidy notes recently, so just 
prefer to avoid this task in future :-)


https://reviews.llvm.org/D40937



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40937: [clang-tidy] Infinite loop checker

[Peter Szecsi via Phabricator via cfe-commits]

szepet added a comment.

Hi Eugen!
Good question, probably should have detailed it in the description.
This matcher based solution would not gain much benefit from the symbolic 
execution provided information. (I mean, it would mean a much different type of 
check on the states.) 
The main problems that the analyzer does not completely unroll the loops only 
the first steps and we always have information about the simulated path. 
However, detecting that some variables will surely not be modified requires a 
top level overview on the loop and the AST provides these informations. The one 
thing (that I can say right now) that can come handy is that we would able to 
detect more precisely the happened-before relation on the escape and the loop 
statements. Since the CFG can provide us fair enough information on this one, I 
do not think that this is enough reason to put this checker to the analyzer.

Other note: If somebody would came up with an approach which can benefit from 
the symbolic execution, these solutions could still live happily in the 
different tools eg. UseAfterMove (tidy) and MisusedMovedObjectChecker 
(analyzer).


https://reviews.llvm.org/D40937



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40937: [clang-tidy] Infinite loop checker

[Peter Szecsi via Phabricator via cfe-commits]

szepet updated this revision to Diff 125870.
szepet marked an inline comment as done.
szepet added a comment.

Updated the wording in the docs.


https://reviews.llvm.org/D40937

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/InfiniteLoopCheck.cpp
  clang-tidy/misc/InfiniteLoopCheck.h
  clang-tidy/misc/MiscTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-infinite-loop.rst
  test/clang-tidy/misc-infinite-loop.cpp

Index: test/clang-tidy/misc-infinite-loop.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-infinite-loop.cpp
@@ -0,0 +1,104 @@
+// RUN: %check_clang_tidy %s misc-infinite-loop %t
+
+void simple_infinite_loop1() {
+  int i = 0;
+  int j = 0;
+  while (i < 10) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: The condition variable (i) is not updated in the loop body [misc-infinite-loop]
+j++;
+  }
+
+  do {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: The condition variable (i) is not updated in the loop body
+j++;
+  } while (i < 10);
+
+  for (i = 0; i < 10; ++j) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: The condition variable (i) is not updated in the loop body
+  }
+}
+
+void simple_infinite_loop2() {
+  int i = 0;
+  int j = 0;
+  int Limit = 10;
+  while (i < Limit) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: None of the condition variables (i, Limit) are updated in the loop body [misc-infinite-loop]
+j++;
+  }
+
+  do {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: None of the condition variables (i, Limit) are updated in the loop body
+j++;
+  } while (i < Limit);
+
+  for (i = 0; i < Limit; ++j) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: None of the condition variables (i, Limit) are updated in the loop body
+  }
+}
+
+void simple_not_infinite() {
+  int i = 0;
+  int Limit = 100;
+  while (i < Limit) { // Not an error since 'Limit' is updated
+Limit--;
+  }
+  do {
+Limit--;
+  } while (i < Limit);
+
+  for (i = 0; i < Limit;Limit--) {
+  }
+}
+
+void escape_before1() {
+  int i = 0;
+  int Limit = 100;
+  int *p = 
+  while (i < Limit) { // Not an error, since p is alias of i.
+*++p;
+  }
+
+  do {
+*++p;
+  } while (i < Limit);
+
+  for (i = 0; i < Limit; *++p) {
+  }
+}
+
+void escape_before2() {
+  int i = 0;
+  int Limit = 100;
+  int *p = 
+  while (i < Limit) { // We do not warn since the var 'i' is escaped but it is
+  // an actual error, since the pointer 'p' is increased.
+*(p++);
+  }
+}
+
+void escape_after() {
+  int i = 0;
+  int j = 0;
+  int Limit = 10;
+
+  while (i < Limit) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: None of the condition variables (i, Limit) are updated in the loop body
+  }
+  int *p = 
+}
+
+int glob;
+void glob_var(int ) {
+  int i = 0, Limit = 100;
+  while (x < Limit) { // Not an error since 'x' can be an alias of glob.
+glob++;
+  }
+}
+
+void glob_var2() {
+  int i = 0, Limit = 100;
+  while (glob < Limit) { // Since 'glob' is declared out of the function we do not warn.
+i++;
+  }
+}
Index: docs/clang-tidy/checks/misc-infinite-loop.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-infinite-loop.rst
@@ -0,0 +1,30 @@
+.. title:: clang-tidy - misc-infinite-loop
+
+misc-infinite-loop
+==
+
+The check finds loops where none of the condition variables are updated in the
+body. This performs a very conservative check in order to avoid false positives
+and work only on integer types at the moment.
+
+Examples:
+
+.. code-block:: c++
+
+  void simple_infinite_loop() {
+int i = 0;
+int j = 0;
+int Limit = 10;
+while (i < Limit) { // Error, since none of the variables are updated.
+  j++;
+}
+  }
+
+  void escape_before() {
+int i = 0;
+int Limit = 100;
+int *p = 
+while (i < Limit) { // Not an error, since p is alias of i.
+  *++p;
+}
+  }
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -119,6 +119,7 @@
misc-definitions-in-headers
misc-forwarding-reference-overload
misc-incorrect-roundings
+   misc-infinite-loop
misc-lambda-function-name
misc-macro-parentheses
misc-macro-repeated-side-effects
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,6 +57,12 @@
 Improvements to clang-tidy
 --
 
+- New `misc-infinite-loop
+  `_ check
+
+  The check finds loops where none of the condition variables are updated in the
+  body.
+
 - The 'misc-move-constructor-init' check was renamed to `performance-move-constructor-init
   

[PATCH] D40937: [clang-tidy] Infinite loop checker

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Does it really belong to Clang-tidy or Clang Analyzer is better place because 
of path-ensitivity?

Please rebase from trunk.

General note: CLang-tidy use //check// in terminology, not //checker//.




Comment at: docs/ReleaseNotes.rst:63
+
+  The checker aims to find loops where none of the
+  condition variables are updated in the body.

Just //Finds// instead of //The checker aims to find//. Please also align on 80 
symbols per line. Same in documentation.


https://reviews.llvm.org/D40937



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40937: [clang-tidy] Infinite loop checker

[Peter Szecsi via Phabricator via cfe-commits]

szepet created this revision.
Herald added subscribers: rnkovacs, baloghadamsoftware, whisperity, mgorny.

The checker aims to find loops where none of the condition variables are 
updated in the body. 
In this version it only works on integer types but the final aim is to make it 
work for objects as well. (via checking non-const method calls, etc)

Note: this kind of check is supported by clang warning as well 
(-Wfor-loop-analysis), however, it only works on for-loops and not investigate 
escape statements (modification via alias generates false positives e.g.  
`escape_before1()` test case).

Any suggestions on the checker are welcome!


https://reviews.llvm.org/D40937

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/InfiniteLoopCheck.cpp
  clang-tidy/misc/InfiniteLoopCheck.h
  clang-tidy/misc/MiscTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-infinite-loop.rst
  test/clang-tidy/misc-infinite-loop.cpp

Index: test/clang-tidy/misc-infinite-loop.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-infinite-loop.cpp
@@ -0,0 +1,104 @@
+// RUN: %check_clang_tidy %s misc-infinite-loop %t
+
+void simple_infinite_loop1() {
+  int i = 0;
+  int j = 0;
+  while (i < 10) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: The condition variable (i) is not updated in the loop body [misc-infinite-loop]
+j++;
+  }
+
+  do {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: The condition variable (i) is not updated in the loop body
+j++;
+  } while (i < 10);
+
+  for (i = 0; i < 10; ++j) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: The condition variable (i) is not updated in the loop body
+  }
+}
+
+void simple_infinite_loop2() {
+  int i = 0;
+  int j = 0;
+  int Limit = 10;
+  while (i < Limit) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: None of the condition variables (i, Limit) are updated in the loop body [misc-infinite-loop]
+j++;
+  }
+
+  do {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: None of the condition variables (i, Limit) are updated in the loop body
+j++;
+  } while (i < Limit);
+
+  for (i = 0; i < Limit; ++j) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: None of the condition variables (i, Limit) are updated in the loop body
+  }
+}
+
+void simple_not_infinite() {
+  int i = 0;
+  int Limit = 100;
+  while (i < Limit) { // Not an error since 'Limit' is updated
+Limit--;
+  }
+  do {
+Limit--;
+  } while (i < Limit);
+
+  for (i = 0; i < Limit;Limit--) {
+  }
+}
+
+void escape_before1() {
+  int i = 0;
+  int Limit = 100;
+  int *p = 
+  while (i < Limit) { // Not an error, since p is alias of i.
+*++p;
+  }
+
+  do {
+*++p;
+  } while (i < Limit);
+
+  for (i = 0; i < Limit; *++p) {
+  }
+}
+
+void escape_before2() {
+  int i = 0;
+  int Limit = 100;
+  int *p = 
+  while (i < Limit) { // We do not warn since the var 'i' is escaped but it is
+  // an actual error, since the pointer 'p' is increased.
+*(p++);
+  }
+}
+
+void escape_after() {
+  int i = 0;
+  int j = 0;
+  int Limit = 10;
+
+  while (i < Limit) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: None of the condition variables (i, Limit) are updated in the loop body
+  }
+  int *p = 
+}
+
+int glob;
+void glob_var(int ) {
+  int i = 0, Limit = 100;
+  while (x < Limit) { // Not an error since 'x' can be an alias of glob.
+glob++;
+  }
+}
+
+void glob_var2() {
+  int i = 0, Limit = 100;
+  while (glob < Limit) { // Since 'glob' is declared out of the function we do not warn.
+i++;
+  }
+}
Index: docs/clang-tidy/checks/misc-infinite-loop.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-infinite-loop.rst
@@ -0,0 +1,31 @@
+.. title:: clang-tidy - misc-infinite-loop
+
+misc-infinite-loop
+==
+
+The checker aims to find loops where none of the condition variables are
+updated in the body. This performs a very conservative check in order to
+avoid false positives and work only on integer types at the moment.
+
+Examples:
+
+.. code-block:: c++
+
+  void simple_infinite_loop() {
+int i = 0;
+int j = 0;
+int Limit = 10;
+while (i < Limit) { // Error, since none of the variables are updated.
+  j++;
+}
+  }
+
+  void escape_before() {
+int i = 0;
+int Limit = 100;
+int *p = 
+while (i < Limit) { // Not an error, since p is alias of i.
+  *++p;
+}
+  }
+  
\ No newline at end of file
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -119,6 +119,7 @@
misc-definitions-in-headers
misc-forwarding-reference-overload
misc-incorrect-roundings
+   misc-infinite-loop
misc-lambda-function-name
misc-macro-parentheses
misc-macro-repeated-side-effects
Index: