[analyzer] LoopUnrolling: update the matched assignment operators

[szepet]

Summary

Extended the matched assignment operators when checking for bound changes in a body of the loop. This updated list covers all the (current) possible assignments.

[danielmarjamaki]

LGTM.. however I would like approval from somebody else also.

[NoQ]

Neat! Tests?

[xazax.hun]

lib/StaticAnalyzer/Core/LoopUnrolling.cpp

99–100

Maybe instead of enumerating all the assignment operators here it would be better to have a matcher that checks isAssignmentOp for CXXOperatorCalls/BinaryOperators? This would be less error prone and more future proof.

[szepet]

• Updated to use a custom AST matcher for assignment operator check. (More structured and efficient.)
• Tests added as well.

[NoQ]

Yay.

Thanks Gabor, i didn't think about it but it looks very nice to have such matcher.

I think the matcher shouldn't be checker-local, but shared into ASTMatchers.h all other matchers - not only this matcher is universally useful, but also i have weird vague memories that matchers may collide by name if defined in different translation units differently (if this is indeed this way then having the same matcher in multiple places is very bad).

[xazax.hun]

I agree it might be useful to expose this matcher to everybody.

[szepet]

isAssignmentOp matcher moved to ASTMatchers.h
(Manuel added to reviewers as the code owner of ASTMatchers)

[lebedev.ri]

Not sure how clang/docs/LibASTMatchersReference.html is supposed to be updated.

include/clang/ASTMatchers/ASTMatchers.h

3931 ↗

(On Diff #123176)

Maybe

-/// \brief Matches all kind of assignment operators.
+/// \brief Matches all kinds of assignment operators.

3938 ↗

(On Diff #123176)

Needs unittests, probably in clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp ?

[szepet]

Testfiles added and HTML updated.

[xazax.hun]

A nit, otherwise LG!

lib/StaticAnalyzer/Core/PathDiagnostic.cpp

693 ↗

(On Diff #123187)

Is this change related? Maybe you forgot to add a dependency to this revision?

[szepet]

Added Alexander and Aaron as reviewers for the matcher parts.

[aaron.ballman]

Aside from a request for another test, the matcher parts LGTM.

unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

1992 ↗

(On Diff #123187)

Can you also add a test like:

struct S { S& operator=(const S&); };
void x() { S s1, s2; s1 = s2; }

[alexfh]

Do you want to submit the matcher part separately?

[szepet]

In D38921#1037180, @alexfh wrote:

Do you want to submit the matcher part separately?

Yepp, but added as a different patch since implemented it for cxxOperatorCall as well. (I guess Aaron's test request implied it and I find it useful as well.)
See: https://reviews.llvm.org/D44893

[szepet]

Added the assignment operator matcher patch as a dependency and rebased on top of that.

lib/StaticAnalyzer/Core/LoopUnrolling.cpp

return binaryOperator(anyOf(hasOperatorName("<"), hasOperatorName(">"),
       .bind("conditionOperator");
 }
 
 static internal::Matcher<Stmt>
 changeIntBoundNode(internal::Matcher<Decl> VarNodeMatcher) {
   return anyOf(
       unaryOperator(anyOf(hasOperatorName("--"), hasOperatorName("++")),
                     hasUnaryOperand(ignoringParenImpCasts(
                         declRefExpr(to(varDecl(VarNodeMatcher)))))),
-      binaryOperator(anyOf(hasOperatorName("="), hasOperatorName("+="),
-                           hasOperatorName("/="), hasOperatorName("*="),
-                           hasOperatorName("-=")),
+      binaryOperator(isAssignmentOperator(),
                      hasLHS(ignoringParenImpCasts(
                          declRefExpr(to(varDecl(VarNodeMatcher)))))));
 }
 
 static internal::Matcher<Stmt>
 callByRef(internal::Matcher<Decl> VarNodeMatcher) {
   return callExpr(forEachArgumentWithParam(
       declRefExpr(to(varDecl(VarNodeMatcher))),

test/Analysis/loop-unrolling.cpp

for (i = 0; i < 9; i++) {
     clang_analyzer_numTimesReached(); // expected-warning {{4}}
     a[i] = 42;
     int &j{i};
   }
   int b = 22 / (k - 42); // no-warning
   return 0;
 }
 
+int no_unroll_assignment() {
+  for (int i = 0; i < 9; i++) {
+    i = i + 1;
+    clang_analyzer_numTimesReached(); // expected-warning {{4}}
+  }
+  return 0;
+}
+
+int no_unroll_assignment2() {
+  for (int i = 0; i < 9; i++) {
+    i *= 2;
+    clang_analyzer_numTimesReached(); // expected-warning {{4}}
+  }
+  return 0;
+}
+
+int no_unroll_assignment3() {
+  for (int i = 128; i > 0; i--) {
+    i /= 2;
+    clang_analyzer_numTimesReached(); // expected-warning {{4}}
+  }
+  return 0;
+}
+
+int no_unroll_assignment4() {
+  for (int i = 0; i < 9; i++) {
+    i -= 2;
+    clang_analyzer_numTimesReached(); // expected-warning {{4}}
+  }
+  return 0;
+}
+
+int no_unroll_assignment5() {
+  for (int i = 0; i < 9; i++) {
+    i += 1;
+    clang_analyzer_numTimesReached(); // expected-warning {{4}}
+  }
+  return 0;
+}
+
+int no_unroll_assignment6() {
+  for (int i = 128; i > 0; i--) {
+    i >>= 1;
+    clang_analyzer_numTimesReached(); // expected-warning {{4}}
+  }
+  return 0;
+}
+
+int no_unroll_assignment7() {
+  for (int i = 0; i < 512; i++) {
+    i <<= 1;
+    clang_analyzer_numTimesReached(); // expected-warning {{4}}
+  }
+  return 0;
+}
+
+int no_unroll_assignment8() {
+  for (int i = 0; i < 9; i++) {
+    i %= 8;
+    clang_analyzer_numTimesReached(); // expected-warning {{4}}
+  }
+  return 0;
+}
+
+int no_unroll_assignment9() {
+  for (int i = 0; i < 9; i++) {
+    i &= 31;
+    clang_analyzer_numTimesReached(); // expected-warning {{4}}
+  }
+  return 0;
+}
+
+int no_unroll_assignment10() {
+  for (int i = 0; i < 9; i++) {
+    i |= 2;
+    clang_analyzer_numTimesReached(); // expected-warning {{4}}
+  }
+  return 0;
+}
+
+int no_unroll_assignment11() {
+  for (int i = 0; i < 9; i++) {
+    i ^= 2;
+    clang_analyzer_numTimesReached(); // expected-warning {{4}}
+  }
+  return 0;
+}
+
 int make_new_branches_loop_cached() {
   for (int i = 0; i < 8; i++) {
     clang_analyzer_numTimesReached(); // expected-warning {{4}}
-    if(getNum()){
-        (void) i; // Since this Stmt does not change the State the analyzer
-                  // won't make a new execution path but reuse the earlier nodes.
-      }
+    if (getNum()) {
+      (void)i; // Since this Stmt does not change the State the analyzer
   }
   clang_analyzer_warnIfReached(); // no-warning
   return 0;
 }
 
 int make_new_branches_loop_uncached() {
   int l = 2;
   for (int i = 0; i < 8; i++) {
     clang_analyzer_numTimesReached(); // expected-warning {{10}}
-    if(getNum()){
+    if (getNum()) {
       ++l;
     }
   }
   clang_analyzer_warnIfReached(); // no-warning
   return 0;
 }
 
 int make_new_branches_loop_uncached2() {
   int l = 2;
   for (int i = 0; i < 8; i++) {
     clang_analyzer_numTimesReached(); // expected-warning {{10}}
-    if(getNum()){
+    if (getNum()) {
       ++l;
     }
     (void)&i; // This ensures that the loop won't be unrolled.
   }
   clang_analyzer_warnIfReached(); // no-warning
   return 0;
 }
 
int nested_outer_unrolled() {
   int a[9];
   int k = 42;
   int j = 0;
   for (int i = 0; i < 9; i++) {
     clang_analyzer_numTimesReached(); // expected-warning {{1}}
     for (j = 0; j < 9; ++j) {
       clang_analyzer_numTimesReached(); // expected-warning {{4}}
       a[j] = 22;
-      (void) &j; // ensures that the inner loop won't be unrolled
+      (void)&j; // ensures that the inner loop won't be unrolled
     }
     a[i] = 42;
   }
   int b = 22 / (k - 42); // no-warning
   return 0;
 }
 
 int nested_inner_unrolled() {
int nested_inlined_no_unroll1() {
   int a = 22 / k; // no-warning
   return 0;
 }
 
 int recursion_unroll1(bool b) {
   int k = 2;
   for (int i = 0; i < 5; i++) {
     clang_analyzer_numTimesReached(); // expected-warning {{13}}
-    if(i == 0 && b) // Splits the state in the first iteration but the recursion
+    if (i == 0 && b)                  // Splits the state in the first iteration but the recursion
                                       // call will be unrolled anyway since the condition is known there.
       recursion_unroll1(false);
     clang_analyzer_numTimesReached(); // expected-warning {{14}}
   }
   int a = 22 / k; // no-warning
   return 0;
 }
 
 int recursion_unroll2(bool b) {
   int k = 0;
   for (int i = 0; i < 5; i++) {
     clang_analyzer_numTimesReached(); // expected-warning {{9}}
-    if(i == 0 && b)
+    if (i == 0 && b)
       recursion_unroll2(false);
     clang_analyzer_numTimesReached(); // expected-warning {{9}}
   }
   int a = 22 / k; // expected-warning {{Division by zero}}
   return 0;
 }
 
 int recursion_unroll3(bool b) {
int recursion_unroll3(bool b) {
   int a = 22 / k;
   return 0;
 }
 
 int recursion_unroll4(bool b) {
   int k = 2;
   for (int i = 0; i < 5; i++) {
     clang_analyzer_numTimesReached(); // expected-warning {{13}}
-    if(i == 0 && b) {
+    if (i == 0 && b) {
       recursion_unroll4(false);
       continue;
     }
     clang_analyzer_numTimesReached(); // expected-warning {{13}}
   }
   int a = 22 / k;
   return 0;
 }