MISRA-C 2012 Compliance in kernel (kernel/*) and base OS code

[nashif]

Adhere to MISRA-C 2012 where it makes sense and provide deviations and documentation of rules we can't adhere to because of the nature of Zephyr.

[pfalcon]

@ceolin, So, what does MISRA says on addition of unsigned and signed integers (suppose of the same width)? I would expect to condemn that loudly, given that it's know to call for just crazy things. I'm looking for ally on reworking entire Zephyr timehandling API. In almost all places, we have signed values for elapsed time and delays, e.g.:

void _nano_sys_clock_tick_announce(s32_t ticks)

That doesn't make sense - the time goes only forward, so can't be negative. One possibility is that s32_t was used to limit the range of delays to INT_MAX (vs UINT_MAX), as crude way of dealing with overdue timeouts (detecting them). But non-negativity of e.g. delays aren't enforced, so nobody knows/can't know that max value of delays is INT_MAX.

Using signed stuff would invoke C undefined behavior, and could be killed on that regard, but cunningly, Zephyr doesn't use signed time everywhere, just in many cases.

This leads for example to u64_t += s32_t. I now hope that can be killed on MISRA C grounds.

(That all assumes that it can't be killed on the grounds of common sense, because if it could be, it wouldn't happen in the first place).

[ceolin]

@pfalcon Sorry for delaying this answer, I'm now jumping on it.
You're right, MISRA C has the "essential type" concept. Signed int (s32_t, s16_t ...) and unsigned int (u32_t, ...) have different "essential type" so it is not ok mix them (without explicit cast - case of signed to unsigned)

So, the u64_t += s32_t example, is NOT valid.

[ceolin]

@pfalcon one more thing, in general is safe presume that any undefined behaviour in C is doomed by MISRA-C

[pfalcon]

@ceolin :

So, the u64_t += s32_t example, is NOT valid.

Cool, thanks, so hope we'll get to resolving #9663 then.

[ceolin]

Useful link: https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite

It contains examples of the problems addressed by MISRA-C

[mped-oticon]

@pfalcon one more thing, in general is safe presume that any undefined behaviour in C is doomed by MISRA-C

btw, we use gcc -fno-strict-overflow in cmake. That flag defines overflow behaviour to that of normal hardware two's complement. I haven't dared to turn it off...

[pabigot]

I'm entirely in support of conforming to standards where doing so improves code quality and maintainability.

It appears MISRA-C:2012 compliance requires limiting code to features available in a 20-year-old version of the C language, excluding the option of clean and efficient solutions that require only a 7-year-old version of C.

Is MISRA-C:2012 conformance more valuable to Zephyr than C11 conformance?

(That question is not rhetorical: the answer may be "yes" or "no", but in either case it should be justified. I'm still working to understand the criteria used to determine how Zephyr should evolve.)

[ceolin]

@pabigot C11, C99, ... are code standards while MISRA is a code guideline. MISRA defines a set of rules to make the code safer and less bug prone. Though, MISRA is not orthogonal to C standards, the latest version is based on C99 but can be used with C90. I know that these are relatively old compared with C1,1unfortunatelly.

In other words, MISRA-C considers that C language (defined in its standards) allows making code with a high probability of problems, e.g. MISRA-C does not allow conversion between a pointer to an incomplete type because it may result in a pointer that is not correctly aligned leading to an undefined behaviour.

Last but no least, if one try to use Zephyr in a Safety&Critical application, there is a huge overlap between the software development recommendations in these standards with the rules in MISRA-C.

[pabigot]

As an IEEE certified software development professional, I sincerely hope nobody involved believes that merely patching Zephyr to conform to MISRA-C:2012 will produce a system that can realistically be used in a safety-critical application. Nor that the effort is measurably improving Zephyr's real-world reliability, no matter how it may contribute to box-checking. (#12384 was found quickly; what has not yet been found?)

[andrewboie]

I sincerely hope nobody involved believes that merely patching Zephyr to conform to MISRA-C:2012 will produce a system that can realistically be used in a safety-critical application.

At no point has anyone said we're going to ensure MISRA-C compliance and just call it a day...

[pabigot]

At no point has anyone said we're going to ensure MISRA-C compliance and just call it a day...

I don't work in safety-critical systems, but the seminars I've attended have pretty uniformly made it clear that, at least for medical devices, any system that was not designed and developed with safety-critical practices starting from day one would never pass, at a minimum for lack of required historical artifacts (functional requirements traces, design documents, test plans, audit trails). Which would rule out ever using Zephyr in that domain.

However, I defer to those with real-world experience in the certification process of a safety-critical system.

[pfalcon]

It appears MISRA-C:2012 compliance requires limiting code to features available in a 20-year-old version of the C language, excluding the option of clean and efficient solutions that require only a 7-year-old version of C.

This can be summarized concisely as: MISRA-C contradicts well established industry standard like ANSI C and POSIX.

[pfalcon]

At no point has anyone said we're going to ensure MISRA-C compliance and just call it a day...

But people in authority of this said:

1. MISRA C "rules" aren't going to applied blindly. Pertinent/useful ones will be selected.
2. MISRA C is only for kernel code. Heck, this ticket is titled "MISRA-C 2012 Compliance in kernel (kernel/*) and base OS code". (It maybe not clear what "base OS code" means. I assume it's difference between e.g. "mov eax, ebx" and e.g. "mov r0, r1", as required to get kernel/* running).

Instead:

1. One of the first thing applied was sweeping change of memset() to (void)memset(). How is that not a nonsensical change?
2. Why stuff like "0U" then proliferates thru net/ and other unrelated areas?

To get this productive, specific suggestions:

1. -1 all pending MISRA PRs which touch anything beyond kernel/*.
2. Revert all MISRA changes outside kernel/* already applied.
3. Finish MISRA work inside kernel/*.
4. Provide a report and RFC what of that would make sense to apply outside kernel/*.

[ceolin]

It appears MISRA-C:2012 compliance requires limiting code to features available in a 20-year-old version of the C language, excluding the option of clean and efficient solutions that require only a 7-year-old version of C.

This can be summarized concisely as: MISRA-C contradicts well established industry standard like ANSI C and POSIX.

These well established standards contains a lot of undefined or unspecified behaviours, MISRA rules are there to avoid them - among other things. Also, as I've mentioned, MISRA-C is not a replacement to ANSI C standards, the latest one is based on C99 and expect that your code is C99 complaint.

[ceolin]

At no point has anyone said we're going to ensure MISRA-C compliance and just call it a day...

I don't work in safety-critical systems, but the seminars I've attended have pretty uniformly made it clear that, at least for medical devices, any system that was not designed and developed with safety-critical practices starting from day one would never pass, at a minimum for lack of required historical artifacts (functional requirements traces, design documents, test plans, audit trails). Which would rule out ever using Zephyr in that domain.

However, I defer to those with real-world experience in the certification process of a safety-critical system.

You're right about that, the code guideline is just a small portion of the requirements. But definitely all of them require that the software follows a well defined code-guideline (which currently don't have) and on top of that they have other requirements for software implementation. MISRA-C overlaps some most of them and also is code guideline that can easily be checked with a static analysis tool.

[ceolin]

At no point has anyone said we're going to ensure MISRA-C compliance and just call it a day...

But people in authority of this said:

1. MISRA C "rules" aren't going to applied blindly. Pertinent/useful ones will be selected.

And they are not. We have plenty of tickets describing tasks and open to discussion. On top of that some things that require change in the logic I'm firing emails asking for comments

2. MISRA C is only for kernel code. Heck, this ticket is titled "MISRA-C 2012 Compliance in kernel (kernel/*) and base OS code". (It maybe not clear what "base OS code" means. I assume it's difference between e.g. "mov eax, ebx" and e.g. "mov r0, r1", as required to get `kernel/*` running).

Instead:

1. One of the first thing applied was sweeping change of `memset()` to `(void)memset()`. How is that not a nonsensical change?

this does not hurt, things more disruptive are definitely being addressed cautiously

2. Why stuff like "0U" then proliferates thru net/ and other unrelated areas?

The justification to this be applied is good enough IMHO. This is a good practice independently of MISRA-C.

To get this productive, specific suggestions:

1. -1 all pending MISRA PRs which touch anything beyond kernel/*.

2. Revert all MISRA changes outside kernel/* already applied.

Regarding the changes out-of-initial scope, they're only things that doesn't cause impact and were automatically generated.

3. Finish MISRA work inside kernel/*.

It is impossible to apply only in kernel/. Since Zephyr's kernel pulls code from other parts, arch/, misc/*

4. Provide a report and RFC what of that would make sense to apply outside kernel/*.

[sslupsky]

I think we should be concentrating on ensuring zephyr can be used with the most recent versions of the c language standards that enables us to take advantage of the huge improvements in the compilers. Namely, c18+. MISRA compliance is an important niche but likely not in the interests of the majority of zephyr users if it slows down adoption of better c standards.

[CJKay]

MISRA C:2012 Amendment 2 introduces guidelines for both C11 and C18.

[ceolin]

MISRA C:2012 Amendment 2 introduces guidelines for both C11 and C18.

But it seems that the features that people want to use is not allowed.

Updates to the C Standard have introduced new language features. The following shall not be used:

● The _Generic operator;
● The _Noreturn function specifier and the <stdnoreturn.h> header file;
● The _Atomic type specifier, the _Atomic type qualifier and the facilities that are specified as
being provided by <stdatomic.h>;
● The _Thread_local storage class specifier and the facilities that are specified as being provided
by <threads.h>;
● The _Alignas alignment specifier, the _Alignof operator and the <stdalign.h> header file;
● Other than defining __STDC_WANT_LIB_EXT

[CJKay]

MISRA C:2012 Amendment 2 introduces guidelines for both C11 and C18.

But it seems that the features that people want to use is not allowed.

Updates to the C Standard have introduced new language features. The following shall not be used:

● The _Generic operator;
● The _Noreturn function specifier and the <stdnoreturn.h> header file;
● The _Atomic type specifier, the _Atomic type qualifier and the facilities that are specified as
being provided by <stdatomic.h>;
● The _Thread_local storage class specifier and the facilities that are specified as being provided
by <threads.h>;
● The _Alignas alignment specifier, the _Alignof operator and the <stdalign.h> header file;
● Other than defining __STDC_WANT_LIB_EXT

The C11 emergent features rule is Required but not Mandatory, so Zephyr can reasonably use these features so long as there is a documented and justified deviation. Not ideal though, admittedly.

[simhein]

@ceolin Would it make sense to assign this issue to me so we can drive this issue in the safety WG?

[ceolin]

@ceolin Would it make sense to assign this issue to me so we can drive this issue in the safety WG?

yes, please :)