GNOME Bugzilla – Bug 741168
[review] unrelated patches to avoid compiler warning with -Wstrict-overflow
Last modified: 2015-01-12 15:51:03 UTC
I wanted to compile with gcc-4.8.3, "-O2 -Wstrict-overflow". It almost works. Attached 3 patches... Arguably, "wifi: avoid strict-overflow warning in nm_ap_utils_complete_c..." is ugly. But I don't see another way.
Created attachment 292197 [details] [review] libnm: return 0 for empty address in hwaddr_binary_len() Motivated by avoiding compiler warning with -O2 -Wstrict-overflow (gcc-4.8.3): make[4]: Entering directory `./NetworkManager/libnm-core' CC nm-utils.lo ../libnm-core/nm-utils.c: In function 'nm_utils_hwaddr_valid': ../libnm-core/nm-utils.c:2725:14: error: assuming signed overflow does not occur when simplifying conditional to constant [-Werror=strict-overflow] if (length == 0 || length > NM_UTILS_HWADDR_LEN_MAX) ^ ../libnm-core/nm-utils.c: In function 'nm_utils_hwaddr_canonical': ../libnm-core/nm-utils.c:2755:14: error: assuming signed overflow does not occur when simplifying conditional to constant [-Werror=strict-overflow] if (length == 0 || length > NM_UTILS_HWADDR_LEN_MAX) ^
Created attachment 292198 [details] [review] dispatcher: refactor constructing environment variables from strv This also avoids a warning with -O2 -Wstrict-overflow (gcc-4.8.3): make[4]: Entering directory `./NetworkManager/callouts' CC libtest_dispatcher_envp_la-nm-dispatcher-utils.lo nm-dispatcher-utils.c: In function 'construct_ip6_items': nm-dispatcher-utils.c:283:8: error: assuming signed overflow does not occur when simplifying conditional to constant [-Werror=strict-overflow] if (i != 0) ^ nm-dispatcher-utils.c: In function 'construct_ip4_items': nm-dispatcher-utils.c:144:8: error: assuming signed overflow does not occur when simplifying conditional to constant [-Werror=strict-overflow] if (i != 0) ^ nm-dispatcher-utils.c:168:8: error: assuming signed overflow does not occur when simplifying conditional to constant [-Werror=strict-overflow] if (i != 0) ^
Created attachment 292199 [details] [review] wifi: avoid strict-overflow warning in nm_ap_utils_complete_connection() avoid a warning with -O2 -Wstrict-overflow (gcc-4.8.3): make[6]: Entering directory `./NetworkManager/src/devices/wifi/tests' CC nm-wifi-ap-utils.o CCLD test-wifi-ap-utils test-wifi-ap-utils.c: In function 'complete_connection.2533.constprop.19': ./../nm-wifi-ap-utils.c:727:5: error: assuming signed overflow does not occur when simplifying conditional to constant [-Werror=strict-overflow] if (adhoc) { ^
Patches apply on master, 5bfb4c8c23e5b9d8020d6caff8c63830d2d8ec37
Comment on attachment 292199 [details] [review] wifi: avoid strict-overflow warning in nm_ap_utils_complete_connection() >- gboolean adhoc = FALSE; >+ guint adhoc = FALSE; No. That is so wrong. -Wstrict-overflow is pretty obnoxious, in that it doesn't tell you where the compiler thinks the bug is, it just tells you what later lines would be affected if there had been a bug earlier. And changing ints to uints is a silly fix anyway, because it doesn't actually fix the problem that gcc thinks it sees. It just turns the signed overflow into an unsigned overflow, which gcc doesn't warn you about, because it has well-defined behavior according to the spec (despite the fact that overflowing is almost certainly wrong if it could actually happen in the code). In this case, we have absolutely no clue what gcc is thinking, so silencing the warning in a way that doesn't address the underlying issue is wrong, because there may be an actual bug here.
(In reply to comment #5) > (From update of attachment 292199 [details] [review]) > >- gboolean adhoc = FALSE; > >+ guint adhoc = FALSE; > > No. That is so wrong. > > -Wstrict-overflow is pretty obnoxious, in that it doesn't tell you where the > compiler thinks the bug is, it just tells you what later lines would be > affected if there had been a bug earlier. > > And changing ints to uints is a silly fix anyway, because it doesn't actually > fix the problem that gcc thinks it sees. It just turns the signed overflow into > an unsigned overflow, which gcc doesn't warn you about, because it has > well-defined behavior according to the spec (despite the fact that overflowing > is almost certainly wrong if it could actually happen in the code). > > In this case, we have absolutely no clue what gcc is thinking, so silencing the > warning in a way that doesn't address the underlying issue is wrong, because > there may be an actual bug here. Looks like the warning is incorrect, so there is no underlying issue to fix. (Unless you can see it). Either the code gets somehow restructured so that the warning goes away, or the code cannot compile with these flags. I can not compile without -Wstrict-overflow, ... but that is silencing the warning too (and not only for this code place). So you suggest that -Wstrict-overflow makes no sense in general and we just don't enable it?
(In reply to comment #6) > Looks like the warning is incorrect, so there is no underlying issue to fix. > (Unless you can see it). I can't see any bug, but who knows what gcc's optimizer is doing to that code before getting to the point where it sees a potential overflow... > So you suggest that -Wstrict-overflow makes no sense in general and we just > don't enable it? Yeah. The gcc docs even warn that "this warning can easily give a false positive". And given that as far as we can tell, it didn't actually find any real bugs in this run, it seems like it's more of a nuisance than a feature. (You could add "-Wstrict-overflow -Wno-error=strict-overflow" to CFLAGS when doing your own builds if you want to see the warnings but not break the build...)
Review of attachment 292197 [details] [review]: LGTM
Review of attachment 292198 [details] [review]: Rest looks good to me. ::: callouts/nm-dispatcher-utils.c @@ +54,3 @@ } +static GSList *_list_append_val_strv (GSList *items, char **values, const char *format, ...) __attribute__((__format__ (__printf__, 3, 4))); G_GNUC_PRINTF(3,4) instead? it's shorter and evals to the same thing.
(In reply to comment #3) > Created an attachment (id=292199) [details] [review] > wifi: avoid strict-overflow warning in nm_ap_utils_complete_connection() > > avoid a warning with -O2 -Wstrict-overflow (gcc-4.8.3): > > make[6]: Entering directory `./NetworkManager/src/devices/wifi/tests' > CC nm-wifi-ap-utils.o > CCLD test-wifi-ap-utils > test-wifi-ap-utils.c: In function 'complete_connection.2533.constprop.19': > ./../nm-wifi-ap-utils.c:727:5: error: assuming signed overflow does not > occur when simplifying conditional to constant [-Werror=strict-overflow] > if (adhoc) { > ^ Does it build correctly in wifi, but *not* in test? Since src/devices/wifi/Makefile.am has "SUBDIRS= . test" this seems to imply that it does, unless you have parallel make running?
Comment on attachment 292197 [details] [review] libnm: return 0 for empty address in hwaddr_binary_len() Patch attachment 292197 [details] [review] commited.
Comment on attachment 292198 [details] [review] dispatcher: refactor constructing environment variables from strv Merged as http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=37361a038f0dbc503943552d9bf4297451bea417 after changing G_GNUC_PRINTF
(In reply to comment #10) > (In reply to comment #3) > > Created an attachment (id=292199) [details] [review] [details] [review] > > wifi: avoid strict-overflow warning in nm_ap_utils_complete_connection() > > > > avoid a warning with -O2 -Wstrict-overflow (gcc-4.8.3): > > > > make[6]: Entering directory `./NetworkManager/src/devices/wifi/tests' > > CC nm-wifi-ap-utils.o > > CCLD test-wifi-ap-utils > > test-wifi-ap-utils.c: In function 'complete_connection.2533.constprop.19': > > ./../nm-wifi-ap-utils.c:727:5: error: assuming signed overflow does not > > occur when simplifying conditional to constant [-Werror=strict-overflow] > > if (adhoc) { > > ^ > > Does it build correctly in wifi, but *not* in test? Since > src/devices/wifi/Makefile.am has "SUBDIRS= . test" this seems to imply that it > does, unless you have parallel make running? I think it happens due to inlining of nm_ap_utils_complete_connection() into the test routine.