numbers.h: downgrade MIN() and MAX() to unsafe versions #4119
Conversation
|
https://sof-ci.01.org/sofpr/PR4119/build8880/devicetest is all green The checkpatch warnings about unsafe macros are... valid https://sof-ci.01.org/sofpr/PR4119/build8880/checkpatch |
marc-hb
requested review from
akloniex,
cujomalainey,
dbaluta,
dgreid,
johnylin76,
lbetlej,
lgirdwood,
mmaka1,
mrajwa and
plbossart
as code owners
lgirdwood
left a comment
lgirdwood
left a comment
There was a problem hiding this comment.
Thanks @marc-hb this moves us closer and closer to Zephyr. One thing that is also needed here is that we should have in numbers.h and other places.
#if !__ZEPHYR__
#define MIN and MAX sof version
#endif This will make sure we really are using Zephyr versions. These checks can all be removed when we remove xtos.
The current I will replace it with the above after one successful daily test run on this merged PR (EDIT: #4130) |
For consistency with Zephyr, MIN and MAX macros are "downgraded" to the basic, compile-time, standard compliant and unsafe implementation that Zephyr uses. "Unsafe" because it can evaluate its arguments twice which is obviously not desired when the argument has side-effects: https://wiki.sei.cmu.edu/confluence/display/c/PRE31-C.+Avoid+side+effects+in+arguments+to+unsafe+macros I reviewed every single invocation of MIN() and MAX() and found no increment or assignment but I found a number of function calls in their arguments. This commit removes them all. Note I did _not_ check for volatile variables. Most of these functions do not appear to have side-effects _but_ it's much simpler and faster for the reader or static analyzer not to even have to wonder about the possibility of side-effects and it's also more future-proof; no risk of a function accidentally gaining side effects. Note the following files are not compiled in the default configurations and the changes in them were NOT compile-tested locally: src/audio/codec_adapter/codec_adapter.c src/audio/drc/drc_generic.c Signed-off-by: Marc Herbert <marc.herbert@intel.com>
For compatibility with Zephyr, MIN and MAX macros are downgraded to the basic, compile-time, standard compliant and unsafe implementation. This means the code will now be the same whether it's compiled with Zephyr versus not. Fixes zephyrproject-rtos/zephyr#33567 which means it's now possible to use MIN() or MAX() in either Zephyr's BUILD_ASSERT() or in SOF's equivalent STATIC_ASSERT() or in C11's _Static_assert() This implementation is unsafe because it can evaluate its arguments twice which is obviously not desired when the argument has side-effects: https://wiki.sei.cmu.edu/confluence/display/c/PRE31-C.+Avoid+side+effects+in+arguments+to+unsafe+macros Signed-off-by: Marc Herbert <marc.herbert@intel.com>
|
https://sof-ci.01.org/sof-pr-viewer/#/build/PR4119/build6376934 failed on ICL, TGL and CNL because I made a (fixed) typo in @mrajwa , @dbaluta , @cujomalainey , @lgirdwood : is there some open-source codec we could use in some open-source configuration accessible and added to Github Actions? So we don't have to wait for Quickbuild, sometimes it takes days to run or does not run at all. |
|
Of course when https://sof-ci.01.org/sof-pr-viewer/#/build/PR4119/build6380518 is green then it's the turn of one system in https://sof-ci.01.org/sofpr/PR4119/build8897/devicetest/ to have a glitch. Can't have all planets aligned at the same time! => Test are all passing. |
2 participants
[#4002 was a completely different and rejected solution to this problem , please start the review there]
For compatibility with Zephyr, MIN and MAX macros are downgraded to the
basic, compile-time, standard compliant and unsafe implementation.
Fixes zephyrproject-rtos/zephyr#33567
See much longer commit messages.