From 35bdab5c9a03d49954bbcb222f22625221e9be40 Mon Sep 17 00:00:00 2001 From: Marc Herbert Date: Sat, 8 Jan 2022 00:57:31 +0000 Subject: [PATCH] cmake: drop -fno-strict-overflow for better performance The `-fno-strict-overflow` option blocks some gcc optimizations, removing it in SOF showed a measurable performance improvement: see code review #41457 of commit 60325019aa79 ("samples/subsys/audio/sof: use -fstrict-overflow for SOF"). Add these optimizations to every Zephyr project beyond the somewhat awkward and maybe temporary way the SOF `samples/` (?) is currently built. There are non-SOF specific discussions in #41457 BTW. Fixes #8924 which already had a plan to remove this option in 2018. It looks like some bad Github feature unfortunately auto-closed the issue before it was actually removed. `-fno-strict-overflow` is needed only for code that relies on signed or pointer wrapping, which is an undefined C behavior. Most security policies seem to forbid undefined C behaviors. (Digression: _unsigned_ integer wrapping is _defined_ behavior and expected from any half-decent compiler.) Until gcc version 7, the default -fstrict-overflow value was depending on the -On optimization level. In gcc version 8, the whole feature was greatly simplified: -fnostrict-overflow became a simple alias for `-fwrapv` and `-fwrapv-pointer`: wrapping or no wrapping, period. For the subtle, pre-v8 difference between -fno-strict-overflow and -fwrapv please read this great, pre-v8 intro from the author of strict-overflow: - https://www.airs.com/blog/archives/120 And also: - https://gcc.gnu.org/onlinedocs/gcc-11.2.0/gcc/Code-Gen-Options.html - https://gcc.gnu.org/onlinedocs/gcc-7.3.0/gcc/Optimize-Options.html Important quote from Code-Gen-Options.html: > Most of the options have both positive and negative forms; the > negative form of -ffoo is -fno-foo. In the table below, only one of the > forms is listed: the one that is not the default. This means _undefined_ wrapping is the default. So simply removing this zephyr_cc_option() line is enough to switch and great if any project desires overriding it locally: no need to scrutinize the command line order and pray it does the right thing (have a look at the precedence between -f[no]-wrapv and -f[no-]trapv for some of that fun). With gcc, the documented `-Wall` list includes `-Wstrict-overflow=1`: - https://gcc.gnu.org/onlinedocs/gcc-11.2.0/gcc/Warning-Options.html -Wextra does not increase the level of this warning. This warning reports some of the cases where a gcc optimization comes and actually "breaks" signed (and undefined!) wrapping. A real example was just fixed in commit f38c6b67ed16 ("samples: big_http_download: make num_iterations unsigned"). Note this reporting is incomplete and it also assumes that developers look at warnings (not always true) or that CI defines `-DEXTRA_CFLAGS=-Werror` as it should (I am pretty sure I remember Zephyr CI adding -Werror in the past but I'm afraid it's gone right now). Increasing the level to -Wstrict-overflow=2 or more seems to report too many false positives: #41551 Additional info: - gcc also supports `-ftrapv` to catch signed overflow at _run-time_. - Clang has `-fsanitize=signed-integer-overflow`. I did not test it. https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html - zephyr_cc_option() silently discards its argument for toolchains that fail it (so arguments with typos are silently ignored globally) Signed-off-by: Marc Herbert --- CMakeLists.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 5087be083de..af6c8cc3682 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -351,7 +351,6 @@ include(cmake/extra_flags.cmake) zephyr_cc_option(-fno-asynchronous-unwind-tables) zephyr_cc_option(-fno-pie) zephyr_cc_option(-fno-pic) -zephyr_cc_option(-fno-strict-overflow) if(CONFIG_THREAD_LOCAL_STORAGE) # Only support local exec TLS model at this point.