[QUALITY] The MISRA topic

[alex31]

coverity is a well known static code checker, it's not free, but the company offers her service to some
big open source project. recently, they decided to open their service to every open source project.

I have submited a project wich use chibios, so the checker pass my code (founding several issues), and chibios.

On the chibios side, there is a lot false positive, but some errors seems reals

I will put some on this post, but, coverity offer for open source project to add automatic build/report at each
commit to follow easily new bug and regressions, perhaps is worth the investment (in work since the service is free).

some errors :

Code: Select all

void rtcSetPeriodicWakeup_v2(RTCDriver *rtcp, const RTCWakeup *wakeupspec){
   
deref_ptr: Directly dereferencing pointer wakeupspec.
242  chDbgCheck((wakeupspec->wakeup != 0x30000),
243              "rtc_lld_set_periodic_wakeup, forbidden combination");
244
   
CID 59709 (#1 of 2): Dereference before null check (REVERSE_INULL)check_after_deref: Null-checking wakeupspec suggests that it may be null, but it has already been dereferenced on all paths leading to the check.


Code: Select all

void mac_lld_stop(MACDriver *macp) {
365
366  if (macp->state != MAC_STOP) {
367#if STM32_MAC_ETH1_CHANGE_PHY_STATE
368    /* PHY in power down mode until the driver will be restarted.*/
369    mii_write(macp, MII_BMCR, mii_read(macp, MII_BMCR) | BMCR_PDOWN);
370#endif
371
372    /* MAC and DMA stopped.*/
373    ETH->MACCR    = 0;
374    ETH->DMAOMR   = 0;
375    ETH->DMAIER   = 0;
   
CID 25515 (#1 of 2): Self assignment (NO_EFFECT)self_assign: Assignment operation (ETH_TypeDef *)0x40028000->DMASR = (ETH_TypeDef *)0x40028000->DMASR has no effect.
376    ETH->DMASR    = ETH->DMASR;


Code: Select all

there is the same self assignment in mac_lld_start


What could be seen, is that with only one real problem on all the codebase (i suppose the self assignment is not a bug but a stm32 trick), the chibios code quality
seen by the coverity metric is very high !

Alexandre

[Giovanni]

Thanks for posting the results, I am thinking to go for MISRA compliance in 3.0 and it looks we are starting from a good base.

Feel free to propose fixes of course.

Giovanni

[alex31]

on the few 3 problems seen, only one is real, and i have submited a ticket on it.

the 2 others are of type self assignment, on an ethernet register, but, as i have read on a thread in this form, this self assignment
clears ISR flags, so it's a false positive.

Alexandre

[colin]

I'm surprised coverity still warns on self-assignment where the variable is declared volatile.

[Giovanni]

It is possible it does not "see" the volatile declaration into the ST header because the weird macros used there.

Next week I will give it a try using CodeSonar for double-check.

About MISRA, I am thinking to buy PCLint, any advice?

Giovanni

[skute]

Just to give another option there is also the open-source clang static analyzer: http://clang-analyzer.llvm.org/

[alex31]

I'm surprised coverity still warns on self-assignment where the variable is declared volatile.

volatile or not, what does that change ?

i could be wrong, but for me, if this address wasn't mapped on a register where assignment triggers side effects, this would
have no effect volatile or not, volatile doesnt inform compiler about side effect of assignment, just that
not to rely on a register cache but to the actual value in memory.

Just to give another option there is also the open-source clang static analyzer: http://clang-analyzer.llvm.org/

i have tried clang scan-build, it's easy to let it scan the posix port, but i have not been able to make it works with arm-none cross compiler,
it should be possible, but does not work out the box.

Alexandre

[colin]

I'm surprised coverity still warns on self-assignment where the variable is declared volatile.

volatile or not, what does that change ?

i could be wrong, but for me, if this address wasn't mapped on a register where assignment triggers side effects, this would
have no effect volatile or not, volatile doesnt inform compiler about side effect of assignment, just that
not to rely on a register cache but to the actual value in memory.

Well, without volatile, in code like this

Code: Select all

int x;  // global
void f(void) {
   x = x;
}


the language allows making assumptions about the value of 'x', so it's desirable to have a warning for that assignment.

But in this code:

Code: Select all

volatile int x;  // global
void f(void) {
   x = x;  // x is volatile
}


the compiler can't assume that the value read from 'x' will be the same twice. It may have side-effects.

[russian]

I am thinking to go for MISRA compliance in 3.0 and it looks we are starting from a good base.

That would be cool, but right now it's about 9000 errors with the 2.6 codebase.
Mostly
11 (req) Identifiers should have at most 31 significant characters.
14 (req) Use "unsigned char" or "signed char" instead of plain "char".
33 (req) The right hand side of "&&" or "||" should not contain side effects.
45 (req) Do not use type casting from any type to / from pointers.
59 (req) An "if" or loop body shall always be enclosed in braces.
109 Do not use overlapping variable storage.
111 Bit fields shall have type "unsigned int" or "signed int".
113 (req) Do not use unnamed struct / union members. (??? not sure about this one)

[Giovanni]

It looks like the checker does not recognize the standard uint8_t, uint32_t types and complain about that, sorry but I would simply waiver this kind of warnings.

I am more interested in the others, could you run the tool on 3.0 and send me the report?

Giovanni