|
<rant mode on>
Could someone tell me what is all this MISRA compliance eagerness for?
I can't believe that no one rebelled against that yet.
I just looked at a proposed PR and this makes me want to cry.
Some of the changes are good, like stricter boolean usage.
But the majority of the changes are pure code obfuscation!
Who in its right mind will believe that sprinkling typecasts around
would make the code any safer and understandable? And that's not
mentioning those _double_ typecasts being added too!!! We should instead
aim for cast _reduction_ to let the compiler warn more about type
mismatches and air the code, not add new ones!
Note: Sometimes double casts are necessary, but we do hide them behind
special accessors.
Why do we have to make all loop index vars unsigned? It is a very common
idiom to test for "i < 0" in loops. Making some signed and some unsigned
is just adding to the inconsistency for no gain.
Why do we need to test against an unsigned zero everywhere? Does anyone
know a about a compiler implementation where a signed 0 vs an unsigned
0U makes any difference?
And what is that obsession about qualifying every literal values anyway!
When signed and unsigned expressions are combined, the result is
promoted to unsigned already. So I do ask you frankly which of the
following is the safest and most intuitive:
#define FOO 5
#define BAR 5U
[ ... code far away ... ]
bool foo(int x) { return (x < FOO); }
bool bar(int x) { return (x < BAR); }
Notice how foo(-1) and bar(-1) don't give the same answer. Try it if you
don't believe me.
Whereas:
bool foo(unsigned int x) { return (x < FOO); }
bool bar(unsigned int x) { return (x < BAR); }
Here foo(UINT_MAX) and bar(UINT_MAX) both do give the proper result.
So qualifying literals everywhere when it is not absolutely required is
actually evil and opens the possibility for *more* bugs not less.
And why casting pointer difference to a size_t? The official type for a
pointer difference is ptrdiff_t and the standard offers no explicit
guarantee that sizeof(ptrdiff_t) equals sizeof(size_t). People who live
by the standard will tell you that size_t _could_ be smaller and
therefore silently truncate the result.
What's the point of casting to a char before assigning to a char when
the cast is implicit already?
And a real kicker is this change:
- * @return 0 on success, nonzero on failure
+ * @return false on success, true on failure
Who really thinks that the above is idiomatic and an improvement in code
understandability? It is not that MISRA does require it in this
particular case, but it certainly did skew the mindset of the person who
did that change.
Etc. Etc.
<rant mode off>
So I really do challenge if MISRA compliance is a good thing for the
project. Some of it certainly makes sense. Some of it is really
questionable and should be excluded.
I do guess (even if I don't understand why in the end) that some Zephyr
users do require MISRA compliance for their project. I however do
believe that this is not beneficial for the Zephyr community as a whole,
and that the cost of MISRA compliance and its side effects should not be
imposed on everyone without further scrutiny and discrimination.
Static analysis tools have come a long way and they are far more useful,
realistic and relevant when it comes to code validity and correctness at
this point.
Many embedded libraries and SDK's out there already maintain a MISRA
compliance exception list. I think this would be a sane thing to do and
that wouldn't be a first.
Nicolas
|
|
|
Who in its right mind will believe that sprinkling typecasts around
would make the code any safer and understandable? And that's not
mentioning those _double_ typecasts being added too!!! We should instead
aim for cast _reduction_ to let the compiler warn more about type
mismatches and air the code, not add new ones!
Note: Sometimes double casts are necessary, but we do hide them behind
special accessors. In the specific, double casts are needed in two situations: 1) A char argument to ctype predicates should be first converted to unsigned char (as the C standard requires) and then converted to int to avoid mixing of signed and unsigned. Said that, nothing prevents us from using helpers that accept char argument or to embed such double casts in, e.g., #define CTYPE_CHAR(c) ((int)(unsigned char)(c)) 2) A narrower computation is assigned to a wider or differently signed result, e.g., u64 = u32_a - u32_b; In this case we should write u64 = (uint64_t)(uint32_t)(u32_a - u32_b); This is used to make it explicit that (1) the type in which the difference should be computed is uint32_t and (2) that the (possibly wrapped) result should be converted to the destination type. Without that the risk is writing something like, e.g., u64 = u32_a + u32_b; without realizing that an unwanted wrapping may happen. Of course, if we want an unsigned 64-bit addition, we should write u64 = (uint64_t)u32_a + u32_b; Again, it is possible to encapsulate the cast used to express the "deliberately done" property in a macro. Another possibility to avoid the double cast and remain MISRA compliant is to use an intermediate variable: u32_c = u32_a - u32_b; u64 = u32_c; but I find that is not so nice in some situations and it does not fit well in macros. Why do we have to make all loop index vars unsigned? It is a very common
idiom to test for "i < 0" in loops. Making some signed and some unsigned
is just adding to the inconsistency for no gain.
Why do we need to test against an unsigned zero everywhere? Does anyone
know a about a compiler implementation where a signed 0 vs an unsigned
0U makes any difference? s32 < 0U and s32 < 0 makes a *lot* of difference: this is one of the reasons why MISRA insists so much to not mix signed and unsigned. And what is that obsession about qualifying every literal values anyway!
When signed and unsigned expressions are combined, the result is
promoted to unsigned already. So I do ask you frankly which of the
following is the safest and most intuitive:
#define FOO 5
#define BAR 5U
[ ... code far away ... ]
bool foo(int x) { return (x < FOO); }
bool bar(int x) { return (x < BAR); }
Notice how foo(-1) and bar(-1) don't give the same answer. Try it if you
don't believe me. This is exactly the reason why signed and unsigned should not be mixed. The mistakes above would be marked as non-compliant by any MISRA checker and would not go unnoticed (differently from the situation where such mixing is spread in the code). Whereas:
bool foo(unsigned int x) { return (x < FOO); }
bool bar(unsigned int x) { return (x < BAR); }
Here foo(UINT_MAX) and bar(UINT_MAX) both do give the proper result.
So qualifying literals everywhere when it is not absolutely required is
actually evil and opens the possibility for *more* bugs not less. We might consider deviating for signed constants whose value is unchanged by promotion and usual arithmetic conversion. Would this be ok for you? And why casting pointer difference to a size_t? The official type for a
pointer difference is ptrdiff_t and the standard offers no explicit
guarantee that sizeof(ptrdiff_t) equals sizeof(size_t). People who live
by the standard will tell you that size_t _could_ be smaller and
therefore silently truncate the result. Actually, a standard C implementation must ensure that the maximum (positive) value for ptrdiff_t must be representable as size_t. What's the point of casting to a char before assigning to a char when
the cast is implicit already? Implicit casts can (and very often do) go unnoticed. This is the very reason MISRA asks to make them explicit for the sake of code readability. [Off topic: IMHO we would also have great benefits from introducing BARR-C:2018 Rule 1.6.a, which asks that *all* non-value-preserving casts are accompanied by an explicative comment.] And a real kicker is this change:
- * @return 0 on success, nonzero on failure
+ * @return false on success, true on failure
Who really thinks that the above is idiomatic and an improvement in code
understandability? It is not that MISRA does require it in this
particular case, but it certainly did skew the mindset of the person who
did that change. Here I really don't follow you: if the function was *already* returning a Boolean, why is this change troublesome? Said that, if the intention was *not* to return a Boolean, then the signature should not have been changed and the returned value should have been computed in a different way. The author of this change had no way to deduce the original intention with a 100% chance of success. What is needed is simply urbane feedback of the form "this function signature is deliberately returning an integer for reason X, please amend the MISRA compliance changes taking in account that." Many embedded libraries and SDK's out there already maintain a MISRA
compliance exception list. I think this would be a sane thing to do and
that wouldn't be a first. This is definitely the intention, code quality is *always* the greater priority. Nonetheless I think that the right mindset is that missing adherence to MISRA rules should be justified carefully and not the opposite (i.e., by only following the rules that change nothing or that we are used to). This will help the whole Zephyr community: the part that needs qualifiable/certifiable code and the part that just needs better/safer code. -- Abramo Bagnara BUGSENG srl - http://bugseng.commailto:abramo.bagnara@...
|
|
|
On Sat, Dec 11, 2021 at 2:01 PM Nicolas Pitre <npitre@...> wrote:
<rant mode on>
Could someone tell me what is all this MISRA compliance eagerness for?
I can't believe that no one rebelled against that yet.
I started to question a bunch of the choices in the early PRs, but it was moot. I also got the impression that the mess they are creating now with the enormous amount of obfuscating casting can/should be fixed once they've worked through the bulk of the warnings and can scale back some of them. I _really_ hope that to be the case, and not a "well, the coding is done now, so please merge it into main as well". I worry that'll be a bad experience for everybody. I just looked at a proposed PR and this makes me want to cry.
Some of the changes are good, like stricter boolean usage.
But the majority of the changes are pure code obfuscation!
Who in its right mind will believe that sprinkling typecasts around
would make the code any safer and understandable? And that's not
mentioning those _double_ typecasts being added too!!! We should instead
aim for cast _reduction_ to let the compiler warn more about type
mismatches and air the code, not add new ones! I fully agree. Once they can complete the first pass on their topic branch, I expect there to be a checkpoint with a judgment call on whether it's a net improvement to Zephyr or not, and what to do with it w.r.t. merging piece of it into the main development branches. For now, I am willing to give them the benefit of the doubt, but I do worry about the final outcome being a project that's significantly harder to work with and understand. But I will wait until they're ready for said scruity. [...] So I really do challenge if MISRA compliance is a good thing for the
project. Some of it certainly makes sense. Some of it is really
questionable and should be excluded.
I do guess (even if I don't understand why in the end) that some Zephyr
users do require MISRA compliance for their project. I however do
believe that this is not beneficial for the Zephyr community as a whole,
and that the cost of MISRA compliance and its side effects should not be
imposed on everyone without further scrutiny and discrimination. Again, 100% agree, especially if we start to approach things with "every PR needs to have clean MISRA checks". I certainly worry about the overall readability and general ease of both using (reading/understanding) as well as contributing to the project. But, as I said above, I'm willing to have them do the work and then we see how things look like when it's ready. Static analysis tools have come a long way and they are far more useful,
realistic and relevant when it comes to code validity and correctness at
this point. My understanding is that MISRA and correctness are two completely unrelated concepts, and MISRA is mostly about avoiding undefined behaviors in specifications (that might not be causing bugs). That, plus business motivations of having a code base that is passing all checks -- ("Our code is MISRA clean", and whatever that might do to product malfunction liabilities or other certifications, etc.). Many embedded libraries and SDK's out there already maintain a MISRA
compliance exception list. I think this would be a sane thing to do and
that wouldn't be a first. What would you expect that to look like? Specific rules in MISRA that we're choosing to not conform to? I think that could be a very useful discussion to have once we see how the final outcome looks like, and avoid the most obfuscating ones if we can. -Olof
|
|
|
On Sun, 12 Dec 2021, Abramo Bagnara wrote: Who in its right mind will believe that sprinkling typecasts around
would make the code any safer and understandable? And that's not
mentioning those _double_ typecasts being added too!!! We should instead
aim for cast _reduction_ to let the compiler warn more about type
mismatches and air the code, not add new ones!
Note: Sometimes double casts are necessary, but we do hide them behind
special accessors. In the specific, double casts are needed in two situations:
1) A char argument to ctype predicates should be first converted to
unsigned char (as the C standard requires) and then converted to
int to avoid mixing of signed and unsigned. Said that, nothing
prevents us from using helpers that accept char argument or to
embed such double casts in, e.g.,
#define CTYPE_CHAR(c) ((int)(unsigned char)(c))
That's certainly better. And this provides you with the opportunity to document the reason for such a macro alongside its definition. 2) A narrower computation is assigned to a wider or differently
signed result, e.g.,
u64 = u32_a - u32_b;
In this case we should write
u64 = (uint64_t)(uint32_t)(u32_a - u32_b);
This is used to make it explicit that (1) the type in which the
difference should be computed is uint32_t and (2) that the
(possibly wrapped) result should be converted to the destination
type.
Without that the risk is writing something like, e.g.,
u64 = u32_a + u32_b;
without realizing that an unwanted wrapping may happen. Well, you aren't preventing any wrapping by adding a bunch of casts. Of course, if we want an unsigned 64-bit addition, we should write
u64 = (uint64_t)u32_a + u32_b; Or even: u64 = u32_a; u64 += u32_b; Two lines, but still more enjoyable to read. Again, it is possible to encapsulate the cast used to express the
"deliberately done" property in a macro. Only if that was the actual intent. I doubt such thing was deliberate. Most likely a value wrap is just not a possible outcome ever in a given code context and no one deemed it necessary to explicitly spell such thing out. Assigning to an u64 is indeed a bit suspicious in such a case. Maybe the u64 is unnecessary to start with, and if so that's what needs fixing whereas papering over it with a cast pipeline doesn't improve the code. Why do we have to make all loop index vars unsigned? It is a very common
idiom to test for "i < 0" in loops. Making some signed and some unsigned
is just adding to the inconsistency for no gain.
Why do we need to test against an unsigned zero everywhere? Does anyone
know a about a compiler implementation where a signed 0 vs an unsigned
0U makes any difference? s32 < 0U and s32 < 0 makes a *lot* of difference: this is one of the
reasons why MISRA insists so much to not mix signed and unsigned.
Here I disagree. s32 < 5U is wrong. s32 < 5 is OK u32 < 5U is OK u32 < 5 is OK So the unqualified literal is less cluttered, and always OK. The qualified 5U is ugly, and wrong in mixed signedness cases. So the safest thing to do, and the most convivial for developers and reviewers alike, is to _NOT_ qualify literals as unsigned by default. The signed/unsigned semantic is already carried by variables those literals are used with. We might consider deviating for signed constants whose value is unchanged by
promotion and usual arithmetic conversion. Would this be ok for you? That would be an improvement indeed. And why casting pointer difference to a size_t? The official type for a
pointer difference is ptrdiff_t and the standard offers no explicit
guarantee that sizeof(ptrdiff_t) equals sizeof(size_t). People who live
by the standard will tell you that size_t _could_ be smaller and
therefore silently truncate the result. Actually, a standard C implementation must ensure that the maximum
(positive) value for ptrdiff_t must be representable as
size_t.
OK. But still, what's the point? Just to get rid of the signed nature of it? And a real kicker is this change:
- * @return 0 on success, nonzero on failure
+ * @return false on success, true on failure
Who really thinks that the above is idiomatic and an improvement in code
understandability? It is not that MISRA does require it in this
particular case, but it certainly did skew the mindset of the person who
did that change. Here I really don't follow you: if the function was *already* returning
a Boolean, why is this change troublesome?
Because it is not returning a boolean. It is returning a non-zero value on failure. That's not a boolean. The common operating system idiom in that case is to return an error code, typically negative. If the particular error reason is unnecessary then it is of course a valid simplification to return a boolean instead. But it should be true for success and false for failure which is by far the most common expectation. And then the name of the function should probably be modified as well to make it clear to users that the function is no longer following the error code return paradigm. Otherwise it is best to leave it as is. Nonetheless I think that the right mindset is that missing adherence
to MISRA rules should be justified carefully and not the opposite
(i.e., by only following the rules that change nothing or that we are
used to). I think both cases should be justified carefully. Some rules might have been justified back when people wrote C code K&R style but are just an annoyance with no real benefit nowdays. I've also seen too many times a bug being introduced because the code was modified just to make it [name your favorite static analysis tool / coding standard here]-compliant. It is often the case that the code is questionnably written in the first place, and adding typecasts to it makes it worse even if the scanning tool is then happy. Nicolas
|
|
|
I figure that that the MISRA-C changes are an attempt to step in the direction to having a version of Zephyr that is or can be certified.
I've been involved in the certification process (DO-178C, though there are others) for other embedded OSes in the past and they have admittedly been beasts of process. One of the early steps that we had to go through was ensuring coding standard compliance. There is no doubt that it was both time consuming and tedious and sometimes led to code looking a little weird for compliance purposes. Despite that, the benefits to doing so (in the context of certification) were substantial as it helped to open up new business opportunities. Similarly, I suspect one of the longer term goals here is to help open up new Zephyr adoption opportunities.
I realize that the above does not directly address the points of contention that you raised. By the time I am chiming in on this, I believe that others have already responded to them. My primary point that I am trying to draw attention to is how this could fit into a larger beneficial picture, despite the pain (in what is hoped to be "short-term").
Peter
toggle quoted message
Show quoted text
-----Original Message-----
From: devel@... <devel@...> On Behalf Of Nicolas Pitre
Sent: Saturday, December 11, 2021 5:01 PM
To: devel@...; Kevin Hilman <khilman@...>
Subject: [Zephyr-devel] MISRA
<rant mode on>
Could someone tell me what is all this MISRA compliance eagerness for?
I can't believe that no one rebelled against that yet.
I just looked at a proposed PR and this makes me want to cry.
Some of the changes are good, like stricter boolean usage.
But the majority of the changes are pure code obfuscation!
Who in its right mind will believe that sprinkling typecasts around would make the code any safer and understandable? And that's not mentioning those _double_ typecasts being added too!!! We should instead aim for cast _reduction_ to let the compiler warn more about type mismatches and air the code, not add new ones!
Note: Sometimes double casts are necessary, but we do hide them behind special accessors.
Why do we have to make all loop index vars unsigned? It is a very common idiom to test for "i < 0" in loops. Making some signed and some unsigned is just adding to the inconsistency for no gain.
Why do we need to test against an unsigned zero everywhere? Does anyone know a about a compiler implementation where a signed 0 vs an unsigned 0U makes any difference?
And what is that obsession about qualifying every literal values anyway!
When signed and unsigned expressions are combined, the result is promoted to unsigned already. So I do ask you frankly which of the following is the safest and most intuitive:
#define FOO 5
#define BAR 5U
[ ... code far away ... ]
bool foo(int x) { return (x < FOO); }
bool bar(int x) { return (x < BAR); }
Notice how foo(-1) and bar(-1) don't give the same answer. Try it if you don't believe me.
Whereas:
bool foo(unsigned int x) { return (x < FOO); } bool bar(unsigned int x) { return (x < BAR); }
Here foo(UINT_MAX) and bar(UINT_MAX) both do give the proper result.
So qualifying literals everywhere when it is not absolutely required is actually evil and opens the possibility for *more* bugs not less.
And why casting pointer difference to a size_t? The official type for a pointer difference is ptrdiff_t and the standard offers no explicit guarantee that sizeof(ptrdiff_t) equals sizeof(size_t). People who live by the standard will tell you that size_t _could_ be smaller and therefore silently truncate the result.
What's the point of casting to a char before assigning to a char when the cast is implicit already?
And a real kicker is this change:
- * @return 0 on success, nonzero on failure
+ * @return false on success, true on failure
Who really thinks that the above is idiomatic and an improvement in code understandability? It is not that MISRA does require it in this particular case, but it certainly did skew the mindset of the person who did that change.
Etc. Etc.
<rant mode off>
So I really do challenge if MISRA compliance is a good thing for the project. Some of it certainly makes sense. Some of it is really questionable and should be excluded.
I do guess (even if I don't understand why in the end) that some Zephyr users do require MISRA compliance for their project. I however do believe that this is not beneficial for the Zephyr community as a whole, and that the cost of MISRA compliance and its side effects should not be imposed on everyone without further scrutiny and discrimination.
Static analysis tools have come a long way and they are far more useful, realistic and relevant when it comes to code validity and correctness at this point.
Many embedded libraries and SDK's out there already maintain a MISRA compliance exception list. I think this would be a sane thing to do and that wouldn't be a first.
Nicolas
|
|
|
Hi Nicolas,
Just a note that the project is not seeking MISRA compliance, we only looking at complying with our own published coding guidelines (which is a subset of MISRA).
When we adopted the coding guidelines, it was obvious that during the implementation of such guidelines we will face issues and certain rules while they look good in theory might be infeasible to implement
on a codebase like ours and the agreement was to deal with those by amending the guidelines and eventually removing those rules.
Anas
From: devel@... <devel@...> on behalf of Nicolas Pitre <npitre@...>
Date: Saturday, December 11, 2021 at 5:01 PM
To: devel@... <devel@...>, Kevin Hilman <khilman@...>
Subject: [Zephyr-devel] MISRA
<rant mode on>
Could someone tell me what is all this MISRA compliance eagerness for?
I can't believe that no one rebelled against that yet.
I just looked at a proposed PR and this makes me want to cry.
Some of the changes are good, like stricter boolean usage.
But the majority of the changes are pure code obfuscation!
Who in its right mind will believe that sprinkling typecasts around
would make the code any safer and understandable? And that's not
mentioning those _double_ typecasts being added too!!! We should instead
aim for cast _reduction_ to let the compiler warn more about type
mismatches and air the code, not add new ones!
Note: Sometimes double casts are necessary, but we do hide them behind
special accessors.
Why do we have to make all loop index vars unsigned? It is a very common
idiom to test for "i < 0" in loops. Making some signed and some unsigned
is just adding to the inconsistency for no gain.
Why do we need to test against an unsigned zero everywhere? Does anyone
know a about a compiler implementation where a signed 0 vs an unsigned
0U makes any difference?
And what is that obsession about qualifying every literal values anyway!
When signed and unsigned expressions are combined, the result is
promoted to unsigned already. So I do ask you frankly which of the
following is the safest and most intuitive:
#define FOO 5
#define BAR 5U
[ ... code far away ... ]
bool foo(int x) { return (x < FOO); }
bool bar(int x) { return (x < BAR); }
Notice how foo(-1) and bar(-1) don't give the same answer. Try it if you
don't believe me.
Whereas:
bool foo(unsigned int x) { return (x < FOO); }
bool bar(unsigned int x) { return (x < BAR); }
Here foo(UINT_MAX) and bar(UINT_MAX) both do give the proper result.
So qualifying literals everywhere when it is not absolutely required is
actually evil and opens the possibility for *more* bugs not less.
And why casting pointer difference to a size_t? The official type for a
pointer difference is ptrdiff_t and the standard offers no explicit
guarantee that sizeof(ptrdiff_t) equals sizeof(size_t). People who live
by the standard will tell you that size_t _could_ be smaller and
therefore silently truncate the result.
What's the point of casting to a char before assigning to a char when
the cast is implicit already?
And a real kicker is this change:
- * @return 0 on success, nonzero on failure
+ * @return false on success, true on failure
Who really thinks that the above is idiomatic and an improvement in code
understandability? It is not that MISRA does require it in this
particular case, but it certainly did skew the mindset of the person who
did that change.
Etc. Etc.
<rant mode off>
So I really do challenge if MISRA compliance is a good thing for the
project. Some of it certainly makes sense. Some of it is really
questionable and should be excluded.
I do guess (even if I don't understand why in the end) that some Zephyr
users do require MISRA compliance for their project. I however do
believe that this is not beneficial for the Zephyr community as a whole,
and that the cost of MISRA compliance and its side effects should not be
imposed on everyone without further scrutiny and discrimination.
Static analysis tools have come a long way and they are far more useful,
realistic and relevant when it comes to code validity and correctness at
this point.
Many embedded libraries and SDK's out there already maintain a MISRA
compliance exception list. I think this would be a sane thing to do and
that wouldn't be a first.
Nicolas
|
|
|
Lee Courtney <leec2124@...>
"I figure that that the MISRA-C changes are an attempt to step in the direction to having a version of Zephyr that is or can be certified."
And that is what is being watched by those with safety-critical requirements, and that would benefit from a robust open-source RTOS with significant community and organizational backing.
Lee Courtney
I figure that that the MISRA-C changes are an attempt to step in the direction to having a version of Zephyr that is or can be certified.
I've been involved in the certification process (DO-178C, though there are others) for other embedded OSes in the past and they have admittedly been beasts of process. One of the early steps that we had to go through was ensuring coding standard compliance. There is no doubt that it was both time consuming and tedious and sometimes led to code looking a little weird for compliance purposes. Despite that, the benefits to doing so (in the context of certification) were substantial as it helped to open up new business opportunities. Similarly, I suspect one of the longer term goals here is to help open up new Zephyr adoption opportunities.
I realize that the above does not directly address the points of contention that you raised. By the time I am chiming in on this, I believe that others have already responded to them. My primary point that I am trying to draw attention to is how this could fit into a larger beneficial picture, despite the pain (in what is hoped to be "short-term").
Peter
-----Original Message-----
From: devel@... <devel@...> On Behalf Of Nicolas Pitre
Sent: Saturday, December 11, 2021 5:01 PM
To: devel@...; Kevin Hilman <khilman@...>
Subject: [Zephyr-devel] MISRA
<rant mode on>
Could someone tell me what is all this MISRA compliance eagerness for?
I can't believe that no one rebelled against that yet.
I just looked at a proposed PR and this makes me want to cry.
Some of the changes are good, like stricter boolean usage.
But the majority of the changes are pure code obfuscation!
Who in its right mind will believe that sprinkling typecasts around would make the code any safer and understandable? And that's not mentioning those _double_ typecasts being added too!!! We should instead aim for cast _reduction_ to let the compiler warn more about type mismatches and air the code, not add new ones!
Note: Sometimes double casts are necessary, but we do hide them behind special accessors.
Why do we have to make all loop index vars unsigned? It is a very common idiom to test for "i < 0" in loops. Making some signed and some unsigned is just adding to the inconsistency for no gain.
Why do we need to test against an unsigned zero everywhere? Does anyone know a about a compiler implementation where a signed 0 vs an unsigned 0U makes any difference?
And what is that obsession about qualifying every literal values anyway!
When signed and unsigned expressions are combined, the result is promoted to unsigned already. So I do ask you frankly which of the following is the safest and most intuitive:
#define FOO 5
#define BAR 5U
[ ... code far away ... ]
bool foo(int x) { return (x < FOO); }
bool bar(int x) { return (x < BAR); }
Notice how foo(-1) and bar(-1) don't give the same answer. Try it if you don't believe me.
Whereas:
bool foo(unsigned int x) { return (x < FOO); } bool bar(unsigned int x) { return (x < BAR); }
Here foo(UINT_MAX) and bar(UINT_MAX) both do give the proper result.
So qualifying literals everywhere when it is not absolutely required is actually evil and opens the possibility for *more* bugs not less.
And why casting pointer difference to a size_t? The official type for a pointer difference is ptrdiff_t and the standard offers no explicit guarantee that sizeof(ptrdiff_t) equals sizeof(size_t). People who live by the standard will tell you that size_t _could_ be smaller and therefore silently truncate the result.
What's the point of casting to a char before assigning to a char when the cast is implicit already?
And a real kicker is this change:
- * @return 0 on success, nonzero on failure
+ * @return false on success, true on failure
Who really thinks that the above is idiomatic and an improvement in code understandability? It is not that MISRA does require it in this particular case, but it certainly did skew the mindset of the person who did that change.
Etc. Etc.
<rant mode off>
So I really do challenge if MISRA compliance is a good thing for the project. Some of it certainly makes sense. Some of it is really questionable and should be excluded.
I do guess (even if I don't understand why in the end) that some Zephyr users do require MISRA compliance for their project. I however do believe that this is not beneficial for the Zephyr community as a whole, and that the cost of MISRA compliance and its side effects should not be imposed on everyone without further scrutiny and discrimination.
Static analysis tools have come a long way and they are far more useful, realistic and relevant when it comes to code validity and correctness at this point.
Many embedded libraries and SDK's out there already maintain a MISRA compliance exception list. I think this would be a sane thing to do and that wouldn't be a first.
Nicolas
-- Lee Courtney +1-650-704-3934 cell
|
|
|
On Mon, 13 Dec 2021, Lee Courtney wrote: "I figure that that the MISRA-C changes are an attempt to step in the
direction to having a version of Zephyr that is or can be certified."
And that is what is being watched by those with safety-critical
requirements, and that would benefit from a robust open-source RTOS with
significant community and organizational backing. Significant community backing won't come about if the project blindly adopts obnoxious rules, especially with some of them having little intrinsic value, just to let a few brag certification compliance. What's the point of certification if you drive part of your community away in doing so? Might as well just use a commercial RTOS where no "community" is implied. Nicolas
|
|
|
I find this discussion extremely interesting.
I don't come from a linux background. I historically have worked with MCU's in a bare metal or conventional rtos context. While I'm not one to be a MISRA stickler. I do believe in a lot of the policies.
Example: I gratuitously typedef's Reasons:
1. Tends to make code more self documenting 2. Though it may be weak enforcement, typedef enables you to at least have the illusion of strong types. Compiler will remind you
I'd love to discuss this topic further because I'm curious if it uncovers a division within the community between linux developers and 'MCU' developers.
-Garrett LoVerde
On Mon, Dec 13, 2021 at 1:44 PM Nicolas Pitre < nico@...> wrote: On Mon, 13 Dec 2021, Lee Courtney wrote:
> "I figure that that the MISRA-C changes are an attempt to step in the
> direction to having a version of Zephyr that is or can be certified."
>
> And that is what is being watched by those with safety-critical
> requirements, and that would benefit from a robust open-source RTOS with
> significant community and organizational backing.
Significant community backing won't come about if the project blindly
adopts obnoxious rules, especially with some of them having little
intrinsic value, just to let a few brag certification compliance.
What's the point of certification if you drive part of your community
away in doing so? Might as well just use a commercial RTOS where no
"community" is implied.
Nicolas
|
|
|
Hopefully most people in the community agree that many, if not most, Misra rules are outdated or even slightly harmful. But you optimize with the constraint being the way the world works, not how it should ideally work. If this is what it takes to get zephyr backed by Daimler and Volvo, I for one can't blame the steering committee for thinking the tradeoff is justified. I'd do the same thing probably, despite loathing Misra with all my heart.
On Mon, Dec 13, 2021, 7:44 PM Nicolas Pitre < nico@...> wrote: On Mon, 13 Dec 2021, Lee Courtney wrote:
> "I figure that that the MISRA-C changes are an attempt to step in the
> direction to having a version of Zephyr that is or can be certified."
>
> And that is what is being watched by those with safety-critical
> requirements, and that would benefit from a robust open-source RTOS with
> significant community and organizational backing.
Significant community backing won't come about if the project blindly
adopts obnoxious rules, especially with some of them having little
intrinsic value, just to let a few brag certification compliance.
What's the point of certification if you drive part of your community
away in doing so? Might as well just use a commercial RTOS where no
"community" is implied.
Nicolas
|
|
|
On Mon, 13 Dec 2021, Benjamin Lindqvist wrote: Hopefully most people in the community agree that many, if not most, Misra
rules are outdated or even slightly harmful. But you optimize with the
constraint being the way the world works, not how it should ideally work.
If this is what it takes to get zephyr backed by Daimler and Volvo, I for
one can't blame the steering committee for thinking the tradeoff is
justified. Daimler and Volvo are relying on Linux already in many unsuspected ways. Yet, the Linux community has vehemently rejected many of the "best practice" mantras taught by academia and sought by the industry. It is undeniably the most dynamic community out there, and Linux adoption is unrivaled. Maybe the health of its community compensates the lack of formal certification? I'd do the same thing probably, despite loathing Misra with all
my heart. If your employer asks you to do so, and your living depends on it then yo have no choice but to swallow the frog. Hopefully you're fairly compensated in return. But asking the same of a "community" and hoping for it to thrive is delusional. Nicolas
|
|
|
Il 13/12/21 04:37, Nicolas Pitre ha scritto: I've also seen too many times a bug being introduced because the code
was modified just to make it [name your favorite static analysis tool /
coding standard here]-compliant. It is often the case that the code is
questionnably written in the first place, and adding typecasts to it
makes it worse even if the scanning tool is then happy. I'd like make clear once and for all that the happiness of scanning tool is *never* a valuable purpose under a MISRA perspective. What matter is *only* the quality of code and MISRA is a tool to achieve this purpose. Il 13/12/21 04:37, Benjamin Lindqvist ha scritto: Hopefully most people in the community agree that many, if not most,
Misra rules are outdated or even slightly harmful. But you optimize
with the constraint being the way the world works, not how it should
ideally work. If this is what it takes to get zephyr backed by Daimler
and Volvo, I for one can't blame the steering committee for thinking
the tradeoff is justified. I'd do the same thing probably, despite
loathing Misra with all my heart. In my experience this is a consequence of a misunderstanding of what MISRA really is. MISRA process has rules, but also deviations and permits and the only sane way to use it is as a tool to improve safety/readability/understanding/analyzability. Please forget every other idea of it as an enemy to beat or to surrender to. As Nicolas has already done you are welcome to point out *any* proposed change related to MISRA compliance that you think will make code worse together with an alternative proposal to improve code at the same time. I hope this will clarify that everyone in the community has the same code improvement goal and, as usual, constructive collaboration is the key. -- Abramo Bagnara BUGSENG srl - http://bugseng.commailto:abramo.bagnara@...
|
|
|
Well if code quality is truly the only goal of the steering committee and there's no politics involved then I retract everything I said. Misra offers less than nothing for this project. But I don't believe that's true. This is 100% politics if you ask me.
Also before declaring the virtues of Misra, you'll be sure to clarify any vested financial interest you might have in the matter, right? You know, just to make sure you're not shilling.
Il 13/12/21 04:37, Nicolas Pitre ha scritto:
> I've also seen too many times a bug being introduced because the code
> was modified just to make it [name your favorite static analysis tool /
> coding standard here]-compliant. It is often the case that the code is
> questionnably written in the first place, and adding typecasts to it
> makes it worse even if the scanning tool is then happy.
I'd like make clear once and for all that the happiness of scanning tool
is *never* a valuable purpose under a MISRA perspective.
What matter is *only* the quality of code and MISRA is a tool to achieve
this purpose.
Il 13/12/21 04:37, Benjamin Lindqvist ha scritto:
> Hopefully most people in the community agree that many, if not most,
> Misra rules are outdated or even slightly harmful. But you optimize
> with the constraint being the way the world works, not how it should
> ideally work. If this is what it takes to get zephyr backed by Daimler
> and Volvo, I for one can't blame the steering committee for thinking
> the tradeoff is justified. I'd do the same thing probably, despite
> loathing Misra with all my heart.
In my experience this is a consequence of a misunderstanding of what
MISRA really is.
MISRA process has rules, but also deviations and permits and the only
sane way to use it is as a tool to improve
safety/readability/understanding/analyzability.
Please forget every other idea of it as an enemy to beat or to surrender to.
As Nicolas has already done you are welcome to point out *any* proposed
change related to MISRA compliance that you think will make code worse
together with an alternative proposal to improve code at the same time.
I hope this will clarify that everyone in the community has the same
code improvement goal and, as usual, constructive collaboration is the key.
--
Abramo Bagnara
BUGSENG srl - http://bugseng.com
mailto:abramo.bagnara@...
|
|
|
All,
- As a reminder, the TSC agreed upon the
Zephyr Project Coding Guidelines. We are working towards compliance to the Project CG. Yes, they are a subset of Misra, but we are not going for Misra compliance.
- The Safety Committee, in driving toward the Project goal of certification-ready, has partnered with Bugseng, as part of the community, to help build momentum towards this goal.
- There is a process for removing, revising, having exceptions, etc to the CG rules. If you think a rule should be changed, open an issue in Github.
The Safety Committee is bringing an update to the TSC tomorrow, where we can have a discussion around the CG compliance work.
-Amber
toggle quoted message
Show quoted text
From: devel@... <devel@...>
On Behalf Of Benjamin Lindqvist
Sent: Monday, December 13, 2021 2:44 PM
To: Abramo Bagnara <abramo.bagnara@...>
Cc: Pitre, Nicolas <npitre@...>; Zephyr Devel <devel@...>; Kevin Hilman <khilman@...>
Subject: Re: [Zephyr-devel] MISRA
Well if code quality is truly the only goal of the steering committee and there's no politics involved then I retract everything I said. Misra offers less than nothing for this project. But I don't believe that's true. This is 100% politics
if you ask me.
Also before declaring the virtues of Misra, you'll be sure to clarify any vested financial interest you might have in the matter, right? You know, just to make sure you're not shilling.
Il 13/12/21 04:37, Nicolas Pitre ha scritto:
> I've also seen too many times a bug being introduced because the code
> was modified just to make it [name your favorite static analysis tool /
> coding standard here]-compliant. It is often the case that the code is
> questionnably written in the first place, and adding typecasts to it
> makes it worse even if the scanning tool is then happy.
I'd like make clear once and for all that the happiness of scanning tool
is *never* a valuable purpose under a MISRA perspective.
What matter is *only* the quality of code and MISRA is a tool to achieve
this purpose.
Il 13/12/21 04:37, Benjamin Lindqvist ha scritto:
> Hopefully most people in the community agree that many, if not most,
> Misra rules are outdated or even slightly harmful. But you optimize
> with the constraint being the way the world works, not how it should
> ideally work. If this is what it takes to get zephyr backed by Daimler
> and Volvo, I for one can't blame the steering committee for thinking
> the tradeoff is justified. I'd do the same thing probably, despite
> loathing Misra with all my heart.
In my experience this is a consequence of a misunderstanding of what
MISRA really is.
MISRA process has rules, but also deviations and permits and the only
sane way to use it is as a tool to improve
safety/readability/understanding/analyzability.
Please forget every other idea of it as an enemy to beat or to surrender to.
As Nicolas has already done you are welcome to point out *any* proposed
change related to MISRA compliance that you think will make code worse
together with an alternative proposal to improve code at the same time.
I hope this will clarify that everyone in the community has the same
code improvement goal and, as usual, constructive collaboration is the key.
--
Abramo Bagnara
BUGSENG srl - http://bugseng.com
mailto:abramo.bagnara@...
|
|
|
All,
- As a reminder, the TSC agreed upon the
Zephyr Project Coding Guidelines. We are working towards compliance to the Project CG. Yes, they are a subset of Misra, but we are not going for Misra compliance.
- The Safety Committee, in driving toward the Project goal of certification-ready, has partnered with Bugseng, as part of the community, to help build momentum towards this goal.
- There is a process for removing, revising, having exceptions, etc to the CG rules. If you think a rule should be changed, open an issue in Github.
The Safety Committee is bringing an update to the TSC tomorrow, where we can have a discussion around the CG compliance work.
It’d be great to have a pre-read to frame the discussion and help us can come prepared with questions based on said update (the linked Nov 10 email in last week’s minutes is fairly high level).
Unfortunately, TSC agendas don’t seem to be communicated in a timely fashion any more, usually sent out mere hours before the meeting (which is early morning for us on the US west coast).
It makes it impossible to tell if it’s worth attending calls or not beforehand, and I normally skip those kind of meetings to reduce meeting overhead.
I hope we can change this to be a smoother process in the future and optimize the value of a one hour weekly meeting.
-Olof
Well if code quality is truly the only goal of the steering committee and there's no politics involved then I retract everything I said. Misra offers less than nothing for this project. But I don't believe that's true. This is 100% politics
if you ask me.
Also before declaring the virtues of Misra, you'll be sure to clarify any vested financial interest you might have in the matter, right? You know, just to make sure you're not shilling.
Il 13/12/21 04:37, Nicolas Pitre ha scritto:
> I've also seen too many times a bug being introduced because the code
> was modified just to make it [name your favorite static analysis tool /
> coding standard here]-compliant. It is often the case that the code is
> questionnably written in the first place, and adding typecasts to it
> makes it worse even if the scanning tool is then happy.
I'd like make clear once and for all that the happiness of scanning tool
is *never* a valuable purpose under a MISRA perspective.
What matter is *only* the quality of code and MISRA is a tool to achieve
this purpose.
Il 13/12/21 04:37, Benjamin Lindqvist ha scritto:
> Hopefully most people in the community agree that many, if not most,
> Misra rules are outdated or even slightly harmful. But you optimize
> with the constraint being the way the world works, not how it should
> ideally work. If this is what it takes to get zephyr backed by Daimler
> and Volvo, I for one can't blame the steering committee for thinking
> the tradeoff is justified. I'd do the same thing probably, despite
> loathing Misra with all my heart.
In my experience this is a consequence of a misunderstanding of what
MISRA really is.
MISRA process has rules, but also deviations and permits and the only
sane way to use it is as a tool to improve
safety/readability/understanding/analyzability.
Please forget every other idea of it as an enemy to beat or to surrender to.
As Nicolas has already done you are welcome to point out *any* proposed
change related to MISRA compliance that you think will make code worse
together with an alternative proposal to improve code at the same time.
I hope this will clarify that everyone in the community has the same
code improvement goal and, as usual, constructive collaboration is the key.
--
Abramo Bagnara
BUGSENG srl - http://bugseng.com
mailto:abramo.bagnara@...
|
|
|
Olof,
Point taken, will start posting agenda the day before.
Anas
From: devel@... <devel@...> on behalf of Olof Johansson <olof@...>
Date: Wednesday, December 15, 2021 at 2:14 AM
To: Hibberd, Amber M <amber.m.hibberd@...>
Cc: Abramo Bagnara <abramo.bagnara@...>, Benjamin Lindqvist <benjamin.lindqvist@...>, Kevin Hilman <khilman@...>, Pitre, Nicolas <npitre@...>, Zephyr Devel <devel@...>
Subject: Re: [Zephyr-devel] MISRA
toggle quoted message
Show quoted text
On Tue, Dec 14, 2021 at 16:42 Hibberd, Amber M <amber.m.hibberd@...>
wrote:
All,
1.
As a reminder, the TSC agreed upon the
Zephyr Project Coding Guidelines. We are working towards compliance to the Project CG. Yes, they
are a subset of Misra, but we are not going for Misra compliance.
2.
The Safety Committee, in driving toward the Project goal of certification-ready, has partnered with Bugseng, as part of the community, to help build momentum towards this goal.
3.
There is a process for removing, revising, having exceptions, etc to the CG rules. If you think a rule should be changed, open an issue in Github.
The Safety Committee is bringing an update to the TSC tomorrow, where we can have a discussion around the CG compliance work.
It’d be great to have a pre-read to frame the discussion and help us can come prepared with questions based on said update (the linked Nov 10 email in last week’s minutes is fairly
high level).
Unfortunately, TSC agendas don’t seem to be communicated in a timely fashion any more, usually sent out mere hours before the meeting (which is early morning for us on the US west
coast).
It makes it impossible to tell if it’s worth attending calls or not beforehand, and I normally skip those kind of meetings to reduce meeting overhead.
I hope we can change this to be a smoother process in the future and optimize the value of a one hour weekly meeting.
Well if code quality is truly the only goal of the steering committee and there's no politics involved then I retract everything I said. Misra offers less than nothing for this project. But I don't believe that's
true. This is 100% politics if you ask me.
Also before declaring the virtues of Misra, you'll be sure to clarify any vested financial interest you might have in the matter, right? You know, just to make sure you're not shilling.
Il 13/12/21 04:37, Nicolas Pitre ha scritto:
> I've also seen too many times a bug being introduced because the code
> was modified just to make it [name your favorite static analysis tool /
> coding standard here]-compliant. It is often the case that the code is
> questionnably written in the first place, and adding typecasts to it
> makes it worse even if the scanning tool is then happy.
I'd like make clear once and for all that the happiness of scanning tool
is *never* a valuable purpose under a MISRA perspective.
What matter is *only* the quality of code and MISRA is a tool to achieve
this purpose.
Il 13/12/21 04:37, Benjamin Lindqvist ha scritto:
> Hopefully most people in the community agree that many, if not most,
> Misra rules are outdated or even slightly harmful. But you optimize
> with the constraint being the way the world works, not how it should
> ideally work. If this is what it takes to get zephyr backed by Daimler
> and Volvo, I for one can't blame the steering committee for thinking
> the tradeoff is justified. I'd do the same thing probably, despite
> loathing Misra with all my heart.
In my experience this is a consequence of a misunderstanding of what
MISRA really is.
MISRA process has rules, but also deviations and permits and the only
sane way to use it is as a tool to improve
safety/readability/understanding/analyzability.
Please forget every other idea of it as an enemy to beat or to surrender to.
As Nicolas has already done you are welcome to point out *any* proposed
change related to MISRA compliance that you think will make code worse
together with an alternative proposal to improve code at the same time.
I hope this will clarify that everyone in the community has the same
code improvement goal and, as usual, constructive collaboration is the key.
--
Abramo Bagnara
BUGSENG srl - http://bugseng.com
mailto:abramo.bagnara@...
|
|
|
A few points from my side (probably nothing that has not been already said) - MISRA mostly tries to avoid undefined behavior derived issues. Other languages such as Rust have done a better job on this topic from the very beginning.
- Not all rules are mandatory to make a codebase MISRA compliant, so a project can decide to deviate where it makes sense.
- If seeking for MISRA compliance, the coding style used by a project needs to introduce certain rules (e.g. if always needs to use braces). From my perspective this is not a problem, I, personally, prefer consistent codebases even if they have certain rules I'm not 100% happy with. If we take for example the STM32 HAL (which is MISRA C 2012 compliant) one will observe that code is much more consistent compared to Zephyr. Nowadays, Zephyr just runs checkpatch, which loosely enforces Linux Kernel coding style.
- Zephyr has the potential to target safety critical applications, so I think it makes sense to certify core parts of the project. Without that, there are markets that are simply out of the game. This is likely not a concern for Linux.
Gerard
Olof,
Point taken, will start posting agenda the day before.
Anas
All,
1.
As a reminder, the TSC agreed upon the
Zephyr Project Coding Guidelines. We are working towards compliance to the Project CG. Yes, they
are a subset of Misra, but we are not going for Misra compliance.
2.
The Safety Committee, in driving toward the Project goal of certification-ready, has partnered with Bugseng, as part of the community, to help build momentum towards this goal.
3.
There is a process for removing, revising, having exceptions, etc to the CG rules. If you think a rule should be changed, open an issue in Github.
The Safety Committee is bringing an update to the TSC tomorrow, where we can have a discussion around the CG compliance work.
It’d be great to have a pre-read to frame the discussion and help us can come prepared with questions based on said update (the linked Nov 10 email in last week’s minutes is fairly
high level).
Unfortunately, TSC agendas don’t seem to be communicated in a timely fashion any more, usually sent out mere hours before the meeting (which is early morning for us on the US west
coast).
It makes it impossible to tell if it’s worth attending calls or not beforehand, and I normally skip those kind of meetings to reduce meeting overhead.
I hope we can change this to be a smoother process in the future and optimize the value of a one hour weekly meeting.
Well if code quality is truly the only goal of the steering committee and there's no politics involved then I retract everything I said. Misra offers less than nothing for this project. But I don't believe that's
true. This is 100% politics if you ask me.
Also before declaring the virtues of Misra, you'll be sure to clarify any vested financial interest you might have in the matter, right? You know, just to make sure you're not shilling.
Il 13/12/21 04:37, Nicolas Pitre ha scritto:
> I've also seen too many times a bug being introduced because the code
> was modified just to make it [name your favorite static analysis tool /
> coding standard here]-compliant. It is often the case that the code is
> questionnably written in the first place, and adding typecasts to it
> makes it worse even if the scanning tool is then happy.
I'd like make clear once and for all that the happiness of scanning tool
is *never* a valuable purpose under a MISRA perspective.
What matter is *only* the quality of code and MISRA is a tool to achieve
this purpose.
Il 13/12/21 04:37, Benjamin Lindqvist ha scritto:
> Hopefully most people in the community agree that many, if not most,
> Misra rules are outdated or even slightly harmful. But you optimize
> with the constraint being the way the world works, not how it should
> ideally work. If this is what it takes to get zephyr backed by Daimler
> and Volvo, I for one can't blame the steering committee for thinking
> the tradeoff is justified. I'd do the same thing probably, despite
> loathing Misra with all my heart.
In my experience this is a consequence of a misunderstanding of what
MISRA really is.
MISRA process has rules, but also deviations and permits and the only
sane way to use it is as a tool to improve
safety/readability/understanding/analyzability.
Please forget every other idea of it as an enemy to beat or to surrender to.
As Nicolas has already done you are welcome to point out *any* proposed
change related to MISRA compliance that you think will make code worse
together with an alternative proposal to improve code at the same time.
I hope this will clarify that everyone in the community has the same
code improvement goal and, as usual, constructive collaboration is the key.
--
Abramo Bagnara
BUGSENG srl - http://bugseng.com
mailto:abramo.bagnara@...
--
Gerard Marull-ParetasTeslabs Engineering S.L.
CONFIDENTIALITY NOTICE: The contents of this email message and any attachments are intended solely for the addressee(s) and may contain confidential and/or privileged information and may be legally protected from disclosure. If you are not the intended recipient of this message or their agent, or if this message has been addressed to you in error, please immediately alert the sender by reply email and then delete this message and any attachments. If you are not the intended recipient, you are hereby notified that any use, dissemination, copying, or storage of this message or its attachments is strictly prohibited.
|
|
|
Hi, I've written this email two days ago and somehow it never left my outbox so I'm re-sending it because I think it is still relevant. Well if code quality is truly the only goal of the steering committee and
there's no politics involved then I retract everything I said. Misra offers
less than nothing for this project. But I don't believe that's true. This is
100% politics if you ask me. We should keep this discussion as much technical as possible. The code guidelines rules we are following were discussed in the past involving the whole community and not by small group of people. That's being said, they are not written in stone, we can review them to add changes or completely remove if we want, just open an issue and expose technical reasons. I really don't get this statement "offers less than nothing for this project" ... Most MISRA guidelines address undefined / unspecified behaviors in the C language. For a project that aims to support multiple compilers and platforms this seems to be a good thing.
Also before declaring the virtues of Misra, you'll be sure to clarify any
vested financial interest you might have in the matter, right? You know, just
to make sure you're not shilling.
Abramo is putting technical arguments to justify the changes, please challenge the arguments. Everyone here has its own interests.
On Mon, Dec 13, 2021, 9:38 PM Abramo Bagnara <abramo.bagnara@...>
wrote:
Il 13/12/21 04:37, Nicolas Pitre ha scritto:
> I've also seen too many times a bug being introduced because the code
> was modified just to make it [name your favorite static analysis tool /
> coding standard here]-compliant. It is often the case that the code is
> questionnably written in the first place, and adding typecasts to it
> makes it worse even if the scanning tool is then happy.
I'd like make clear once and for all that the happiness of scanning tool
is *never* a valuable purpose under a MISRA perspective.
What matter is *only* the quality of code and MISRA is a tool to achieve
this purpose.
Il 13/12/21 04:37, Benjamin Lindqvist ha scritto:
> Hopefully most people in the community agree that many, if not most,
> Misra rules are outdated or even slightly harmful. But you optimize
> with the constraint being the way the world works, not how it should
> ideally work. If this is what it takes to get zephyr backed by Daimler
> and Volvo, I for one can't blame the steering committee for thinking
> the tradeoff is justified. I'd do the same thing probably, despite
> loathing Misra with all my heart.
In my experience this is a consequence of a misunderstanding of what
MISRA really is.
MISRA process has rules, but also deviations and permits and the only
sane way to use it is as a tool to improve
safety/readability/understanding/analyzability.
Please forget every other idea of it as an enemy to beat or to surrender
to.
As Nicolas has already done you are welcome to point out *any* proposed
change related to MISRA compliance that you think will make code worse
together with an alternative proposal to improve code at the same time.
I hope this will clarify that everyone in the community has the same
code improvement goal and, as usual, constructive collaboration is the key.
--
Abramo Bagnara
BUGSENG srl - http://bugseng.com
mailto:abramo.bagnara@...
Regards, Flavio Ceolin
|
|
|
I appreciate your points, and similarly for a few others before yours,
but I think they are missing the point.
No one is against virtue.
Everybody wants the code base to be more consistent, more secure, etc.
But mechanically making the code MISRA compliant doesn't achieve that.
I persist in insisting that adding any more typecasts to a codebase
makes it worse. Typecasts should be _removed_ as much as possible, not
added. Thinking about how to avoid a cast produces better code almost
all the time. I would hardly trust "compliant" code for safety critical
applications if compliance came about due to added typecasts.
Same thing for qualifying literals as unsigned. I made the demonstration
already showing how evil that is. And no, just saying that the MISRA
scan tool will spot mixed signedness and complain -- that's also missing
the point. It is just safer, clearer, lighter and more enjoyable to just
_not_ qualify literals when not required, and let the compiler promote
expressions to unsigned when the semantic-carrying non-literal part is
unsigned, at which point you don't need a tool to spot mixed signedness
because they're just OK. For those rare cases where mixed signedness is
bad there are tricks to spot them e.g. look at the Linux min() and max()
implementations where the constraint is encapsulated in one place and
not spread all over the entire code.
Avoiding so called "undefined" syntax when all existing compiler
implementations handle many of them just fine is stupid. Usually there
is a couple reasons why compilers still implement some "undefined"
behaviors: it is totally logical, uncontroversial, and also useful by
making the code clearer and easier to maintain. Instead of banning them
outright, we should instead implement a test safeguarding our reliance
on them.
I don't care about braces everywhere. Well, that's not true: I do hate
them, but that I can live with and there is a technical argument in
favor of them and no real technical arguments against.
Etc. Etc.
So let's stop thinking that XYZ conformance automatically makes the code
intrinsicly better. It doesn't. It may bring a false sense of security
though.
toggle quoted message
Show quoted text
On Wed, 15 Dec 2021, Gerard Marull Paretas wrote: A few points from my side (probably nothing that has not been already said)
- MISRA mostly tries to avoid undefined behavior derived issues. Other
languages such as Rust have done a better job on this topic from the very
beginning.
- Not all rules are mandatory to make a codebase MISRA compliant, so a
project can decide to deviate where it makes sense.
- If seeking for MISRA compliance, the coding style used by a project
needs to introduce certain rules (e.g. if always needs to use braces). From
my perspective this is not a problem, I, personally, prefer consistent
codebases even if they have certain rules I'm not 100% happy with. If we
take for example the STM32 HAL (which is MISRA C 2012 compliant) one will
observe that code is much more consistent compared to Zephyr. Nowadays,
Zephyr just runs checkpatch, which loosely enforces Linux Kernel coding
style.
- Zephyr has the potential to target safety critical applications, so I
think it makes sense to certify core parts of the project. Without that,
there are markets that are simply out of the game. This is likely not a
concern for Linux.
Gerard
On Wed, Dec 15, 2021 at 2:35 PM Nashif, Anas <anas.nashif@...> wrote:
Olof,
Point taken, will start posting agenda the day before.
Anas
*From: *devel@... <devel@...> on
behalf of Olof Johansson <olof@...>
*Date: *Wednesday, December 15, 2021 at 2:14 AM
*To: *Hibberd, Amber M <amber.m.hibberd@...>
*Cc: *Abramo Bagnara <abramo.bagnara@...>, Benjamin Lindqvist <
benjamin.lindqvist@...>, Kevin Hilman <khilman@...>,
Pitre, Nicolas <npitre@...>, Zephyr Devel <
devel@...>
*Subject: *Re: [Zephyr-devel] MISRA
On Tue, Dec 14, 2021 at 16:42 Hibberd, Amber M <amber.m.hibberd@...>
wrote:
All,
1. As a reminder, the TSC agreed upon the Zephyr Project Coding
Guidelines
<https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html>.
We are working towards compliance to the Project CG. Yes, they are a subset
of Misra, but we are not going for Misra compliance.
2. The Safety Committee, in driving toward the Project goal of
certification-ready, has partnered with Bugseng, as part of the community,
to help build momentum towards this goal.
3. There is a process for removing, revising, having exceptions, etc to
the CG rules. If you think a rule should be changed, open an issue in
Github.
The Safety Committee is bringing an update to the TSC tomorrow, where we
can have a discussion around the CG compliance work.
It’d be great to have a pre-read to frame the discussion and help us can
come prepared with questions based on said update (the linked Nov 10 email
in last week’s minutes is fairly high level).
Unfortunately, TSC agendas don’t seem to be communicated in a timely
fashion any more, usually sent out mere hours before the meeting (which is
early morning for us on the US west coast).
It makes it impossible to tell if it’s worth attending calls or not
beforehand, and I normally skip those kind of meetings to reduce meeting
overhead.
I hope we can change this to be a smoother process in the future and
optimize the value of a one hour weekly meeting.
-Olof
-Amber
*From:* devel@... <devel@...> *On
Behalf Of *Benjamin Lindqvist
*Sent:* Monday, December 13, 2021 2:44 PM
*To:* Abramo Bagnara <abramo.bagnara@...>
*Cc:* Pitre, Nicolas <npitre@...>; Zephyr Devel <
devel@...>; Kevin Hilman <khilman@...>
*Subject:* Re: [Zephyr-devel] MISRA
Well if code quality is truly the only goal of the steering committee and
there's no politics involved then I retract everything I said. Misra offers
less than nothing for this project. But I don't believe that's true. This
is 100% politics if you ask me.
Also before declaring the virtues of Misra, you'll be sure to clarify any
vested financial interest you might have in the matter, right? You know,
just to make sure you're not shilling.
On Mon, Dec 13, 2021, 9:38 PM Abramo Bagnara <abramo.bagnara@...>
wrote:
Il 13/12/21 04:37, Nicolas Pitre ha scritto:
I've also seen too many times a bug being introduced because the code
was modified just to make it [name your favorite static analysis tool /
coding standard here]-compliant. It is often the case that the code is
questionnably written in the first place, and adding typecasts to it
makes it worse even if the scanning tool is then happy. I'd like make clear once and for all that the happiness of scanning tool
is *never* a valuable purpose under a MISRA perspective.
What matter is *only* the quality of code and MISRA is a tool to achieve
this purpose.
Il 13/12/21 04:37, Benjamin Lindqvist ha scritto:
Hopefully most people in the community agree that many, if not most, > Misra rules are outdated or even slightly harmful. But you optimize
> with the constraint being the way the world works, not how it should
> ideally work. If this is what it takes to get zephyr backed by Daimler
> and Volvo, I for one can't blame the steering committee for thinking
> the tradeoff is justified. I'd do the same thing probably, despite
> loathing Misra with all my heart.
In my experience this is a consequence of a misunderstanding of what
MISRA really is.
MISRA process has rules, but also deviations and permits and the only
sane way to use it is as a tool to improve
safety/readability/understanding/analyzability.
Please forget every other idea of it as an enemy to beat or to surrender
to.
As Nicolas has already done you are welcome to point out *any* proposed
change related to MISRA compliance that you think will make code worse
together with an alternative proposal to improve code at the same time.
I hope this will clarify that everyone in the community has the same
code improvement goal and, as usual, constructive collaboration is the key.
--
Abramo Bagnara
BUGSENG srl - http://bugseng.com
mailto:abramo.bagnara@...
--
*Gerard Marull-Paretas*
Teslabs Engineering S.L.
teslabs.com T. +34 622 321 312
CONFIDENTIALITY NOTICE:
The contents of this email message and any attachments are intended solely
for the addressee(s)
and may contain confidential and/or privileged information and may be
legally protected from
disclosure. If you are not the intended recipient of this message or their
agent, or if this message
has been addressed to you in error, please immediately alert the sender by
reply email and then
delete this message and any attachments. If you are not the intended
recipient, you are hereby
notified that any use, dissemination, copying, or storage of this message
or its attachments is
strictly prohibited.
|
|
|
Il 15/12/21 19:50, Nicolas Pitre ha scritto: I appreciate your points, and similarly for a few others before yours,
but I think they are missing the point.
No one is against virtue.
Everybody wants the code base to be more consistent, more secure, etc.
But mechanically making the code MISRA compliant doesn't achieve that.
I persist in insisting that adding any more typecasts to a codebase
makes it worse. Typecasts should be _removed_ as much as possible, not
added. Thinking about how to avoid a cast produces better code almost
all the time. I would hardly trust "compliant" code for safety critical
applications if compliance came about due to added typecasts. Are you really arguing that a commented explicit cast (for non obviously value-preserving conversion) is worse than an implicit cast? Same thing for qualifying literals as unsigned. I made the demonstration
already showing how evil that is. And no, just saying that the MISRA
scan tool will spot mixed signedness and complain -- that's also missing
the point. It is just safer, clearer, lighter and more enjoyable to just
_not_ qualify literals when not required, and let the compiler promote
expressions to unsigned when the semantic-carrying non-literal part is
unsigned, at which point you don't need a tool to spot mixed signedness
because they're just OK. For those rare cases where mixed signedness is
bad there are tricks to spot them e.g. look at the Linux min() and max()
implementations where the constraint is encapsulated in one place and
not spread all over the entire code.
Avoiding so called "undefined" syntax when all existing compiler
implementations handle many of them just fine is stupid. Usually there
is a couple reasons why compilers still implement some "undefined"
behaviors: it is totally logical, uncontroversial, and also useful by
making the code clearer and easier to maintain. Instead of banning them
outright, we should instead implement a test safeguarding our reliance
on them. Are you taking in account that although you can write tests for compiler *extensions* (defining otherwise undefined behaviors), you definitely can not write tests for compiler undefined and unspecified behaviors? So let's stop thinking that XYZ conformance automatically makes the code
intrinsicly better. It doesn't. It may bring a false sense of security
though. IMHO you are victim of an huge misunderstanding: the aim is *only* better code quality and conformance is exclusively a mean to achieve that. When a specific instance of conformance to whatever rule hurts code quality it should never been done, who says otherwise prove it does not know XYZ (supposing it is a sane coding standard, like MISRA is). If you are able to present a single non-deviable MISRA rule that hurts code quality and has been chosen for Zephyr community please present it, otherwise we risk to talk about gut feelings. Similarily if you are able to present a single deviable MISRA rule that in *any* instance hurts code quality/verificability/readability/understanding and/or has no point to exists please talk about that under a pure technical point of view. In summary, I believe that to keep repeating "XYZ conformance stinks" does not help. On Wed, 15 Dec 2021, Gerard Marull Paretas wrote:
A few points from my side (probably nothing that has not been already said)
- MISRA mostly tries to avoid undefined behavior derived issues. Other
languages such as Rust have done a better job on this topic from the very
beginning.
- Not all rules are mandatory to make a codebase MISRA compliant, so a
project can decide to deviate where it makes sense.
- If seeking for MISRA compliance, the coding style used by a project
needs to introduce certain rules (e.g. if always needs to use braces). From
my perspective this is not a problem, I, personally, prefer consistent
codebases even if they have certain rules I'm not 100% happy with. If we
take for example the STM32 HAL (which is MISRA C 2012 compliant) one will
observe that code is much more consistent compared to Zephyr. Nowadays,
Zephyr just runs checkpatch, which loosely enforces Linux Kernel coding
style.
- Zephyr has the potential to target safety critical applications, so I
think it makes sense to certify core parts of the project. Without that,
there are markets that are simply out of the game. This is likely not a
concern for Linux.
Gerard
On Wed, Dec 15, 2021 at 2:35 PM Nashif, Anas <anas.nashif@...> wrote:
Olof,
Point taken, will start posting agenda the day before.
Anas
*From: *devel@... <devel@...> on
behalf of Olof Johansson <olof@...>
*Date: *Wednesday, December 15, 2021 at 2:14 AM
*To: *Hibberd, Amber M <amber.m.hibberd@...>
*Cc: *Abramo Bagnara <abramo.bagnara@...>, Benjamin Lindqvist <
benjamin.lindqvist@...>, Kevin Hilman <khilman@...>,
Pitre, Nicolas <npitre@...>, Zephyr Devel <
devel@...>
*Subject: *Re: [Zephyr-devel] MISRA
On Tue, Dec 14, 2021 at 16:42 Hibberd, Amber M <amber.m.hibberd@...>
wrote:
All,
1. As a reminder, the TSC agreed upon the Zephyr Project Coding
Guidelines
<https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html>.
We are working towards compliance to the Project CG. Yes, they are a subset
of Misra, but we are not going for Misra compliance.
2. The Safety Committee, in driving toward the Project goal of
certification-ready, has partnered with Bugseng, as part of the community,
to help build momentum towards this goal.
3. There is a process for removing, revising, having exceptions, etc to
the CG rules. If you think a rule should be changed, open an issue in
Github.
The Safety Committee is bringing an update to the TSC tomorrow, where we
can have a discussion around the CG compliance work.
It’d be great to have a pre-read to frame the discussion and help us can
come prepared with questions based on said update (the linked Nov 10 email
in last week’s minutes is fairly high level).
Unfortunately, TSC agendas don’t seem to be communicated in a timely
fashion any more, usually sent out mere hours before the meeting (which is
early morning for us on the US west coast).
It makes it impossible to tell if it’s worth attending calls or not
beforehand, and I normally skip those kind of meetings to reduce meeting
overhead.
I hope we can change this to be a smoother process in the future and
optimize the value of a one hour weekly meeting.
-Olof
-Amber
*From:* devel@... <devel@...> *On
Behalf Of *Benjamin Lindqvist
*Sent:* Monday, December 13, 2021 2:44 PM
*To:* Abramo Bagnara <abramo.bagnara@...>
*Cc:* Pitre, Nicolas <npitre@...>; Zephyr Devel <
devel@...>; Kevin Hilman <khilman@...>
*Subject:* Re: [Zephyr-devel] MISRA
Well if code quality is truly the only goal of the steering committee and
there's no politics involved then I retract everything I said. Misra offers
less than nothing for this project. But I don't believe that's true. This
is 100% politics if you ask me.
Also before declaring the virtues of Misra, you'll be sure to clarify any
vested financial interest you might have in the matter, right? You know,
just to make sure you're not shilling.
On Mon, Dec 13, 2021, 9:38 PM Abramo Bagnara <abramo.bagnara@...>
wrote:
Il 13/12/21 04:37, Nicolas Pitre ha scritto:
I've also seen too many times a bug being introduced because the code
was modified just to make it [name your favorite static analysis tool /
coding standard here]-compliant. It is often the case that the code is
questionnably written in the first place, and adding typecasts to it
makes it worse even if the scanning tool is then happy. I'd like make clear once and for all that the happiness of scanning tool
is *never* a valuable purpose under a MISRA perspective.
What matter is *only* the quality of code and MISRA is a tool to achieve
this purpose.
Il 13/12/21 04:37, Benjamin Lindqvist ha scritto:
Hopefully most people in the community agree that many, if not most, > Misra rules are outdated or even slightly harmful. But you optimize
> with the constraint being the way the world works, not how it should
> ideally work. If this is what it takes to get zephyr backed by Daimler
> and Volvo, I for one can't blame the steering committee for thinking
> the tradeoff is justified. I'd do the same thing probably, despite
> loathing Misra with all my heart.
In my experience this is a consequence of a misunderstanding of what
MISRA really is.
MISRA process has rules, but also deviations and permits and the only
sane way to use it is as a tool to improve
safety/readability/understanding/analyzability.
Please forget every other idea of it as an enemy to beat or to surrender
to.
As Nicolas has already done you are welcome to point out *any* proposed
change related to MISRA compliance that you think will make code worse
together with an alternative proposal to improve code at the same time.
I hope this will clarify that everyone in the community has the same
code improvement goal and, as usual, constructive collaboration is the key.
--
Abramo Bagnara
BUGSENG srl - http://bugseng.com
mailto:abramo.bagnara@...
--
*Gerard Marull-Paretas*
Teslabs Engineering S.L.
teslabs.com T. +34 622 321 312
CONFIDENTIALITY NOTICE:
The contents of this email message and any attachments are intended solely
for the addressee(s)
and may contain confidential and/or privileged information and may be
legally protected from
disclosure. If you are not the intended recipient of this message or their
agent, or if this message
has been addressed to you in error, please immediately alert the sender by
reply email and then
delete this message and any attachments. If you are not the intended
recipient, you are hereby
notified that any use, dissemination, copying, or storage of this message
or its attachments is
strictly prohibited.
-- Abramo Bagnara BUGSENG srl - http://bugseng.commailto:abramo.bagnara@...
|
|
|
