3

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.

Rog Matthews
  • 3,147
  • 16
  • 37
  • 56

2 Answers2

6

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 */
    }
  }
}
Lundin
  • 195,001
  • 40
  • 254
  • 396
3

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

Samuel Edwin Ward
  • 6,526
  • 3
  • 34
  • 62
  • Yes, & only is required. Samuel I was able to solve first by using braces {} before if and at the end i.e. #define Wait(a,b) \ { if... }. Is this correct way to solve it? – Rog Matthews Dec 09 '13 at 19:35
  • 1
    @RogMatthews, the problem with doing it that way is if you call it like `if (x) Wait(a, b); else foo();` you will get a syntax error because it expands to `if (x) { ... }; else ...` and that semicolon can't be there. Using `do { ... } while (0)` resolves this because a semicolon is required after the while portion. – Samuel Edwin Ward Dec 09 '13 at 19:41
  • 2
    @SamuelEdwinWard If you write MISRA compatible code, that's a non-issue, since MISRA would have enforced you to write `if (x) { Wait(a, b); }` when calling the macro. Only people who insist on skipping braces after statements get these kind of obscure bugs. – Lundin Dec 10 '13 at 12:43
  • @Lundin Certainly, but I think it's worth the small effort to write it so that it will work correctly even if the person reusing your code does something MISRA doesn't approve of. – Samuel Edwin Ward Dec 11 '13 at 14:17