Misra C error in Macro definition

Question

This piece of code reports three misra c errors:

1. Inappropriate macro expansion
2. Function-like macro definition
3. Macro parameter with no parentheses

The original code is:

#define Wait(a, b)                         \
if (READ(b+0x1U))                          \
{                                          \
    while ((a & Write(b)))                 \
    {                                      \
        /* Do nothing - Busy wait */       \
    }                                      \
}

Here READ(b) is a macro and Write(b) is a function with no Misra C error.

I have trying changing it to remove errors

#define Wait(a, b)                                                 \
if ((uint32_t)0U != READ((b)+0x1U))                                \
{                                                                  \
    while ((uint32_t)0U != ((uint32_t)(a) & Write((uint32_t)(b)))) \
    {                                                              \
        /* Do nothing - Busy wait */                               \
    }                                                              \
}

But i am still getting the first two errors. What needs to be done to remove these Misra C errors.

Answer 1

There's a list of things macros are allowed to expand to, and an if block isn't one of them. I believe this is because it can cause confusion about the attachment of else clauses. More about that here. You can use this construct:

#define MACRO(X)    \
do {                \
    body here       \
} while (0)

You should use a function instead of a function-like macro whenever you can. Without knowing what READ expands to I can't say if that's possible in this case. That would be the only way to get rid of the warning about that.

The third one you already figured out; you have to put parentheses around a and b in the body. The idea here is that if you have code like x*2 in the macro and someone passes 3+1 as x, without the parenthesis you'd get 3+1*2, which is 5, instead of (3+1)*2, which is the 8 which was almost certainly intended.

The only other thing I would have to say about your code is are you sure you want & there and not &&?

Answer 2

1.Inappropriate macro expansion

This is because you haven't encapsulated your macro properly. To fix this, you must change the code to:

#define Wait(a, b)                         \
                                           \
do {                                       \
  if (READ(b+0x1U))                        \
  {                                        \
    while ((a & Write(b)))                 \
    {                                      \
        /* Do nothing - Busy wait */       \
    }                                      \
  }                                        \
} while (0);

(But of course, this is pointless exercise if the rest of your code follows MISRA-C and always use {} after every if, for or while statement.)

---

2.Function-like macro definition

You are using a function-like macro. This isn't allowed by MISRA-C. Rewrite the macro as a function.

However, rule 19.7 is advisory, so you could in theory ignore it without raising a deviation. But there is no reason to do so in this case. There exists no reason why this need to be a macro and not a function.

---

3.Macro parameter with no parentheses

As you guessed, this has to do with every macro parameter being a potential sub-expression. Suppose someone calls your macro as Wait(x+y, z). Your code will then crash and burn upon encountering the while loop, because the macro will expand into while(x+y & Write(b)), which is the same thing as while(x + (y & Write(b)) ).

To solve this, surround every instance of a and b with parenthesis, as in your second example.

---

This piece of code reports three misra c errors:

You should report a bug to Klockwork, their tool is not working correctly. It should also have detected the following:

• if (READ(b+0x1U)) violates rules 13.2. MISRA compliant code would be

  if (READ(b+0x1U) != 0u)
  

• while ((a & Write(b))) violates rule 13.2. MISRA compliant code would be

  while ( (a & Write(b)) != 0u )
  

---

Non-MISRA related concerns:

• (uint32_t)0U should preferably be written as 0UL or 0ul, which are more readable forms.

• Frankly, this code was bad to begin with. Trying to make it MISRA-compliant as it stands, will turn it into a completely unreadable mess. Rewrite it from scratch instead:

  void Wait (uint32_t a, uint32 b)
  {
    if( READ(b + 0x1u) != 0u )           /* comment here, explaining the code */
    {
      while ( (a & Write(b)) != 0u )     /* comment here, explaining the code */
      {
        ;                                /* Do nothing - busy wait */
      }
    }
  }