misra.py: R14.2: Verify for loop counter modification (#2409)

* misra.py: R14.2: Verify for counter modification

Add additional check to detect modification of loop counter in loop
body. Related issue: https://trac.cppcheck.net/ticket/9490

Add small fix that treat all assignment operators defined in N1750
6.5.16 as has side affects. This will affects rules 13.1, 13.3, 13.5
and allow to catch some false negatives.

* Add tests for fixed FPs for R13.{1,5,6}

* fix

* use isAssignmentOp from cppcheck data

* remove unused set

* handle case with empty body or syntax error

* add test with outer variable

* Fix FP in nested loops, add tests

* Fix FP on outer variables

* Fixup false positives for not loop counters

Author: jubnzv

addons/misra.py

@@ -256,6 +256,38 @@ def getForLoopExpressions(forToken):
             lpar.astOperand2.astOperand2.astOperand2]
 
 
+def getForLoopCounterVariables(forToken):
+    """ Return a set of Variable objects defined in ``for`` statement and
+    satisfy requirements to loop counter term from section 8.14 of MISRA
+    document.
+    """
+    if not forToken or forToken.str != 'for':
+        return None
+    tn = forToken.next
+    if not tn or tn.str != '(':
+        return None
+    vars_defined = set()
+    vars_exit = set()
+    vars_modified = set()
+    cur_clause = 1
+    te = tn.link
+    while tn and tn != te:
+        if tn.variable:
+            if cur_clause == 1 and tn.variable.nameToken == tn:
+                vars_defined.add(tn.variable)
+            elif cur_clause == 2:
+                vars_exit.add(tn.variable)
+            elif cur_clause == 3:
+                if tn.next and hasSideEffectsRecursive(tn.next):
+                    vars_modified.add(tn.variable)
+                elif tn.previous and tn.previous.str in ('++', '--'):
+                    vars_modified.add(tn.variable)
+        if tn.str == ';':
+            cur_clause += 1
+        tn = tn.next
+    return vars_defined & vars_exit & vars_modified
+
+
 def findCounterTokens(cond):
     if not cond:
         return []
@@ -304,7 +336,7 @@ def isFloatCounterInWhileLoop(whileToken):
 
 
 def hasSideEffectsRecursive(expr):
-    if not expr:
+    if not expr or expr.str == ';':
         return False
     if expr.str == '=' and expr.astOperand1 and expr.astOperand1.str == '[':
         prev = expr.astOperand1.previous
@@ -316,7 +348,7 @@ def hasSideEffectsRecursive(expr):
             e = e.astOperand1
         if e and e.str == '.':
             return False
-    if expr.str in ('++', '--', '='):
+    if expr.isAssignmentOp or expr.str in {'++', '--'}:
         return True
     # Todo: Check function calls
     return hasSideEffectsRecursive(expr.astOperand1) or hasSideEffectsRecursive(expr.astOperand2)
@@ -1425,6 +1457,28 @@ def misra_14_2(self, data):
             elif hasSideEffectsRecursive(expressions[1]):
                 self.reportError(token, 14, 2)
 
+            # Inspect modification of loop counter in loop body
+            counter_vars = getForLoopCounterVariables(token)
+            outer_scope = token.scope
+            body_scope = None
+            tn = token.next
+            while tn and tn.next != outer_scope.bodyEnd:
+                if tn.scope and tn.scope.nestedIn == outer_scope:
+                    body_scope = tn.scope
+                    break
+                tn = tn.next
+            if not body_scope:
+                continue
+            tn = body_scope.bodyStart
+            while tn and tn != body_scope.bodyEnd:
+                if tn.variable and tn.variable in counter_vars:
+                    if tn.next:
+                        # TODO: Check modifications in function calls
+                        if hasSideEffectsRecursive(tn.next):
+                            self.reportError(tn, 14, 2)
+                tn = tn.next
+
+
     def misra_14_4(self, data):
         for token in data.tokenlist:
             if token.str != '(':

addons/test/misra/misra-test.c

@@ -40,7 +40,7 @@ void misra_2_7_used_params (int *param1, int param2, int param3)
 void misra_3_2(int enable)
 {
     // This won't generate a violation because of subsequent blank line \
-	
+
     int y = 0;
     int x = 0;  // 3.2 non-compliant comment ends with backslash \
     if (enable != 0)
@@ -67,16 +67,16 @@ static int misra_5_2_var_hides_var______31x;
 static int misra_5_2_var_hides_var______31y;//5.2
 static int misra_5_2_function_hides_var_31x;
 void misra_5_2_function_hides_var_31y(void) {}//5.2
-void foo(void) 
+void foo(void)
 {
   int i;
   switch(misra_5_2_func1()) //16.4 16.6
   {
-    case 1: 
+    case 1:
     {
       do
       {
-        for(i = 0; i < 10; i++) 
+        for(i = 0; i < 10; i++)
         {
           if(misra_5_2_func3()) //14.4
           {
@@ -85,7 +85,7 @@ void foo(void)
           }
         }
       } while(misra_5_2_func2()); //14.4
-    } 
+    }
   }
 }
 
@@ -161,13 +161,13 @@ void misra_5_5_functionhides_macro31y(int misra_5_5_param_hides_macro__31y){(voi
 struct misra_5_5_tag_hides_macro____31y { //5.5
 int x;
 };
-void misra_5_5_func1() 
+void misra_5_5_func1()
 {
   switch(misra_5_5_func2()) //16.4 16.6
   {
     case 1:
     {
-      do 
+      do
       {
         if(misra_5_5_func3()) //14.4
         {
@@ -184,11 +184,11 @@ void misra_7_1() {
 }
 
 void misra_7_3() {
-  long misra_7_3_a = 0l; //7.3       
-  long misra_7_3_b = 0lU; //7.3     
-  long long misra_7_3_c = 0Ull; //7.3     
-  long long misra_7_3_d = 0ll; //7.3     
-  long double misra_7_3_e = 7.3l; //7.3  
+  long misra_7_3_a = 0l; //7.3
+  long misra_7_3_b = 0lU; //7.3
+  long long misra_7_3_c = 0Ull; //7.3
+  long long misra_7_3_d = 0ll; //7.3
+  long double misra_7_3_e = 7.3l; //7.3
   }
 
 
@@ -241,7 +241,7 @@ void misra_10_4(u32 x, s32 y) {
   enum misra_10_4_enumb { misra_10_4_B1, misra_10_4_B2, misra_10_4_B3 };
   if ( misra_10_4_B1 > misra_10_4_A1 ) //10.4
    {
-      ; 
+      ;
    }
   z = x + y; //10.4
   z = (a == misra_10_4_A3) ? x : y; //10.4
@@ -351,6 +351,12 @@ void misra_12_4() {
 
 struct misra_13_1_t { int a; int b; };
 void misra_13_1(int *p) {
+  volatile int v;
+  int a1[3] = {0, (*p)++, 2}; // 13.1
+  int a2[3] = {0, ((*p) += 1), 2}; // 13.1
+  int a3[3] = {0, ((*p) = 19), 2}; // 13.1
+  int b[2] = {v,1};
+  struct misra_13_1_t c = { .a=4, .b=5 }; // no fp
   volatile int vv;
   int v = 42;
 
@@ -403,10 +409,15 @@ void misra_13_4() {
 
 void misra_13_5() {
   if (x && (y++ < 123)){} // 13.5
+  if (x || ((y += 19) > 33)){} // 13.5
+  if (x || ((y = 25) > 33)){} // 13.5 13.4
+  if (x || ((--y) > 33)){} // 13.5
   else {}
 }
 
 void misra_13_6() {
+  int a = sizeof(x|=42); // 13.6
+  a = sizeof(--x); // 13.6 13.3
   return sizeof(x++); // 13.6
 }
 
@@ -428,24 +439,97 @@ void misra_14_1() {
 void misra_14_2_init_value(int32_t *var) {
     *var = 0;
 }
-void misra_14_2(bool b) {
-  for (dostuff();a<10;a++) {} // 14.2
+void misra_14_2_fn1(bool b) {
   for (;i++<10;) {} // 14.2
   for (;i<10;dostuff()) {} // TODO
   int32_t g = 0;
+  int g_arr[42];
+  g += 2; // no-warning
   for (int32_t i2 = 0; i2 < 8; ++i2) {
-      i2 += 2; // FIXME False negative for "14.2". Trac #9490
-      g += 2; // no-warning
+    i2 += 2; // 14.2
+    i2 |= 2; // 14.2
+    g += 2;
+    i2 ^= 2; // 14.2
+    if (i2 == 2) {
+      g += g_arr[i2];
+    }
+    misra_14_2_init_value(&i2); // TODO: Fix false negative in function call
   }
+
   for (misra_14_2_init_value(&i); i < 10; ++i) {} // no-warning FIXME: False positive for 14.2 Trac #9491
+
   bool abort = false;
   for (i = 0; (i < 10) && !abort; ++i) { // no-warning
       if (b) {
         abort = true;
       }
   }
   for (;;) {} // no-warning
-  // TODO check more variants
+
+  int x = 10;
+  for (int i = x; i < 42; i++) {
+      x++; // no warning
+  }
+  for (int i = (x - 3); i < 42; i++) {
+      x ^= 3; // no warning
+  }
+
+  for (int i = 0, j = 19; i < 42; i++) { // 12.3 14.2
+      i += 12; // 14.2
+      j /= 3; // TODO: 14.2
+  }
+
+  for (int i = 0; i < 19; i++) {
+      for (int j = 0; j < 42; j++) {
+          i--; // 14.2
+          for (int k = j; k > 5; k--) {
+              i++; // 14.2
+              for (int h = 35; h > 5; k++) // 14.2
+              {}
+          }
+      }
+  }
+}
+static void misra_14_2_fn2()
+{
+    int y = 0;
+
+    // Handle cases when i is not treated as loop counter according MISRA
+    // definition.
+    for (int i = 0, j = 19; y < 10, --j > 10; y++, j--) { // 14.2 12.3
+        i++; // no warning
+    }
+    for (int i = 0, j = 19; y < 10, --j > 10; y++, j--) { // 14.2 12.3
+        i++; // no warning
+    }
+    for (int i = 0; y < 10; y++) { // TODO: 14.2
+        i++; // no warning
+    }
+    for (int i = 0; i < 10; y++) { // TODO: 14.2
+        i++; // no warning
+    }
+    for (int i = 0; y < 10; i++) { // TODO: 14.2
+        i++; // no warning
+    }
+    for (int i = 0; i < 10; (y+=i)) {
+        i++; // no warning
+    }
+
+    // i is a loop counter according MISRA definition
+    for (int i = 0; i < 10; i++) {
+        i++; // 14.2
+        if (++i > 5) { // 14.2
+            break;
+        }
+    }
+    for (int i = 0; i < 10; (i+=42)) {
+        i++; // 14.2
+    }
+    for (int i = 0; i < 10; (i|=y)) {
+        i++; // 14.2
+    }
+
+    return 0;
 }
 
 struct {