Get rid of -fno-strict-overflow #8924
Assignees
Labels
Comments
nashif
added
bug
jukkar
added a commit
to jukkar/zephyr
that referenced
this issue
Jul 18, 2018
If -fno-strict-overflow compiler flag is disabled, then this warning
is printed:
samples/net/throughput_server/src/server.c:157:6: \
error: assuming signed overflow does not occur when assuming \
that (X + c) >= X is always true [-Werror=strict-overflow]
if (new_print > curr) {
Fixes zephyrproject-rtos#8924
Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>
marc-hb
added a commit
to marc-hb/zephyr
that referenced
this issue
Jan 4, 2022
For performance reasons, see zephyrproject-rtos#41547 and zephyrproject-rtos#8924 Signed-off-by: Marc Herbert <marc.herbert@intel.com>
|
As of today this requires one new 1 line-fix to pass twister tests: #41554 |
marc-hb
added a commit
to marc-hb/zephyr
that referenced
this issue
Jan 8, 2022
The `-fno-strict-overflow` option blocks some gcc optimizations, removing it in SOF showed a measurable performance improvement: see code review zephyrproject-rtos#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 zephyrproject-rtos#41457 BTW. Fixes zephyrproject-rtos#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: zephyrproject-rtos#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>
nashif
pushed a commit
that referenced
this issue
Jan 10, 2022
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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
We only have 1 issue when this is disabled, lets fix it and get rid of the option:
The text was updated successfully, but these errors were encountered: