4

I need to check if a parameters fits in a well known list. To do so I wrote this function

#define INV       (0x1UL << 15U)
#define NON_INV   0X00000000U
#define RISING    0X00000000U
#define FALLING   (0x1UL << 1U)  
#define BOTH_EDGE ((0x1UL << 1U)|(0x1UL << 3U))  

bool IsTimClockPolarity(uint32_t pol)
{
    bool result = false;
    if ((pol == RISING) || (pol == FALLING) || (pol == INV) || (pol == NON_INV) || (pol == BOTH_EDGE))
    {
        result = true;
    }
    return result;
}

For the sake of clarity, these macros are target implemented for stm32h7 device, I didn't choose them. In QAC analyzer I have the following MISRA violation:

The result of this logical operation is always 'false'

Do you know how to solve this?

Vincenzo Cristiano
  • 77
  • 6
  • 6
    For completeness: what are `RISING`, `FALLING` etc.? [Edit] your question. – Jabberwocky Jul 26 '22 at 10:12
  • 1
    You'll need to show us the definition of the five constants: `RISING`, and the others. – Adrian Mole Jul 26 '22 at 10:12
  • 2
    You might want to separate all `pol == xxx` into their own lines, perhaps it is just one of them. We need to see the definitions. -- OT: Why don't you just `return (pol == RISING) || (pol == FALLING) ...;`? Six lines where one suffices looks complicated. – the busybee Jul 26 '22 at 10:16
  • 1
    Are `RISING`, `FALLING` etc values outside the range of `uint32_t`? – Caleth Jul 26 '22 at 10:20
  • @thebusybee is not the same thing? – Vincenzo Cristiano Jul 26 '22 at 10:28
  • Is it intentional that `NON_INV` and `RISING` are defined to the same value? Do you get the warning when you analyze the single file or only when you analyze the whole project? The analyzer might detect that the possible values for `uint32_t pol` are limited to a smaller set of values in your project. Does the warning message contain a column number which may refer to a specific comparison? I suggest to copy&paste the whole warning message. – Bodo Jul 26 '22 at 10:30
  • Due to short circuit optimization, `(pol == NON_INV)` will only be evaluated if `(pol == RISING)` is false. But as both macros are `0` that comparison also must be false then. – Gerhardh Jul 26 '22 at 10:34
  • 2
    Why isn't it `#define RISING (0x1UL << 3U)` ? Anyway, these look like bit masks that you should isolate before testing. – Weather Vane Jul 26 '22 at 10:36
  • 2
    The macros definitions look like bit masks but you don't use them as bit masks. That looks a bit fishy. – Gerhardh Jul 26 '22 at 10:37
  • All other things aside, why the `result` temporary? You could just `return ( ( pol == ...`? – DevSolar Jul 26 '22 at 10:37
  • @SupportUkraine the message is not about the function but about a certain operation in a subexpression. Most likely about `|| (pol == NON_INV)` – Gerhardh Jul 26 '22 at 10:38
  • @SupportUkraine Yes, that is my assumption. That would mean splitting that line into multiple separate instructions might remove the finding. But I don't know that tool QAC as we use Coverity. – Gerhardh Jul 26 '22 at 10:44
  • There is no reason why anything in the code you show should trigger the warning. – Jabberwocky Jul 26 '22 at 10:52
  • And what is the problem? For which value of the argument you expect true and it gives false? – Marian Jul 26 '22 at 11:05
  • @Marian OP doesn't expect anything. He just wonders why his tool shows the warning and so do I. – Jabberwocky Jul 26 '22 at 11:17
  • @Marian the issue is that MISRA considers this dead code as defined in Rule 2.2 which should not be present and the static code analyzer found that defect. – Gerhardh Jul 26 '22 at 11:26
  • By separating in multiple lines the error message will point to the concerned expression. – the busybee Jul 26 '22 at 11:40
  • **Please copy&paste the whole error message to your question.** As far as I know, QAC messages include line number and column number and a message number. You might find additional information in the help for this message number. – Bodo Jul 26 '22 at 13:21
  • IMHO MISRA C is, in a way, broken. Not just the tools, but the whole concept. It considers your code without any context. It sees dead code, it says "dead code bad" (or an expression that always evaluates to "false" or whatever). What it doesn't know is that your code has a dead piece in this configuration, and is fully alive in a different configuration, *and you want to keep the same code for both configurations*. Correct me if I'm wrong. – n. m. could be an AI Jul 26 '22 at 13:29

1 Answers1

4

The warning occurs because the subexpression pol == NON_INV is always false when it is evaluated.

Per C 2018 6.5.14, in an expression E1 || E2, the expression E2 is evaluated only if E1 is false (compares unequal to zero). Therefore, in (pol == RISING) || … || (pol == NON_INV) || …, (pol == NON_INV) is evaluated only if the prior tests are false. In particular, if (pol == NON_INV) is evaluated, we know that (pol == RISING) is false.

But RISING and INV are both defined to be zero, so (pol == NON_INV) and (pol == RISING) are each equivalent to (pol == 0). So, if we are evaluating (pol == NON_INV), we already know (pol == 0) is false, so (pol == NON_INV) must be false, and so the MISRA checker reports the result of (pol == NON_INV) is always false.

Additionally, I suspect the warning is apropos because the code is not doing what is intended. It looks like INV and FALLING are each supposed to be a bit flag indicating some event or condition, in which case the complement of that condition is not that the entire word pol is zero but rather that the specific bit is off. If so, pol == RISING and pol == NON_INV are the wrong tests for that. The entire expression must be corrected to properly test the desired compound condition, but we do not know what that is without further explanation of what INV and BOTH_EDGE mean and what the compound condition is supposed to be.

Regarding the phrasing of the warning, “The result of this logical operation…,” note that || operation in the formal C grammar is expressed as “logical-OR-expression || logical-AND-expression”. Thus, (pol == NON_INV) appears as an instance of a logical-AND-expression, and thus it is reasonable called a “logical expression.” While the technicalities of the C grammar may unfortunately present concepts and terminology that are not immediately evident to novices, if one is writing code to be highly safe, secure, portable, and reliable, as the use of MISRA indicates, then it is imperative not just to learn C code from practice but to study the C standard and learn it in detail.

Eric Postpischil
  • 195,579
  • 13
  • 168
  • 312
  • It looks that MISRA checker is a bit overzealous here. The `==` is not a logical operation. The warning is very confusing. – tstanisl Jul 26 '22 at 11:30
  • So basically it needs to be `(pol & RISING) || (pol & FALLING) ...` - IOW bitwise-ANDing each flag with `pol` and checking if the result is non-zero. – John Bode Jul 26 '22 at 11:31
  • @tstanisl the resulting `|| false` is a logical operation. But in general I agree the warning could be improved, e.g. with exact indication which code is triggering it. – Gerhardh Jul 26 '22 at 11:37
  • @JohnBode: `RISING` is not a flag. It is zero. `pol & RISING` is always zero. – Eric Postpischil Jul 26 '22 at 11:45
  • @Gerhardh, the problem is that the message "logical operation is always 'false'" does not apply to `X || false` which may true or false depending on `X`. The slightly better warning should be "logical operation *with* always 'false' operand". The best would probably be "redundant logical operation with `pol == NON_INV` which is always false if evaluated`. It is shame that this kind of confusing warning appears in software that is supposed to make mission-critical software safer. – tstanisl Jul 26 '22 at 12:03
  • @tstanisl: For an expression `X || Y`, `Y` is a *logical-AND-expression* in the formal C grammar, so it is a logical expression. So, if `Y` is always false, it is correct to say “The result of this logical operation is always 'false'.” – Eric Postpischil Jul 26 '22 at 12:57
  • @EricPostpischil, It's non-sense. It would mean that in `X || 1`, literal `1` is a logical operation. Confusing as hell. – tstanisl Jul 26 '22 at 13:00
  • @tstanisl: Constants are expressions and are evaluated. Take your complaint to the C standard committee, with an alternate proposal for the formal grammar and specification of expression evaluation that avoids this. Between a choice of a formal specification with some linguistic oddities and an informal description tweaked to make the language sound nice, I choose the formal specification. – Eric Postpischil Jul 26 '22 at 13:05
  • @EricPostpischil, I don't complain about the formal definition of C grammar. I complain about the non-sense that the MISRA checker has produced. – tstanisl Jul 26 '22 at 13:10