kernel: avoid using result of an assignment operator

[DeHess]

Fix coding guideline MISRA C:2012 Rule 13.4 in kernel:

The result of an assignment operator should not be used.

This PR is part of the enhancement issue #48002 which port the coding guideline fixes done by BUGSENG on the https://github.com/zephyrproject-rtos/zephyr/tree/v2.7-auditable-branch back to main

The commit in this PR is a subset of the original auditable-branch commit:
64336f4

[andyross]

How sticky is this particular rule? The last time the MISRApolcalypse came by, I remember arguing strongly that this particular pattern ("while((iterator = next()) != NULL)") was cleaner, safer and generally more readable than the alternatives, and (I think?) winning the argument.

In particular what you're doing is replacing them with a conceptually infinite loop (!!), which seems like the pessimal choice. Ew.

Can you at least do this with an equivalent for() construction instead? e.g. something along the lines of for(whatever* iter = next(); iter != NULL; iter = next())

[cfriedt]

Would probably be better to use do .. while loops in most of these situations.

Also, for (;;) can be on the fence in terms of coding style (although I personally like using it).

[cfriedt]

Suggested change

	for (;;) {
	pending_thread = z_unpend_first_thread(&condvar->wait_q);
	if (pending_thread == NULL) {
	break;
	}
	do {
	pending_thread = z_unpend_first_thread(&condvar->wait_q);
	} while(pending_thread != NULL);

[cfriedt]

Suggested change

	for (;;) {
	pending_thread = z_unpend_first_thread(&msgq->wait_q);
	if (pending_thread == NULL) {
	break;
	}
	do {
	pending_thread = z_unpend_first_thread(&msgq->wait_q);
	} while (pending_thread != NULL);

[cfriedt]

Suggested change

	for (;;) {
	thread = z_waitq_head(wait_q);
	if (thread == NULL) {
	break;
	}
	do {
	thread = z_waitq_head(wait_q);
	} while (thread != NULL);

[andyross]

Agreed that do/while is cleaner than for for a lot of these, but I have a nagging memory that MISRA disallows those too? Or maybe I'm confusing it with some other coding standard. But regardless let's try to do this with some care towards coding aesthetics. Unbounded loops where you have to dig to understand the invariant constraints are just bad.

[DeHess]

Would probably be better to use do .. while loops in most of these situations.

Also, for (;;) can be on the fence in terms of coding style (although I personally like using it).

Unfortunately these proposed changes only work if the condition/assignment is the only thing happening in the loop. Sadly that is not the case here. Even with do-while we'd be forced to have an if statement in there to break in case pending_thread == NULL. I don't think we can avoid such an if statement if we want to adhere to this Misra Guideline

Now whether we use for(;;), while, or do-while thats a different topic altogether.

[DeHess]

How sticky is this particular rule? The last time the MISRApolcalypse came by, I remember arguing strongly that this particular pattern ("while((iterator = next()) != NULL)") was cleaner, safer and generally more readable than the alternatives, and (I think?) winning the argument.

In particular what you're doing is replacing them with a conceptually infinite loop (!!), which seems like the pessimal choice. Ew.

Can you at least do this with an equivalent for() construction instead? e.g. something along the lines of for(whatever* iter = next(); iter != NULL; iter = next())

From what I understand changing it to a for loop would have to look like this to not violate the rule:

pending_thread = z_unpend_first_thread(&condvar->wait_q);
for (pending_thread; pending_thread != NULL; pending_thread = z_unpend_first_thread(&condvar->wait_q)) {
...
}

Else we're still utilising the result of an assignment operator directly. At least I think so.
Would you prefer this over the for(;;)?

[cfriedt]

Would probably be better to use do .. while loops in most of these situations.

Also, for (;;) can be on the fence in terms of coding style (although I personally like using it).

Unfortunately these proposed changes only work if the condition/assignment is the only thing happening in the loop. Sadly that is not the case here.

In the areas where I suggested changes, jte assignment was the only thing happening in the loop.

Even with do-while we'd be forced to have an if statement in there to break in case pending_thread == NULL.

Wait... on what planet would the loop not exit with the condition while (pending_thread != NULL)?

I can understand if there were other instructions within the loop other than a comparison, but there are none.

I don't think we can avoid such an if statement if we want to adhere to this Misra Guideline

What is / are the exact MISRA rules that this change purportedly adheres to?

Rule 6-6-4 states "For any iteration statement there shall be no more than one break or goto statement used for loop termination."

Using the loop condition to terminate the loop is definitely allowed. do .. while loops are definitely allowed.

Is there maybe a misinterpretation that there should be exactly one break statement rather than maximally one break statement?

Ref:
https://www.synopsys.com/software-integrity/static-analysis-tools-sast/misra.html

Now whether we use for(;;), while, or do-while thats a different topic altogether.

for(;;) is an infinite loop, implicitly, as Andy mentioned, which is (AFAIK) also not allowed.

[andyross]

Else we're still utilising the result of an assignment operator directly. At least I think so.

Uh... no? Can you explain where you're seeing that. This was my suggestion:

for(whatever_t *iter = next(); iter != NULL; iter = next()) { ... }

The first of the three for() expressions is an initializer. It declares a local variable and initializes it. Strictly that's not an "assignment operator" at all, but even if it were the result of it isn't being used outside the statement. The second is a test, no assignment operator. The third is an assignment operator which is used only for its side effects, no further use of the result of the "=" operator is made.

But the broader point is this: loops whose invariants are expressed as internal exit conditions and not using the proper C syntax for loop invariants are bad style. And frankly they're much worse style than anything MISRA is trying to prevent. I'm neutral on do/while, for(), etc... There are a million ways to write perfectly readable code. But an infinite loop with a break inside of it is not one. There are circumstances where you need to write such a thing, but they're rare (and generally isomorphic to something like "error handling").

I think I'm -1 on this. If the compliance rules and the requirements for them are really inflexible, then escalate and let's have the fight to overrule me.

[DeHess]

@cfriedt
Currently, the first subroutine in upstream main looks like this:
(condvar.c, starting on line 87)

while ((pending_thread = z_unpend_first_thread(&condvar->wait_q)) != NULL) {
	woken++;
	arch_thread_return_value_set(pending_thread, 0);
	z_ready_thread(pending_thread);
}

BUGSENG authored and I committed the following change initially:

for (;;) {
	pending_thread = z_unpend_first_thread(&condvar->wait_q);
	if (pending_thread == NULL) {
		break;
	} 
	woken++;
	arch_thread_return_value_set(pending_thread, 0);
	z_ready_thread(pending_thread);
}

You requested these changes:

do {
	pending_thread = z_unpend_first_thread(&condvar->wait_q);
} while(pending_thread != NULL);
	woken++;
	arch_thread_return_value_set(pending_thread, 0);
	z_ready_thread(pending_thread);
}	

The initial subroutine is not only an assignment and a condition. You are right on all other counts however.
I will use a for loop just like @andyross proposed. Personally I think that would be the most satisfactory solution

[andyross]

OK, thanks. I don't hate this version. :)

[DeHess]

OK, thanks. I don't hate this version. :)

Thanks for bearing with me for this one :)