CODEWITHSUNDEEP
cmacrosmisra
Rog Matthews
Rog Matthews

Reputation: 3247

Misra C error in Macro definition

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.

Upvotes: 3

Views: 5981

Answers (2)

Lundin
Lundin

Reputation: 213892

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 */
    }
  }
}

Upvotes: 6

Samuel Edwin Ward
Samuel Edwin Ward

Reputation: 6675

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 &&?

Upvotes: 3

Related Questions