5

I have a very simple function which created a time delay:

void delay(int time_in_ms)
{
   int t = get_time() + time_in_ms;
   while (get_time() < t)
   {
   };
}

delay(750);

I'm getting the warning that the control variable t is not modified inside the while loop. I don't need to modify the variable, I just need to be inside the while loop as long as the condition is met. How to bypass this in a "nice" way?

The warning is created by MISRA static analysis tool. The full message:

"The control variable in this loop construct is never modified"
MISRA C:2012 Rules applicable to message 2467:

Jongware
  • 22,200
  • 8
  • 54
  • 100
JohnDoe
  • 825
  • 1
  • 13
  • 31
  • 2
    Which compiler are you using? – alk Jan 03 '18 at 15:15
  • 2
    How about using sleep()? Is it a requirement to create this function? – Segmentation Jan 03 '18 at 15:16
  • 1
    Is the compiler generating this warning, or a source code analysis tool? – dbush Jan 03 '18 at 15:16
  • @alk A compiler that doesn't need a `;` after `int t = get_time() + time_in_ms` – wildplasser Jan 03 '18 at 15:17
  • 2
    is the warning also generated when using `while (...);`, i.e. without braces/body? – Karsten Koop Jan 03 '18 at 15:17
  • @wildplasser: I forgot do add ; – JohnDoe Jan 03 '18 at 15:22
  • @dbush it's a source code analysis tool – JohnDoe Jan 03 '18 at 15:23
  • 1
    How `get_time()` is defined? Does it return a `const`? – OlivierM Jan 03 '18 at 15:23
  • 1
    Maybe get_time() returns a larger type than int? We'll never know... – wildplasser Jan 03 '18 at 15:25
  • @OlivierM No it returns the value of an free running counter register – JohnDoe Jan 03 '18 at 15:25
  • @JohnDoe could you add `get_time()` prototype to the original post? – OlivierM Jan 03 '18 at 15:26
  • What about `t = t;` inside the loop? – CIsForCookies Jan 03 '18 at 15:26
  • 2
    [Edit] your post to tag which static analyser you are using and quote the full error, preferably including any ID the analyser assigns. And show all the code needed to understand the fragment you posted, including the declaration and definition of any symbols you use in it. There is not enough information here to answer. – underscore_d Jan 03 '18 at 15:27
  • 6
    Busy-waiting is kind of a hack (CPU-and-energy-inefficient) way to implement a delay; perhaps your system's APIs have a better method (usleep() or similar) that you could use instead? – Jeremy Friesner Jan 03 '18 at 15:36
  • Technically the tool is in error and you should forward this to their help desk. – Paul Ogilvie Jan 03 '18 at 16:16
  • Why are you trying to roll your own sleep function? Most OS's come with the sleep function built in and it is implemented in a much nicer way (scheduled) than endless-looping. You should consider using that if available. – smac89 Jan 03 '18 at 18:31
  • Your tool fails to spot that `get_time` contains any side effects. What is `get_time()`? Does it contain any side effects? If this is some home-brewed code and you failed to include a `volatile` register access somewhere, then your code has a bug and the tool is correct to warn you. If there are such side effects present, then it is a tool bug. – Lundin Jan 08 '18 at 09:48

3 Answers3

2

The static analysis tool seems to be expecting a common case like:

int i = 0;
while(i < get_size(var))
{
    do_something(var[i]);
    i++;
}

However, in your case, the loop control variable is the result of getTime() while t is a limit.

You can use a real loop control variable. The compiler will probably optimize it out.

void delay(int time_in_ms)
{
   int t = get_time() + time_in_ms;
   int current_time;
   do
   {
      current_time = getTime();
   } while(current_time < t);
}

Or you can try to discover what makes your static analysis tool think that t is the loop control variable. A const int t declaration could help.

Melebius
  • 6,183
  • 4
  • 39
  • 52
  • Right, the lack of `const`-correctness could well be the culprit here (though I'm not familiar with MISRA specifically). I guess I'd be fine if I ever ran my code through something like this, as I put `const` everywhere possible. :) – underscore_d Jan 03 '18 at 15:47
  • @Melebius: The second proposal with the do-while loop fix the warning! Just as an review, are we sure that we do not change the logic on that way? – JohnDoe Jan 04 '18 at 10:50
  • 1
    @JohnDoe You can check the generated code using disassembler if the result remains same instruction after instruction. The logic definitely remains unchanged, I just added an auxiliary variable. MISRA requires them in some cases, e.g. for more complicated type casts. – Melebius Jan 04 '18 at 11:00
  • @Melebius Yes you are right. Your solution is also correct! – JohnDoe Jan 04 '18 at 13:51
0

Every Compiler or code review tools or memory leak analyzer tool has its own logic and based on it's logic it gives you warning or error message. Here in this case you compiler/tool is considering t as a control variable based on which while loop should break. As compiler/tool did not find any code inside while loop block to manipulate variable t its raised a warning for you, because it thought that your while loop might be a infinite while loop. Either you can ignore it as you are sure that it will never be a infinite loop or you can modify your code in a different way (see below, but might not fix this warning) to avoid this warning.

void delay(int time_in_ms)
{
   int t = get_time() + time_in_ms;
   while ( t > get_time() );
}

delay(750);

In the above changes I moved t to left hand side so that your tool should not consider t as control variable and should not raise any warning.

Abhijit Pritam Dutta
  • 5,521
  • 2
  • 11
  • 17
  • The first paragraph may explain the reasoning, though by the time you answered it was very clear that the question is about a static analyser, not a compiler. I also don't see how your 2nd suggestion could possibly produce different results in any case: it merely inverts the comparison and drops the empty block, neither of which I can imagine making any difference to optimisation or warnings, especially not the latter. – underscore_d Jan 03 '18 at 21:22
  • @underscore_d, Thanks for the correction. Even this kind of tools to create code reviews and memory leak etc having their own logic. A warning from this tools should not be taken very seriously every time. I am agree that first while loop I have just removed the block but in the second code the tool might consider get_time() as control variable. I am just asking for try or else just ignore the warning which is base less in this case. – Abhijit Pritam Dutta Jan 04 '18 at 09:56
  • @Abhijit You are right, the proposed code also didn't fix the warning. – JohnDoe Jan 04 '18 at 10:43
0

While you may have some knowledge about getTime(), the static analyzer basically assumes that the return value of getTime() might never trigger the loop end condition, thus the loop would potentially be infinite.

So what you need is an infinite loop that's accepted by the analyzer, combined with a break-condition within the loop body. As far as I can tell from a quick search, only for(;;) is accepted as an infinite loop by MISRA.

for ( ; ; )
{
    if (t <= get_time()) break;
}

This should not trigger a warning.

grek40
  • 13,113
  • 1
  • 24
  • 50
  • Oh, for goodness sake, no! There is nothing wrong with the code ass shown above - although as LundIn points out, maybe in the implementation of get_time(). Trying to be clever to quieten your SA tool is the wrong approach! – Andrew Jan 11 '18 at 12:45
  • @Andrew what are you trying to say? I think it's clear that the question code is correct without any change (just the static analyzer doesn't understand this), so the whole question is, how to make the code pass some analyzer tests without warning. – grek40 Jan 11 '18 at 13:05
  • The OP's example code doesn't show if get_time() has a side effect (ie LundIn's point) - the SA tool seems to think that both t and get_time() are invariant. Changing the do/while to an if will not change that. – Andrew Jan 11 '18 at 15:55