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 60325019aa ("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 f38c6b67ed ("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 <marc.herbert@intel.com>
This commit is contained in:
Marc Herbert 2022-01-08 00:57:31 +00:00 committed by Anas Nashif
commit 35bdab5c9a

View file

@ -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.