include: util: use single evaluation MIN, MAX, CLAMP by default

[fabiobaltieri]

include: util: use single evaluation MIN, MAX, CLAMP by default

Change the default implementation for MIN, MAX and CLAMP for a single evaluation version. These are already implemented as a toolchain specific version with the rationale that they use some compiler extension, but the only thing they use is typeof [1] and statement expressions [2], which are used by other macros in util.h, so it seems like
a fair assumption to say that all supported compilers implements it.

In the off chances that that's not the case and for cases that do not work with the single evaluation version, provide a set of GENERIC_ version of the same macro, which uses the current double evaluation but more flexible version of the same.

With this change, the default macros are not susceptible to race conditions and other difficult to reproduce erroneous behavior due to the double evaluation aspect for it, the compromise is that some existing macros have to be changed to use the generic version, but that is at least exposed at build time.

Deprecate the old macros.

[1] https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html
[2] https://gcc.gnu.org/onlinedocs/gcc/Typeof.html

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[github-actions]

The following west manifest projects have changed revision in this Pull Request:

Name	Old Revision	New Revision	Diff
hal_nxp	zephyrproject-rtos/hal_nxp@bcab469 (master)	zephyrproject-rtos/hal_nxp#606	zephyrproject-rtos/hal_nxp#606/files
nrf_wifi	zephyrproject-rtos/nrf_wifi@acb24fe (main)	zephyrproject-rtos/nrf_wifi#76	zephyrproject-rtos/nrf_wifi#76/files
sof	zephyrproject-rtos/sof@ba8de75 (zephyr)	zephyrproject-rtos/sof#56	zephyrproject-rtos/sof#56/files

⛔ DNM label due to: 3 projects with PR revision

Note: This message is automatically posted and updated by the Manifest GitHub Action.

[fabiobaltieri]

cc @evgeniy-paltsev @abrodkin @RobinKastberg @tejlmand (from the various Toolchain areas)

[pdgendt]

Maybe a dumb question, but why not introduce macros for the variant that uses __typeof__? I'm guessing this will also impact downstream users?

[aescolar]

My two cents:
MAX and MIN are very typical macros to have with that definition before this PR.
So I'm in no way surprised about the double evaluation, and I expect many of our users won't either.
At the same time, I expect users are going to be using them for constant expr. just like we do in many places.

Given that we already have Z_MIN/Z_MAX, this is mostly a double API renaming exercise. With one of the old one names being deprecated, and the other ending partially incompatible.
So it is going to cause quite a bit of churn.

I would not do this.

[krish2718]

@fabiobaltieri thanks, please update manifest to 53500666a59195b44e2e5a939c111effbf23db7b for nrf_wifi

[RobinKastberg]

This is fine for IAR, but we tend to agree with aescolar above.

[fabiobaltieri]

@pdgendt @aescolar hey, sorry I was meaning to add more context to this:

The current story of MIN/MAX is that they are the "trivial" double evaluation implementations, and we provide a Z_MIN and Z_MAX in include/zephyr/toolchain/gcc.h and include/zephyr/toolchain/iar/iccarm.h, which are somwhat sold as "non portable".

The problem with the double evaluation one is that they can create some nasty race condtitions, we just got to the bottom of one and it was a monumental pain to troubleshoot.

As things are now, one would have to be aware of the problem (which I think it's very easy to miss) and explicitly use the Z_ variant of the macro. Given that there's exactly 4 uses of it in the current code base it seems evident that this is not exactly common knowledge. Can't tell for sure if any of the current MIN/MAX use cases are prone to race condition, but given how common the use of those macros are, it's hard to imagine that there's none.

If we do this change instead, the "common" implementation of the macro would be safe, and if it does not work for the specific use case (which there's many as you can tell by the size of this PR) at least it fails at build time and the fix is trivial.

So it is going to cause quite a bit of churn.

Yeah this is probably going to break loads of stuff downstream, but at least it'd be a breakage at build time and the fix is trivial. On the contrary race conditions caused by these are ridiculously hard to troubleshoot, so I think that there's value in having a "default" impementation that is safe.

Bit more context: Linux does not have any of this and gets away with a "Use for obvious constants only.": https://elixir.bootlin.com/linux/v6.16.8/source/include/linux/minmax.h#L314-L315 so maybe I'm overthinking the whole situation, but it seems like a good idea to have a safe default implementation.

On the contrary, in our code base pre-Zephyr we used to have safe MIN/MAX and the generic ones, basically what I'm proposing here.

Anyway, happy to hear everyone's opinion, plan B would be to at least move the Z_ one out of the toolchain specific files into util.h, I don't think there's any portability issues given that the extensions used by those are used by other stuff in utils.h anyway.

[fabiobaltieri]

Actually more about the Linux case, they have some single evaluation min_t and max_t with explicit type (wonder why) which also have a fancy unique identifier logic to make them usable in nested cases. Seems like those are used widely in the code base so maybe we could take that path instead. There's also a min3 and max3 which could be useful, seen many cases of nested min and nested max. Curiously enough, the "3" variant has automatic types.

Gotta love embedded. Nothing is ever easy.

[fabiobaltieri]

So I think the alternative would be:

• move Z_MIN/Z_MAX into util.h
• do a pass of the current code base and use those whenever one of the two operands is a function
• do the same in my downstream application
• hope that that's enough to build a habit on new code

[fabiobaltieri]

Ok I've figured how to take care of the nested case using that clever trick that Linux uses as well

#define _MIN_UNIQUE(a, b, ua, ub) ({ \
                __typeof__(a) ua = (a); \
                __typeof__(b) ub = (b); \
                (ua < ub) ? ua : ub; \
        })
 
#define _MIN(a, b, cnt) \
        _MIN_UNIQUE(a, b, UTIL_CAT(_value_a_, cnt), UTIL_CAT(_value_b_, cnt))
 
#define MIN(a, b) _MIN(a, b, __COUNTER__)

I've asked to discuss this at arch tomorrow, I'd say let's compare notes there, either way I'll incorporate this as well as a followup, if we decide to change the default MAX behavior there's few nested case that I changed here that I can revert back.

[aescolar]

Ok I've figured how to take care of the nested case using that clever trick that Linux uses as well

Sidetracking, AFAIK __COUNTER__ is also a gnu extension, but I guess the only problem with nesting is the -Wshadow warning(?). I guess that __COUNTER__ use could be ifdef'ed with a #if __GNUC__ , or maybe easier the warning disabled:

#define PRAGMA_NOSHADOW_WARN_PUSH \
_Pragma ("GCC diagnostic push") \
_Pragma ("GCC diagnostic ignored \"-Wshadow\"")

#define PRAGMA_WARN_POP \
_Pragma ("GCC diagnostic pop")

#define MYMACRO(a, b) ({ \
PRAGMA_NOSHADOW_WARN_PUSH \
                __typeof__(a) ua = (a); \
                __typeof__(b) ub = (b); \
PRAGMA_WARN_POP \
...

[fabiobaltieri]

I guess that __COUNTER__ use could be ifdef'ed with a #if __GNUC__ , or maybe easier the warning disabled

Ok thanks for looking into it, few options around think I'd rather try with the counter first and then if any folks from the other compiler says they don't support it try something else.

[RobinKastberg]

__COUNTER__ is supported in IAR. (but we don't set __GNUC__)
You can find information like this in the manual:
https://wwwfiles.iar.com/arm/webic/doc/EWARM_DevelopmentGuide.ENU.pdf

[fabiobaltieri]

Alright folks thanks for the feedback, had some good feedback from this in the last few minutes of arch wg yesterday, the consensus seems to be that it's not worth the downstream breakages, so I'll bin this PR and start working on a new one, or rather a few new ones.

Thinking initially I'll just move Z_MIN out of the toolchain file into util.h and add the nested call support and late it bake for a few weeks (while I'm on vacation) just to give it a chance of hitting any compiler compatibility, then I'll propose sync up the macro name with the Linux ones (min/MIN max/MAX, which I think has a lot of value for developers working across projects and will likely raise some eyebrows due to the naming), and then will try to sprinkle some of these around the code base.

Looking at the Linux code it seems like Linux devs are well used to use min/max everywhere and MIN/MAX only when necessary, ideally we'd get there but we should really have enough of those in the code base to make it a habit and have reviewers catch it.

Anyway, that's the plan, start from skratch.