cmake: drop -fno-strict-overflow for better performance

[marc-hb]

Thanks to "peer pressure" from @kv2019i , here comes the longest and most-time consuming commit message for removing a single line of code.

---

The -fno-strict-overflow option blocks some gcc optimizations,
removing it in SOF showed a measurable performance improvement: see code
review #41457 of commit 6032501 ("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 f38c6b6 ("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

[marc-hb]

Any objection, comment? This switch was apparently planned in 2018's #8924 but did not happen at the time, maybe because Github auto-closed the issue?

Please subscribe anyone who may care.

[kv2019i]

@marc-hb The one remaining issue was fixed, so seems no blockers to submit as non-draft (and just remove the no-strict-overlow)?

[andyross]

Yes please.

[marc-hb]

Thanks to "peer pressure" from @kv2019i , here comes the longest and most-time consuming commit message for removing a single line of code.

Wait, I did not even include the (fascinating!) git archeology.

This flag came directly from... the Linux kernel. No joke, see commit a9b1c74 in 2015:

Adding the root Makefile from linux 3.19-rc7 Kbuild as is.
This Makefile will be modified and adapted to the current build
system in following commits.

So here's the original Linux commit in 2009 from Linus:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a137802ee839ace

Don't use '-fwrapv' compiler option: it's buggy in gcc-4.1.x

This causes kernel images that don't run init to completion with certain
broken gcc versions.

This fixes kernel bugzilla entry:
	http://bugzilla.kernel.org/show_bug.cgi?id=13012

I suspect the gcc problem is this:
	http://gcc.gnu.org/bugzilla/show_bug.cgi?id=28230

Fix the problem by using the -fno-strict-overflow flag instead, which
not only does not exist in the known-to-be-broken versions of gcc (it
was introduced later than fwrapv), but seems to be much less disturbing
to gcc too: the difference in the generated code by -fno-strict-overflow
are smaller (compared to using neither flag) than when using -fwrapv.

And even before that, in 2009 too:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=68df3755e383

Add '-fwrapv' to gcc CFLAGS

This makes sure that gcc doesn't try to optimize away wrapping
arithmetic, which the kernel occasionally uses for overflow testing, ie
things like

	if (ptr + offset < ptr)

which technically is undefined for non-unsigned types. See

	http://bugzilla.kernel.org/show_bug.cgi?id=12597 "Kernel should be built with -fwrapv"

for details.

Not all versions of gcc support it, so we need to make it conditional
(it looks like it was introduced in gcc-3.4).

Reminded-by: Alan Cox <alan@lxorguk.ukuu.org.uk>

-fno-strict-overflow is still in the Linux kernel today.

During this dig, found this interesting online tool to test compiler flags and look at the generated assembly: https://godbolt.org/z/odq8h9

[tejlmand]

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

It does here:

zephyr/scripts/pylib/twister/twisterlib.py

Line 2141 in 190c9ba

cflags = "-Werror"

which defaults here:

zephyr/scripts/pylib/twister/twisterlib.py

Line 2338 in 190c9ba

self.warnings_as_errors = kwargs.get('warnings_as_errors', True)

zephyr/scripts/twister

Line 920 in 190c9ba

suite.warnings_as_errors = not options.disable_warnings_as_errors

[tejlmand]

lgtm.

Thanks for the digging.

[marc-hb]

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

It does here:

This does not work when compiling zephyr/tests/arch/x86/static_idt/src/test_stubs.S. Because it's assembly? Is this expected? I hope not.

I can file a bug, I have detailed and very fast reproduction steps.

[marc-hb]

Twister fix: twister: add -Werror to CMAKE_AFLAGS #41706