We used
COMPILER_FLAG(LIBSYSTEMD_NM_CFLAGS, "-Wno-gnu-variable-sized-type-not-at-end")
to detect whether the flag is supported. However, that does not work with GCC since
version 4.4 due to https://gcc.gnu.org/wiki/FAQ#wnowarning.
Note that we already had NM_COMPILER_WARNING(), but that again does
something rather different.
With LTO it's easy to get "-Wmaybe-uninitialized" false positives.
But the warning is useful, we we don't want to disable it altogether.
However, while investigating the problem it can be useful to patch
it temporarily. Add a code comment that suggests how to do that.
With LTO builds we get "-Wmaybe-uninitialized" warnings, which break
the build.
These seem false positives to me, due to aggressive inlining. But also
suppressing them with a pragma does not work. So, make them non-fatal
altogether. That is unfortunate, because this warning would be useful
to catch bugs.
Interestingly, when building --with-more-asserts, then the build passes
without warning. Probably because then the inlining doesn't happen.
libtool: link: gcc -Wall -Werror -Wno-stringop-overflow -Wextra -Wdeclaration-after-statement -Wfloat-equal -Wformat-nonliteral -Wformat-security -Wimplicit-function-declaration -Winit-self -Wlogical-op -Wmissing-declarations -Wmissing-include-dirs -Wmissing-prototypes -Wpointer-arith -Wshadow -Wshift-negative-value -Wstrict-prototypes -Wundef -Wvla -Wno-duplicate-decl-specifier -Wno-format-truncation -Wno-format-y2k -Wno-missing-field-initializers -Wno-pragmas -Wno-sign-compare -Wno-unknown-pragmas -Wno-unused-parameter -Wno-array-bounds -Wunused-value -Wcast-function-type -Wimplicit-fallthrough -fno-strict-aliasing -fdata-sections -ffunction-sections -Wl,--gc-sections -flto -flto-partition=none -O2 -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -Wl,-z -Wl,relro -Wl,--as-needed -Wl,-z -Wl,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -o src/platform/tests/test-platform-general src/platform/tests/test_platform_general-test-platform-general.o src/.libs/libNetworkManagerTest.a -luuid -lgnutls -lsystemd -lndp -lselinux -L/lib64 -laudit -lpsl -lcurl -lgio-2.0 -lgobject-2.0 -lgmodule-2.0 -lglib-2.0 -ludev -ldl -pthread
src/platform/nm-linux-platform.c: In function 'ip_route_add':
src/platform/nm-platform.c:4761:24: error: 'MEM[(struct NMPlatformIPRoute *)&obj + 24B].rt_source' may be used uninitialized in this function [-Werror=maybe-uninitialized]
4761 | route->rt_source = nmp_utils_ip_config_source_round_trip_rtprot(route->rt_source);
| ^
src/platform/nm-platform.h:2139:25: error: 'BIT_FIELD_REF <MEM[(const struct NMPlatformIPRoute *)&obj + 24B], 8, 72>' may be used uninitialized in this function [-Werror=maybe-uninitialized]
2139 | return r->table_any ? 254u /* RT_TABLE_MAIN */
| ^
lto1: all warnings being treated as errors
lto-wrapper: fatal error: gcc returned 1 exit status
No amount of _Pragma was able to disable this warning.
In function ‘strncpy’,
inlined from ‘_nm_strndup_a_step’ at ./shared/nm-glib-aux/nm-macros-internal.h:1367:3,
inlined from ‘nms_keyfile_nmmeta_check_filename’ at src/settings/plugins/keyfile/nms-keyfile-utils.c:61:0:
/usr/include/bits/string_fortified.h:106:10: error: ‘__builtin_strncpy’ specified bound depends on the length of the source argument [-Werror=stringop-overflow=]
106 | return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
| ^
src/settings/plugins/keyfile/nms-keyfile-utils.c: In function ‘nms_keyfile_nmmeta_check_filename’:
src/settings/plugins/keyfile/nms-keyfile-utils.c:44: note: length computed here
44 | len = strlen (filename);
|
lto1: all warnings being treated as errors
Oddly enough, gcc is still emitting the warning even with "-Wno-stringop-overflow",
but at least it doesn't break the build.
Seen on Debian 9, clang-3.8 (1:3.8.1-24):
../libnm-core/nm-setting-bond.c:596:49: error: comparison of constant 32 with expression of type 'NMBondMode' is always true [-Werror,-Wtautological-constant-out-of-range-compare]
nm_assert (_NM_INT_NOT_NEGATIVE (mode) && mode < 32);
~~~~ ^ ~~
This warning is not useful. While it may be implementation defined how enum
values outside the defined ones are handled, we commonly rely on placing
special numeric values in enums (e.g. ((NMEnumType) -1)).
An enum is (with our compilers) just a glorified integer, and there is nothing
preventing it from being outside the enum values. The warning is not helpful
and outright wrong. Disable it.
See-also: https://bugs.llvm.org//show_bug.cgi?id=16154
Fixes: 957bb2e111 ('libnm: use binary search for _nm_setting_bond_option_supported() implementation')
clang on CentOS 7.6 (3.4.2-9.el7) warns:
CC clients/tui/newt/clients_tui_newt_libnmt_newt_a-nmt-newt-button.o
In file included from ../clients/tui/newt/nmt-newt-button.c:26:
In file included from ../shared/nm-default.h:280:
../shared/nm-glib-aux/nm-macros-internal.h:1617:2: error: unknown warning group -Wstringop-truncation, ignored [-Werror,-Wunknown-pragmas]
NM_PRAGMA_WARNING_DISABLE ("-Wstringop-truncation");
^
../shared/nm-glib-aux/nm-macros-internal.h:419:9: note: expanded from macro NM_PRAGMA_WARNING_DISABLE
_Pragma(_NM_PRAGMA_WARNING_DO(warning))
^
<scratch space>:109:25: note: expanded from here
GCC diagnostic ignored "-Wstringop-truncation"
^
This warning totally defeats the purpose of why we use the pragma in the
first place.
Some warnings depends on others: -Wformat-security won't work without
-Wformat. With -Wall we're confident enough that we have important
warnings enabled and in any case we're going to enable it anyway.
https://github.com/NetworkManager/NetworkManager/pull/175
With --enable-more-warnings, we already used -std=gnu99, see
commit ba2b2de3ad.
Compilation may behave differently depending on the selected
C standard that we choose. It seems wrong, with more-warnings,
to build against a C standard, while otherwise leaving it undefind.
Indeed, one might argue, that our build system should not use
such compiler specific options. At least, not without detecting
support for the compiler option during ./configure.
However:
- we already did this for --enable-more-warnings.
- we should not program against a theoretical compiler. In practice,
only gcc and clang works to build NetworkManager. Both these compilers
support this option, so there is no reason to not use it. If we ever
come into the situation to support another compiler, adjusting -std=gnu99
will be the smallest problem. Until that happens (and that's far from
imminent), don't pretend to be portable to non-existing compilers and
use the flag that in practice is available.
See-also: https://gcc.gnu.org/onlinedocs/gcc/Standards.html
Make it possible to add compiler options to a different variable than
CFLAGS. This is useful to conditionally disable a compiler warning for a
subpart of a tree.
We don't use them, so add a compiler warning about their use.
I think they are annoying, because sizeof(x) and typeof(x) might
evaluate at runtime. Especially the typeof() extension is useful
for macros, but behaves badly with variable-length arrays due to
running code at runtime.
But the worst is, G_STATIC_ASSERT() is implemented by declaring
an array of negative length. Usually, the checked condition should
be a compile time constant, but with VLAs the compiler would not evaluate
the static-assert at compile time and instead accept it silently. It's easy
to mess up static asserts to wrongly have a non-constant condition,
especially when the static-assert is used inside a macro.
Just say no.
src/NetworkManagerUtils.c:347:18: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces]
NMIPAddr a1 = { 0 }, a2 = { 0 };
^
{}
Should we initialize unions this way? I think it's all right -- the initializer
works well and { { { 0 } } } is probably not what we'd like to see.
(cherry picked from commit 43012156a3)
The warning seems questionable and overly strict.
For now, just disable it to allow building with gcc7.
src/systemd/src/basic/time-util.c: In function ‘format_timespan’:
src/systemd/src/basic/time-util.c:509:46: error: ‘%0*lu’ directive output between 1 and 2147483648 bytes may cause result to exceed ‘INT_MAX’ [-Werror=format-truncation=]
"%s"USEC_FMT".%0*"PRI_USEC"%s",
^~~~
src/systemd/src/basic/time-util.c:509:60: note: format string is defined here
"%s"USEC_FMT".%0*"PRI_USEC"%s",
src/systemd/src/basic/time-util.c:509:46: note: directive argument in the range [0, 18446744073709551614]
"%s"USEC_FMT".%0*"PRI_USEC"%s",
^~~~
https://mail.gnome.org/archives/networkmanager-list/2017-February/msg00001.html
We already use several GCC extenions, like typeof() and
__attribute__((cleanup)). They are too convinient to miss
and every supported compiler must support these.
Currently, gcc and clang does. Maybe other compilers would
support that too, but who knows, nobody seems to test that.
We also already use stdbool.h (C99) and the imported systemd
code is mostly gnu99 too (it's not clear to me, because I don't
find it precisely documented. Certainly it makes use of C99 features
too).
C99/gnu99 has some nice improvements that we no longer should miss
out. For example "flexible array members" or "variable declaration
in init-part of for loop".
It doesn't mean we have to use every obscure (badly supported?)
feature, it means we don't have to forgo features that are well
supported. C99 is 17 years old, I mean, really...
If somebody comes along and ports NM to non-gcc/clang, we can address
bugs about unsupported language features as they surface.
But let's not restrict us to some hypothetical compiler (or language
specification).
Also, NetworkManager is not ported on environment beside Linux.
We don't have to be so considerate about the required build environment.
Gcc is probably the most portable compiler out there. I doubt porting
NetworkManager to *BSD fails due to missing gnu99 features. And if that
causes issues, we should fix them after they happen in practice.
We break the aliasing rules in the code, and thus the flag should
always be enabled to prevent wrong optimizations, even without
--enable-more-warnings.
