andyross
commented
5 years ago One problem with making an assertion into an early return is cleanup. Consider ("normal" C for clarity, not Zephyr code):
int foo(int arg)
{
struct whatever *result = malloc(sizeof(struct whatever));
// ...
__ASSERT(arg != INVALID_ARG, "");
}Now the code is going to leak the resource it acquired because it bailed too soon.
It's not insoluble, but it's messy. We could require that code never assert in a context where a synchronous return would be illegal (too limiting and too difficult a rule to follow). We could have an ASSERT_CLEANUP macro or something that worked like a FOREACH and had a block at the end (weird, but IMHO kinda clever).
Farther afield there are GCC extensions that allow you to define code to run at block exit we could try to leverage (sort of a hook to the C++ destructor invocation logic that the compiler has), and of course in the C++ world there are exceptions which speak to this need too.
But really I think my biggest worry here is that most assertions don't reflect things we want to check at runtime at all. They're bug catchers describing an invariant in the code. Right now we all feel free to sprinkle these around because we know they'll never end up in production and it makes us feel safer to have the extra layer of validation when the code changes. But if we knew we had to be careful with them... would we just stop using them? I think that would make the code worse, not better.
ceolin
andrewboie
b0661
pabigot
yashi
aescolar
marc-hb
Introduction
Purpose of this ticket is to discuss changes to our policies for kernel APIs as they relate to returning error values.
Problem description
Currently, we have the following:
Proposed change
With Zephyr moving towards functional safety certifications, we should allow for robust error checking, with APIs being augmented to return error codes.
1) APIs that return void will be converted to return an integer indicating success or failure, with failure returning a negative errno code. 2) APIs that return a pointer, should return NULL on an error condition. We might consider populating errno with a more detailed error code. 3) All current assertions in API implementation functions should be converted to return an error condition instead. 4) System call checks which duplicate implementation function assertions can be removed since the implementation function is always doing the checking now. 5) Additional system call checks should propagate error values instead of invoking Z_OOPS().
Some random thoughts for discussion
We need to decide if there are any valid uses of Z_OOPS() in system calls. Do we keep using Z_OOPS() for memory access issues, or kernel object validation checks?
We will need consistency on what error values are returned. For example, if we decide to return an error if memory access fails, every API should return -EACCES.
Returning errors instead of failing assertions/Z_OOPS() adds additional responsibility to the end user.
Additional code size due to adding runtime checks and propagation of return values
Is there a way we can add flexibility on what runtime checks do? Consider the following:
We could define __ASSERT three ways, selectable by Kconfig:
1) The default case has __ASSERT() evaluate to the expression in its first argument, cast to a boolean. So if foo != bar it evaluates to false and we return -EINVAL. This is the robust configuration. The string could either be logged or discarded by the compiler.
2) The second case would be for constrained applications which will enable assertions in the debug phase, but otherwise not do return value propagation. __ASSERT() will work like it used to and immediately generate a fatal error =, printing the string, if the check fails.
3) The third case will be for constrained applications where code size must be optimized at all costs. Here __ASSERT() will simply be defined as 'true'. The compiler will see this and not compile anything at all, it will be as if it wasn't there.