|
Re: MISRA
Nicolas Pitre <nico@...>
On Mon, 20 Dec 2021, Abramo Bagnara wrote:
Il 20/12/21 15:45, Nicolas Pitre ha scritto:volatile long long_max = LONG_MAX;PS: You should have a look at tests/lib/c_lib/src/main.c. Lots ofPerhaps you should be more specific pointing to a test that verifies that volatile long long_one = 1L; void test_limits(void) { zassert_true((long_max + long_one == LONG_MIN), NULL); } It is an undefined behavior per one of your previous examples. But it passes here because variables are volatile. I still challenge you to point at some gcc extension documentation mentioning that the use of volatile makes two's complement wrap the expected behavior. More variations on that theme to be found in that file. As the technical arguments seems to not have effects, [...]My technical arguments don't appear to have any effect on you indeed. So let's agree to disagree. As far as I'm concerned I'm done. Nicolas
|
|
|
|
Re: MISRA
Abramo Bagnara
Il 20/12/21 15:45, Nicolas Pitre ha scritto:
On Mon, 20 Dec 2021, Abramo Bagnara wrote:No, I simply keep stating the obvious: you can test a finite number of instances but not all of them and a compiler is free to bite you where you don't expect that (just like you are free to infringe any contract with your compiler).Il 19/12/21 23:36, Nicolas Pitre ha scritto:For the n-th time I'll repeat this: it can be tested.all compilers capable of compiling Zephyr I have access to do produceNo, it is not forbidden, it is in the domain "you can do what you want, but be PS: You should have a look at tests/lib/c_lib/src/main.c. Lots ofPerhaps you should be more specific pointing to a test that verifies that something expected happens for a behavior that is undefined for the C standard and is not a documented compiler extension. As the technical arguments seems to not have effects, please imagine to sell the following as a safe state-of-the-art piece of software: abramo@igor:/tmp$ cat z.c #include <stddef.h> int main() { int *x = NULL; int *y = x + 4; return y - x; } abramo@igor:/tmp$ clang z.c -fsanitize=undefined abramo@igor:/tmp$ ./a.out z.c:5:14: runtime error: applying non-zero offset 16 to null pointer SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior z.c:5:14 in I confess that I would be embarassed by that, but I'm sure many person are better seller than me ;-) -- Abramo Bagnara BUGSENG srl - http://bugseng.com mailto:abramo.bagnara@...
|
|
|
|
Re: MISRA
Nicolas Pitre <nico@...>
On Mon, 20 Dec 2021, Abramo Bagnara wrote:
Il 19/12/21 23:36, Nicolas Pitre ha scritto:For the n-th time I'll repeat this: it can be tested.all compilers capable of compiling Zephyr I have access to do produceNo, it is not forbidden, it is in the domain "you can do what you want, but be In this particular case it is extensively tested by the CI infrastructure. In the unlikely event one compiler implementation decides to exercise its freedom to change the existing behavior then we'll know about it right away. But you keep on dismissing this fact. PS: You should have a look at tests/lib/c_lib/src/main.c. Lots of "undefined" stuff being tested in there, including not very useful stuff. Nicolas
|
|
|
|
Re: MISRA
Abramo Bagnara
Il 19/12/21 23:36, Nicolas Pitre ha scritto:
On Sun, 19 Dec 2021, Abramo Bagnara wrote:The law enforcement analogy is completely misdriven: our case is not related to your freedom to do something (I assure you that you are perfectly free to introduce undefined behaviors in your code).Il 19/12/21 02:43, Nicolas Pitre ha scritto:No. I disagree to such dogmatic stance.On Fri, 17 Dec 2021, Abramo Bagnara wrote:As long as there is still an undefined behavior in the code, I'd like youIn the specific of your PR: have you considered that if, instead to keepWell... I took this as a challenge and attempted it. OTOH it is related to *compiler* freedom to do things that you don't expect for your code but that they can do. Are you the kind of person to never drive faster than 70km/h on theIn this case they would have added a documented extension, if they haven't you can deduce they don't want to, i.e. they are free to do whatever with it. Let me make a real life example (without any horses): gcc manual says that they allow as an extension zero-length arrays in *C* (that in zephyr are used), but I have tried and they don't give any errors also in C++. But then I try to run this program: #include <cstdio> struct S { S() { puts(__FUNCTION__); } ~S() { puts(__FUNCTION__); } int empty[0]; }; int main() { S vals[4]; } And I don't get the expected S S S S ~S ~S ~S ~S oops... they told to me they were free to do what they want... why I've not listened? (I don't want to ruin the suspence telling how clang and gcc behaves) But let's have a look at the most influential and prevalent Open SourceIf someone has no alternatives is forced to extreme measures, this is a fact of life. But as linux kernel AFAIK can be compiled only by gcc and clang I suspect this is dead code. So far so good. Yet we can find these:Please don't confuse unevaluated or compile-time only constructs with code generation, they don't play the same game and they don't have the same risks. all compilers capable of compiling Zephyr I have access to do produceNo, it is not forbidden, it is in the domain "you can do what you want, but be prepared to pay the price" and the price is unspecified. The fact that such an operation remains "undefined" by the standard isI've not said it is related only to the standards, standards can be extended by implementations. I've said that it is undefined by the standard *and* it is not a compiler extension, then it is a no no. antique CPU architectures where a NULL didn't match the value 0 but someThe fact that ABI represents NULL with zero is completely not relevant: in all current architectures the signed integer representation is 2-complement, but still the C arithmetic is undefined for sum overflow. I also understand that you have a task to accomplish and it is evaluatedMy day job is completely not relevant here and I invite you to not make this personal: -#define BUF_OFFSET (size_t)(buf - buf0)To say that many of proposed changes are aimed to fool the comformance tool is rather offensive, don't you think? Would that make your life easier?The easyness of my life is far out of scope. Back to technical: No, the conversion to uintptr_t is not enough to fix that, as in many place the code author relies on undefined NULL arithmetic. As I said one of the possible solutions is to use a buffer offset that will be applied only when buffer is not NULL. However it is definitely not my role to say what should be accepted, this is a job for official reviewers and, I guess, steering commitee for important cases. Said that, personally I think that the attitude toward safety and missing standard/compiler warranties represented in this thread has space for improvement, but I'm sure it will improve as reciprocal trust and awareness will increase. -- Abramo Bagnara BUGSENG srl - http://bugseng.com mailto:abramo.bagnara@...
|
|
|
|
Re: MISRA
Nicolas Pitre <nico@...>
On Sun, 19 Dec 2021, Abramo Bagnara wrote:
Il 19/12/21 02:43, Nicolas Pitre ha scritto:No. I disagree to such dogmatic stance.On Fri, 17 Dec 2021, Abramo Bagnara wrote:As long as there is still an undefined behavior in the code, I'd like youIn the specific of your PR: have you considered that if, instead to keepWell... I took this as a challenge and attempted it. But let's start with a few facts of life. Did you know it is illegal to change a light bulb yourself if you live in Victoria, Australia? Did you know that the Queen Elizabeth hotel in Montreal is required by law to provide a stall and food to its clients' horses? Do you really think those laws are actively enforced? Are you the kind of person to never drive faster than 70km/h on the highway because that's the posted speed limit and therefore the law despite surrounding traffic going at 115km/h? Obviously it is safer to drive at 70km/h in theory, but in practice, some circumstances would make it a safety hazard. The easy resume is that you are stating that pointer arithmetic with NULLYes. And let me precise: with a typed pointer. I fully agree that: long foo(int *x) { return x - NULL; } is undefined and without sensible semantic. But, on the other hand: long foo(int *x) { int *y = NULL; return x - y; } is totally sensible. You give proofs of that showing generated assemblers of pointer differencesI disagree: gcc authors are in a perfect position to know and ensure for themselves that using a NULL pointer is fine. But they certainly could have provided a wrapper just to give the C-standard orthodoxy some peace of mind. But let's have a look at the most influential and prevalent Open Source project out there for a minute. Linux does this: #undef offsetof #ifdef __compiler_offsetof #define offsetof(TYPE, MEMBER) __compiler_offsetof(TYPE, MEMBER) #else #define offsetof(TYPE, MEMBER) ((size_t)&((TYPE *)0)->MEMBER) #endif And elsewhere: #define __compiler_offsetof(a, b) __builtin_offsetof(a, b) So far so good. Yet we can find these: #define typeof_member(T, m) typeof(((T*)0)->m) #define container_of(ptr, type, member) ({ \ void *__mptr = (void *)(ptr); \ static_assert(__same_type(*(ptr), ((type *)0)->member) || \ __same_type(*(ptr), void), \ "pointer type mismatch in container_of()"); \ ((type *)(__mptr - offsetof(type, member))); }) Isn't it ironic that the extra statement used to improve code correctness is itself relying on an "undefined" behavior? But suppose you still don't trust me and then try to reason on similar cases.Perfect! And it makes sense because producing a different result would happen only 0.00000002% of the time. Making an addition beyond the type range "undefined" by the standard allows for compiler implementers to perform optimizations that wouldn't be possible otherwise due to that 0.00000002% where the optimization would be wrong if we take into account the arguably ambiguous wrapping nature of a two's complement representation in that context. Yet, being "undefined" doesn't mean that another compiler implementation is forbidden to do the full operation anyway and return true if provided with INT_MAX. Granted, that would be semantically illogical. And it can be tested in practice. Whereas, with: long foo(int *x) { int *y = NULL; return x - y; } all compilers capable of compiling Zephyr I have access to do produce the right result, *all* the time. It is true whether or not the pointer being NULL is known at compile time (when it is the compiler also does constant propagation optimizations on it). It is not because something is "undefined" that it is necessarily "forbidden". And it also can be tested in practice. The fact that such an operation remains "undefined" by the standard is analogous to requiring hotels to stock hay. Maybe it made sense on antique CPU architectures where a NULL didn't match the value 0 but some special magic value the hardware would trigger a fault with. But I challenge you to find an architecture whose ABI doesn't define NULL as zero today. Saying that "it is impossible to be sure if not sanctioned by the standard" is a strawman argument like driving 70km/h on the highway. In practice it doesn't make sense not to support it on contemporary CPU architectures irrespective of the C standard, and compiler implementers already did what's semantically logical with it. There are simply no confusion possible when the pointer is typed, and it doesn't come in the way of any other optimizations. It is also impossible to use NULL today in ways that could behave unexpectedly. As an example of that last point, let's consider this again: #include <stddef.h> long foo(int *x) { return x - NULL; } The result is: In function ‘foo’: 2:29: error: invalid operands to binary - (have ‘int *’ and ‘void *’) In that case the operation is clearly without proper semantic because the NULL constant isn't typed, and therefore the concept of object pointer difference cannot apply. Now... after all is said... I also understand that you have a task to accomplish and it is evaluated in terms of standard violation occurrence reduction. I'm therefore proposing this change on top of my latest patch (yes, I'm adding more casts): -#define BUF_OFFSET (size_t)(buf - buf0) +#define BUF_OFFSET ((uintptr_t)buf - (uintptr_t)buf0) This should fool the conformance scanning tool just as well as many of the other conformance changes being proposed. Would that make your life easier? Nicolas
|
|
|
|
Re: MISRA
Abramo Bagnara
Il 19/12/21 02:43, Nicolas Pitre ha scritto:
On Fri, 17 Dec 2021, Abramo Bagnara wrote:As long as there is still an undefined behavior in the code, I'd like you resign yourself to the fact that this cannot be accepted.In the specific of your PR: have you considered that if, instead to keepWell... I took this as a challenge and attempted it. Let me try another time to explain, I hope to not make you bored and perhaps you can see that under a different perspective, hopefully useful. The easy resume is that you are stating that pointer arithmetic with NULL pointer, despite being declared undefined behavior in all C standards and despite not being a documented GCC extension is safe. You give proofs of that showing generated assemblers of pointer differences and showing this has been translated as you expect. You seem to have forgot that in some obsolete compilers the offsetof macro was implemented in this way and that this has been changed ages ago due to such undefined behavior (GCC has __builtin_offsetof exactly for this reason). This is a sensible proof that also GCC authors does not rely on the fact that code generation would have worked correctly to implement such macro using null pointer arithmetic. But suppose you still don't trust me and then try to reason on similar cases. Let consider signed addition, as an another example of possible source of undefined behavior. If you write: int add(int x, int y) { return x + y; } and check the generated code in almost any contemporary target you will see that it does wrapping. Following the same reasoning you do above we might deduce that it is harmless and I can count on wrapping signed arithmetic. But let try something different: int is_max_int(int x) { return x + 1 < x; } If signed arithmetic does wrapping on this compiler (as I've "proved" with the test above) then this implementation shall do what I expect, i.e. return truish for the maximum representable int. But if you take a look to generated code you will see that it always return 0. So we are forced to deduce that either the compiler is buggy or the proof is wrong. I can confirm that the compiler do what the language standard require and that for both undefined behaviors the proof is wrong and despite how hard you try you *cannot* write a test (or a thousand tests) to prove that an undefined behavior behave *always* as you expect (unless it is a documented compiler extension and then it is no longer an undefined behavior). I hope this is useful for you or others in this list. -- Abramo Bagnara BUGSENG srl - http://bugseng.com mailto:abramo.bagnara@...
|
|
|
|
Re: MISRA
Nicolas Pitre
On Sat, 18 Dec 2021, Abramo Bagnara wrote:
Il 17/12/21 23:21, Nicolas Pitre ha scritto:Is that the case? If so I missed it.It does not always have to be 0 means success and <0 means error,IOW you are proposing to do exactly what I presented the first time, but One thing that could help you get proper reviews is to split your PR in smaller chunks. It sounds great, but wasn't better then to accept my original changes and openYour original change is way too big. Maybe that particular part was OK, but it was lumped with many other things and this is not conducive to quality reviews. In particular, you should split more or less along different themes: * a PR for boolean: - use bool when the data nature is Boolean - use explicit comparison with 0 or NULL * another PR for enums: - avoid mixing enumerations with integers Etc. This way, the uncontrovertial parts can be reviewed and merged quickly, while the other parts can be judged on their own merit separately. In particular, I think that the following are pointless and harmful to code clarity: - added U suffix to unsigned constants - avoid mixing signed and unsigned integers in computations and comparisons (should be OK if mixedness comes from unqualified iterals) - avoid mixing characters with integers; And again, any added cast to oobtain compliance is harmful to code quality and must be avoided if alternatives are possible. It seems obvious to me that a change aimed to improve code quality (in thisWell... this is a mess! Even the comment for Z_SYSCALL_VERIFY_MSG() is garbage. It says that an oops is triggered if the tested expression is false which is wrong as there is no oops triggering at all in the current code. Bad bad bad. Nicolas
|
|
|
|
Re: MISRA
Nicolas Pitre <nico@...>
On Fri, 17 Dec 2021, Abramo Bagnara wrote:
In the specific of your PR: have you considered that if, instead to keepWell... I took this as a challenge and attempted it. And it didn't work satisfactorily. The problem is that the buf pointer accomplishes two roles: it serves as a buffer offset as well as a memory boundary indicator used for argument alignment. Going with a buffer base and an explicit offset doesn't allow for proper alignment without also taking the buffer base into account. If the buffer base is NULL then you need yet another variable to hold the misalignment. This complexifies the code, increases register pressure, makes resulting binary bigger, and doesn't look any better in the end. So my efforts resulted in the following PR instead which I hope would be an acceptable middle ground. https://github.com/zephyrproject-rtos/zephyr/pull/41321 Nicolas
|
|
|
|
Re: MISRA
Abramo Bagnara
Il 17/12/21 23:21, Nicolas Pitre ha scritto:
On Fri, 17 Dec 2021, Abramo Bagnara wrote:IOW you are proposing to do exactly what I presented the first time, but inverting the boolean logic from original true for failure and false for success to false for failure and true for success.Let resume state after commit with the points still to be decided, so we canIt should, especially if the calling code already uses != 0 or == 0. It sounds great, but wasn't better then to accept my original changes and open an issue for reverting current logic? It seems obvious to me that a change aimed to improve code quality (in this case simply avoiding integer and boolean mixing) that changes semantic of functions would have had *zero* chances to be accepted, am I missing something? -- Abramo Bagnara BUGSENG srl - http://bugseng.com mailto:abramo.bagnara@...
|
|
|
|
Re: MISRA
Abramo Bagnara
Il 17/12/21 19:39, Nicolas Pitre ha scritto:
I'm truly sad saying that, but if you keep stating that to write code that has undefined behavior for C standard and such behavior is not a documented compiler extensions is a sane thing to do, I find very hard to find a common ground.Also in some points I've added /*? shaped comments to points out that the codePlease review https://github.com/zephyrproject-rtos/zephyr/pull/41090 I confess that this is very embarassing for me and I would have hoped to be able to find the right words to explain why this is not sane at all. It is clear now I've failed and I'm the one to blame for that. I hope that someone else will succeed where I have failed. In the specific of your PR: have you considered that if, instead to keep increasing buf, you increase a buffer offset you would avoid to do ugly tricks and NULL subtraction? -- Abramo Bagnara BUGSENG srl - http://bugseng.com mailto:abramo.bagnara@...
|
|
|
|
Re: MISRA
Nicolas Pitre
On Fri, 17 Dec 2021, Abramo Bagnara wrote:
Let resume state after commit with the points still to be decided, so we canIt should, especially if the calling code already uses != 0 or == 0. Success with > 0 is not that frequent. But that's for functions that perform a substantial action. It would be overkill for merely testing a condition. - Z_OOPS takes an error (defined above)It should rather be: Z_OOPS() takes an bool argument indicating whether it should oops the system. - Z_SYSCALL_VERIFY_MSG takes a boolean and return an errorIt may still return a boolean. This is a validation macro. The provided expression is either true or false. It should itself also return true for success or false for error. But it currently reads as: #define Z_SYSCALL_VERIFY_MSG(expr, fmt, ...) ({ \ bool expr_copy = !(expr); \ if (expr_copy) { \ LOG_MODULE_DECLARE(os, CONFIG_KERNEL_LOG_LEVEL); \ LOG_ERR("syscall %s failed check: " fmt, \ __func__, ##__VA_ARGS__); \ } \ expr_copy; }) Here expr_copy is a really really bad name. This is _not_ a copy of anything. This is a variable carrying the result of the expression, and even the inverted result! So, it would have been much better if it had been written as: * @return true on success, false on failure */ #define Z_SYSCALL_VERIFY_MSG(expr, fmt, ...) ({ \ bool is_ok = (expr); \ if (!is_ok) { \ LOG_MODULE_DECLARE(os, CONFIG_KERNEL_LOG_LEVEL); \ LOG_ERR("syscall %s failed check: " fmt, \ __func__, ##__VA_ARGS__); \ } \ is_ok; }) That is much more self explanatory. Then you can do constructs like: Z_OOPS(!Z_SYSCALL_VERIFY_MSG(expression, "expression failed")); The above can naturally be interpreted as: "if the expression does not verify then we oops." Same thing for the other derrivatives. It does not always have to be 0 means success and <0 means error, especially in this case where Z_SYSCALL_VERIFY_MSG() has a clear semantic i.e. it is mainly testing a condition and does not perform any substantial operation (the log is merely a side effect). Nicolas
|
|
|
|
Re: MISRA
Nicolas Pitre <nico@...>
On Fri, 17 Dec 2021, Abramo Bagnara wrote:
Il 17/12/21 09:37, Carlo Caione ha scritto:18K violations and still it works.After taking a look atLet me talk clear: in the portion of Zephyr currently checked we have more There are so many of those occurrences that move back and forth between int and char. Especially in string formatting for numerical value. And in those cases, the code is perfectly fine as it is. The range of possible values is well controlled and well known (like between '0' and '9', sometimes 'A' and 'f' or their lowercase equivalent). There is even an extensive test suite for that code already. So highlighting non-preserving implicit casts with code-cluttering explicit casts is really pointless. Nobody should care because it is never going to be an issue. And in such cases, it is the scanning tool being ultra eager to highlight everything that is the issue, not the code. Instead of trying to fix all 18K violations, I'd suggest you improve the tool so that it can skip over the code that is known to be fine despite the violation. It could be some kind of markup in a comment that would indicate that, e.g. the whole of lib/os/cbprintf_nano.c is just fine and violations x and y are acceptable. My PR has shown some important points where implicit casts happens and thatMISRA compliance has nothing to do with readability. Or, at least, what has been proposed so far to increase MISRA compliance did *decrease* code readability. Source code should cater to humans first. The compiler doesn't care how obfuscated the code is. Humans do care. So any coding standard must be there first and foremost to ease the mental load on humans for understanding the code. Readability is helped a lot when the code is _simplified_. Such simplification comes about with things like good abstractions to encapsulate tricky parts, but implicit conversions also play a part in readability. Littering the code with casts, or even excessive comments for trivial things, may become a distraction and code reviewers might spend less time on understanding the actual logic of the code. For people to do a good job of ensuring code quality, especially "community" people who most often do it on their own will, the code must look nice and be enjoyable to read. Yes I know... there are people for whom coding is just a day job and they don't understand what all this fuss is all about. But many so called "community" people do it as a hobby because they truly enjoy writing or even just reading nice code for some artistic appreciation. Those are the people who will read existing code for the pleasure of it and whom you're relying upon to report/fix flaws that remained unnoticed and impossible to find with scan tools (one of the major advantage of fostering a community around a project). Driving those people away because the code is no longer attractive is detrimental to the project and to everyone. My request to code owners/PR reviewers is to question such points and use thisIf you want to highlight a problem, you should do so with something else than a cast. When using a cast, you legitimize an unorthodox code sequence which is the opposite of drawing attention to a problem. There should also be a third option: - The code is verifiably just fine and should be left alone, possibly with a machine-understandable tag at the top of the file to say so. Also in some points I've added /*? shaped comments to points out that the codePlease review https://github.com/zephyrproject-rtos/zephyr/pull/41090 IMHO the point to understand is that this is a path to improvement: the aim isAdded casts will never improve safety. It will only silence your scan tool and give you a false sense of safety. The fundamental code flaw, if real, will still be there once you put typecasts on it. Having a tool that lists 18k violations is not very useful either if the bulk of them are harmless and silencing them is harmful to code readability. This instead impacts the credibility of the whole exercise. If there were that many real bugs then the project would be so unreliable that nobody would use it, which is obviously not the case. If you want to make the tool happy then it should be done at zero cost to readability: either by fixing the broken code, by reworking the code in some ways that avoids triggering the tool counter, or hiding away the uglification behind some macro, in that preference order. But adding more casts is not the way. Nicolas
|
|
|
|
Re: MISRA
David Leach
+1 on this response as it should be the first thing we attempt to do!
toggle quoted messageShow quoted text
In my past, my first instinct when getting these types of static analysis issues was to look at the underlying code to determine if there is a valid reason for it or if it is a bug. Using explicit types to begin with helps avoid a lot of this. It would be prudent for the reviewers of these PRs to also look at the underlying code and decisions being made in the storage types to ensure that they make sense and are proper because, as Piotr is pointing out, the true fix more likely lays in the code behind this particular static analysis warning. David
-----Original Message-----
From: devel@... <devel@...> On Behalf Of Piotr Pryga via lists.zephyrproject.org Sent: Friday, December 17, 2021 4:20 AM To: carlo.caione@...; Nicolas Pitre <nico@...>; Gerard Marull Paretas <gerard@...> Cc: Nashif, Anas <anas.nashif@...>; Olof Johansson <olof@...>; Zephyr Devel <devel@...> Subject: Re: [Zephyr-devel] MISRA After taking a look at The code is now basically a nightmare of explicit typecasts everywhere, and honestly it looks like a nightmare to maintain now.If I may add my 2 cents here... The code looks ugly, but isn't it because the rules were violate in first place and here is fastest (not prettiest) way to make it compliant? Wouldn't it look good if the types were selected and maintained to avoid casting? One of the points of MISRA is to make code explicit. If there is no other way than to use cast, then MISRA enforces explicit casting, to make sure an author is aware of what a code does. In safety critical applications this is mandatory (at least in my humble opinion). Do not look into past and ugly looking code after fixing compliance issues. Look into future, if there is a way to code to avoid violation of those rules. It seems to me that it could be achieved.... PIOTR PRYGA | Senior Software Engineer M +48 507 154 312 | Krakow, Poland nordicsemi.com | devzone.nordicsemi.com My question is: is this some kind of exercise that must be done from scratch every release? Because I really doubt that all the PRs in the future can be fully compliant to this typecast madness.Also I understand that the casting is silencing the compiler (and some tool I guess) but has anyone checked whether the code needs some change instead or maybe some boundary / error check? Because typecasting everywhere without investigating why and if some actual code change is needed is just a futile exercise IMHO. My 2 cents, Ciao! PIOTR PRYGA | Senior Software Engineer M +48 507 154 312 | Krakow, Poland nordicsemi.com | devzone.nordicsemi.com -----Original Message----- From: devel@... <devel@...> On Behalf Of Carlo Caione via lists.zephyrproject.org Sent: piątek, 17 grudnia 2021 09:38 To: Nicolas Pitre <nico@...>; Gerard Marull Paretas <gerard@...> Cc: Nashif, Anas <anas.nashif@...>; Olof Johansson <olof@...>; Zephyr Devel <devel@...> Subject: Re: [Zephyr-devel] MISRA On 15/12/2021 19:50, Nicolas Pitre wrote: I persist in insisting that adding any more typecasts to a codebaseAfter taking a look at https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fzephyrproject-rtos%2Fzephyr%2Fpull%2F41227&;data=04%7C01%7Cdavid.leach%40nxp.com%7Cadd08b02f1004139185c08d9c14f589f%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637753368801970983%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=n%2Fm%2FdF2vW1UZigClSbPUXTZ0znJhgHpa53nCE2DH5JA%3D&reserved=0 I fully agree with Nicolas and honestly I hate this more than I could have possibly imagined before. The code is now basically a nightmare of explicit typecasts everywhere, and honestly it looks like a nightmare to maintain now. My question is: is this some kind of exercise that must be done from scratch every release? Because I really doubt that all the PRs in the future can be fully compliant to this typecast madness. Also I understand that the casting is silencing the compiler (and some tool I guess) but has anyone checked whether the code needs some change instead or maybe some boundary / error check? Because typecasting everywhere without investigating why and if some actual code change is needed is just a futile exercise IMHO. My 2 cents, Ciao! -- Carlo Caione
|
|
|
|
Re: MISRA
Piotr Pryga
After taking a look at The code is now basically a nightmare of explicit typecasts everywhere, and honestly it looks like a nightmare to maintain now.If I may add my 2 cents here... The code looks ugly, but isn't it because the rules were violate in first place and here is fastest (not prettiest) way to make it compliant? Wouldn't it look good if the types were selected and maintained to avoid casting? One of the points of MISRA is to make code explicit. If there is no other way than to use cast, then MISRA enforces explicit casting, to make sure an author is aware of what a code does. In safety critical applications this is mandatory (at least in my humble opinion). Do not look into past and ugly looking code after fixing compliance issues. Look into future, if there is a way to code to avoid violation of those rules. It seems to me that it could be achieved.... PIOTR PRYGA | Senior Software Engineer M +48 507 154 312 | Krakow, Poland nordicsemi.com | devzone.nordicsemi.com My question is: is this some kind of exercise that must be done from scratch every release? Because I really doubt that all the PRs in the future can be fully compliant to this typecast madness.Also I understand that the casting is silencing the compiler (and some tool I guess) but has anyone checked whether the code needs some change instead or maybe some boundary / error check? Because typecasting everywhere without investigating why and if some actual code change is needed is just a futile exercise IMHO. My 2 cents, Ciao! PIOTR PRYGA | Senior Software Engineer M +48 507 154 312 | Krakow, Poland nordicsemi.com | devzone.nordicsemi.com -----Original Message----- From: devel@... <devel@...> On Behalf Of Carlo Caione via lists.zephyrproject.org Sent: piątek, 17 grudnia 2021 09:38 To: Nicolas Pitre <nico@...>; Gerard Marull Paretas <gerard@...> Cc: Nashif, Anas <anas.nashif@...>; Olof Johansson <olof@...>; Zephyr Devel <devel@...> Subject: Re: [Zephyr-devel] MISRA On 15/12/2021 19:50, Nicolas Pitre wrote: I persist in insisting that adding any more typecasts to a codebaseAfter taking a look at https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fzephyrproject-rtos%2Fzephyr%2Fpull%2F41227&;data=04%7C01%7Cpiotr.pryga%40nordicsemi.no%7C7ac9d594992e49cf0f5708d9c138a061%7C28e5afa2bf6f419a8cf6b31c6e9e5e8d%7C0%7C0%7C637753272012682798%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=AJcwJotkjxMEWS0bYBIeFvwrlzOvYCS%2B0nfT1TlwbfI%3D&reserved=0 I fully agree with Nicolas and honestly I hate this more than I could have possibly imagined before. The code is now basically a nightmare of explicit typecasts everywhere, and honestly it looks like a nightmare to maintain now. My question is: is this some kind of exercise that must be done from scratch every release? Because I really doubt that all the PRs in the future can be fully compliant to this typecast madness. Also I understand that the casting is silencing the compiler (and some tool I guess) but has anyone checked whether the code needs some change instead or maybe some boundary / error check? Because typecasting everywhere without investigating why and if some actual code change is needed is just a futile exercise IMHO. My 2 cents, Ciao! -- Carlo Caione
|
|
|
|
Re: MISRA
Abramo Bagnara
Il 17/12/21 09:37, Carlo Caione ha scritto:
On 15/12/2021 19:50, Nicolas Pitre wrote:Let me talk clear: in the portion of Zephyr currently checked we have more than 18K violations of what I think is one of the most valuable rule of BARR C that asks that any non obviously value-preserving cast is accompanied by a comment that clarify why such value change does not happen or why such value change is deliberate.I persist in insisting that adding any more typecasts to a codebaseAfter taking a look at https://github.com/zephyrproject-rtos/zephyr/pull/41227 I fully agree with Nicolas and honestly I hate this more than I could have possibly imagined before. My PR has shown some important points where implicit casts happens and that need to be explicited to improve readability/understanding of code so to improve code quality *and* to increase MISRA compliance. My request to code owners/PR reviewers is to question such points and use this info for one of: - (possibly implicit) confirmation that the cast (now observable) is ok (either because it is value preserving or because the value change is deliberate). If the code owner is willing to add or suggest a comment it is welcome. - use the added cast as a way to understand that code has *indeed* an existing problem that should be fixed and that this PR fortunately has made evident. Also in some points I've added /*? shaped comments to points out that the code sounds very suspect to my eyes (possibly simply because I have not realized the intention). IMHO the point to understand is that this is a path to improvement: the aim is to reduce the number of points where unwanted value change happens, for code safety sake. -- Abramo Bagnara BUGSENG srl - http://bugseng.com mailto:abramo.bagnara@...
|
|
|
|
Re: MISRA
Carlo Caione
On 15/12/2021 19:50, Nicolas Pitre wrote:
I persist in insisting that adding any more typecasts to a codebaseAfter taking a look at https://github.com/zephyrproject-rtos/zephyr/pull/41227 I fully agree with Nicolas and honestly I hate this more than I could have possibly imagined before. The code is now basically a nightmare of explicit typecasts everywhere, and honestly it looks like a nightmare to maintain now. My question is: is this some kind of exercise that must be done from scratch every release? Because I really doubt that all the PRs in the future can be fully compliant to this typecast madness. Also I understand that the casting is silencing the compiler (and some tool I guess) but has anyone checked whether the code needs some change instead or maybe some boundary / error check? Because typecasting everywhere without investigating why and if some actual code change is needed is just a futile exercise IMHO. My 2 cents, Ciao! -- Carlo Caione
|
|
|
|
Re: MISRA
Abramo Bagnara
Il 17/12/21 04:06, Nicolas Pitre ha scritto:
On Thu, 16 Dec 2021, Abramo Bagnara wrote:Let resume state after commit with the points still to be decided, so we can be more specific and finalize:Wow! OK... I see that this fu^H^Hmixed-up convention already exists inI'd like very much you take a look toAnd a real kicker is this change: - 0 is success, non-zero is error: the comunity agrees to change to have = 0 success, < 0 is error? Is it ok to replace current 1 with -EINVAL?- Z_OOPS takes an error (defined above) - Z_SYSCALL_VERIFY_MSG takes a boolean and return an error - Z_SYSCALL_VERIFY is a frontend for Z_SYSCALL_VERIFY_MSG - Z_SYSCALL_MEMORY returns the result of Z_SYSCALL_VERIFY_MSG - Z_SYSCALL_MEMORY_READ and Z_SYSCALL_MEMORY_WRITE are frontends of Z_SYSCALL_MEMORY - Z_SYSCALL_MEMORY_ARRAY combine Z_SYSCALL_VERIFY_MSG and Z_SYSCALL_MEMORY - Z_SYSCALL_MEMORY_ARRAY_READ/Z_SYSCALL_MEMORY_ARRAY_WRITE are fronted of Z_SYSCALL_MEMORY_ARRAY - Z_SYSCALL_DRIVER_OP returns the result of Z_SYSCALL_VERIFY_MSG - Z_SYSCALL_SPECIFIC_DRIVER combine Z_SYSCALL_OBJ and Z_SYSCALL_VERIFY_MSG - Z_SYSCALL_IS_OBJ returns the result of Z_SYSCALL_VERIFY_MSG: the comunity agrees that is misnamed? What is a better name? - Z_SYSCALL_OBJ, Z_SYSCALL_OBJ_INIT, Z_SYSCALL_OBJ_NEVER_INIT are frontends of Z_SYSCALL_IS_OBJ -- Abramo Bagnara BUGSENG srl - http://bugseng.com mailto:abramo.bagnara@...
|
|
|
|
Re: MISRA
Nicolas Pitre
On Thu, 16 Dec 2021, Abramo Bagnara wrote:
Wow! OK... I see that this fu^H^Hmixed-up convention already exists inI'd like very much you take a look toAnd a real kicker is this change: the tree today. You simply followed that precedent. [...] * @return False on success, True on failure */ #define Z_SYSCALL_VERIFY_MSG(expr, fmt, ...) ({ \ [...] The above is very wrong. It is completely counter-intuitive. But given its name, most people won't expect it to return a bool but rather a 0 for success, which makes the code work the same. Here's what I think the logic should be: - Boolean results should come with a function whose name clearly implies a condition. Example: k_is_user_context(), is_buf_set(), is_el2_sec_supported(), etc. - A function that accomplishes something and returns the outcome (success vs failure) should use 0 for success and a negative value for errors. Sometimes such functions may return different success results using positive values, hence the negative values for errors. So when it says: "returns 0 for success, non-zero for error", in fact it should rather be "0 for success, negative error code otherwise". Hence you should not return 1 for error but rather something like -EINVAL or any other error code that best fits the error condition. Most of the time the caller doesn't care about the actual error code but that still makes the callee more self-explanatory. Nicolas
|
|
|
|
Re: MISRA
Abramo Bagnara
Il 16/12/21 22:17, Nicolas Pitre ha scritto:
On Thu, 16 Dec 2021, Abramo Bagnara wrote:I think you have noted that at least another member has presented some objections.Il 13/12/21 04:37, Nicolas Pitre ha scritto:I think <sys/util.h> would be a good place.On Sun, 12 Dec 2021, Abramo Bagnara wrote:Unless we find a good place for this macro I'd be forced to leave double cast.Who in its right mind will believe that sprinkling typecasts aroundIn the specific, double casts are needed in two situations: Which strategies the comunity have adopted in similar cases to obtain a fast convergence and to avoid bikeshedding? You can find it as the last commit of branch MISRA_Essential_Types ofPlease make it into a fetchable branch. That'd make my reviewing easier.I'd like very much you take a look toAnd a real kicker is this change: https://github.com/Abramo-Bagnara/zephyr Here too it seems that someone prefers Boolean returning API and others 0 success/non-zero error returning API. The 0 success/non-zero error has the benefits to be able to return an informative error code, but currently this has not been used. The important thing is that we get a rapid convergence of opinions, then I can easily adapt the rest to it. The only thing I'd like it was not questioned is the need to separate for clarity and readability Boolean and integers. Integers are for arithmetic, Booleans are for logic and guards. -- Abramo Bagnara BUGSENG srl - http://bugseng.com mailto:abramo.bagnara@...
|
|
|
|
Re: MISRA
Nicolas Pitre
On Thu, 16 Dec 2021, Abramo Bagnara wrote:
Il 13/12/21 04:37, Nicolas Pitre ha scritto:I think <sys/util.h> would be a good place.On Sun, 12 Dec 2021, Abramo Bagnara wrote:Unless we find a good place for this macro I'd be forced to leave double cast.Who in its right mind will believe that sprinkling typecasts aroundIn the specific, double casts are needed in two situations: Please make it into a fetchable branch. That'd make my reviewing easier.I'd like very much you take a look toAnd a real kicker is this change: Nicolas
|
|
|