Date   

Re: MISRA

Nicolas Pitre <nico@...>
 

On Mon, 20 Dec 2021, Abramo Bagnara wrote:

Il 20/12/21 15:45, Nicolas Pitre ha scritto:
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.
Perhaps 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.
volatile long long_max = LONG_MAX;
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:

Il 19/12/21 23:36, Nicolas Pitre ha scritto:
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".
No, 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.
For the n-th time I'll repeat this: it can be tested.
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.
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).

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.
Perhaps 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:
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".
No, 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.
For the n-th time I'll repeat this: it can be tested.

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:

Il 19/12/21 02:43, Nicolas Pitre ha scritto:
On Fri, 17 Dec 2021, Abramo Bagnara wrote:

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?
Well... 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.
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.
No. I disagree to such dogmatic stance.
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?
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).

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 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 NULL
pointer, despite being declared undefined behavior in all C standards and
despite not being a documented GCC extension is safe.
Yes. 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 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.
I 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.
In 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 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)
If 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:
#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?
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 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".
No, 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 is
analogous to requiring hotels to stock hay. Maybe it made sense on
I'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 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.
The 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 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):
My day job is completely not relevant here and I invite you to not make this personal:

-#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.
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:
On Fri, 17 Dec 2021, Abramo Bagnara wrote:

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?
Well... 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.
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.
No. I disagree to such dogmatic stance.

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 NULL
pointer, despite being declared undefined behavior in all C standards and
despite not being a documented GCC extension is safe.
Yes. 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 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.
I 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.
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.
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:

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?
Well... 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.
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.

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:

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).
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.
Is that the case? If so I missed it.

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 open
an issue for reverting current logic?
Your 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 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?
Well... 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 keep
increasing buf, you increase a buffer offset you would avoid to do ugly tricks
and NULL subtraction?
Well... 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:

Let resume state after commit with the points still to be decided, so we can
be more specific and finalize:

- 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?
It 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 error
It 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).
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.

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:
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).
Please review https://github.com/zephyrproject-rtos/zephyr/pull/41090
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.

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 can
be more specific and finalize:

- 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?
It 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 error
It 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:
After 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.
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.
18K violations and still it works.

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 that
need to be explicited to improve readability/understanding of code so to
improve code quality *and* to increase MISRA compliance.
MISRA 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 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.
If 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 code
sounds very suspect to my eyes (possibly simply because I have not realized
the intention).
Please review https://github.com/zephyrproject-rtos/zephyr/pull/41090

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.
Added 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!

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
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&amp;sdata=n%2Fm%2FdF2vW1UZigClSbPUXTZ0znJhgHpa53nCE2DH5JA%3D&amp;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.
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 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.
After 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&amp;sdata=n%2Fm%2FdF2vW1UZigClSbPUXTZ0znJhgHpa53nCE2DH5JA%3D&amp;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
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&amp;sdata=AJcwJotkjxMEWS0bYBIeFvwrlzOvYCS%2B0nfT1TlwbfI%3D&amp;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.
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 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.
After 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&amp;sdata=AJcwJotkjxMEWS0bYBIeFvwrlzOvYCS%2B0nfT1TlwbfI%3D&amp;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:

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.
After 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.
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.

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 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.
After 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:

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.
I'd like very much you take a look to

https://github.com/Abramo-Bagnara/zephyr/commit/87a42d4185828fb1e651604b8ee878063fb6b08a

so to be sure I have intercepted correctly the community expectations.
Wow! OK... I see that this fu^H^Hmixed-up convention already exists in
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.
Let resume state after commit with the points still to be decided, so we can be more specific and finalize:

- 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:

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.
I'd like very much you take a look to

https://github.com/Abramo-Bagnara/zephyr/commit/87a42d4185828fb1e651604b8ee878063fb6b08a

so to be sure I have intercepted correctly the community expectations.
Wow! OK... I see that this fu^H^Hmixed-up convention already exists in
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:

Il 13/12/21 04:37, Nicolas Pitre ha scritto:
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))
Unless we find a good place for this macro I'd be forced to leave double cast.
If it is preferred I can add a line of documentation before each occurence.
I think <sys/util.h> would be a good place.
I think you have noted that at least another member has presented some objections.

Which strategies the comunity have adopted in similar cases to obtain a fast convergence and to avoid bikeshedding?

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.
I'd like very much you take a look to

https://github.com/Abramo-Bagnara/zephyr/commit/87a42d4185828fb1e651604b8ee878063fb6b08a
Please make it into a fetchable branch. That'd make my reviewing easier.
You can find it as the last commit of branch MISRA_Essential_Types of

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:
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))
Unless we find a good place for this macro I'd be forced to leave double cast.
If it is preferred I can add a line of documentation before each occurence.
I think <sys/util.h> would be a good place.

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.
I'd like very much you take a look to

https://github.com/Abramo-Bagnara/zephyr/commit/87a42d4185828fb1e651604b8ee878063fb6b08a
Please make it into a fetchable branch. That'd make my reviewing easier.


Nicolas

301 - 320 of 8570