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.
- `m4/ax_lib_readline.m4` was already aware of "$with_readline".
Move the entire handling of the parameter inside the AX_LIB_READLINE
macro.
This lets our fork of ax_lib_readline.m4 further deviate from upstream
version, but it's already so different that this is no new problem.
- raise an error if the user requested --with-readline=libreadline|libedit
but the library was not found.
- only allow yes|no for --with-nmcli argument. But still default to
"yes", which will always require one libreadline library to be
detected. In particular, don't automatically disable nmcli if
libreadline is not available, because building without nmcli
should be an explicit choice. That is like before.
- update the "$with_readline" variable for the "auto" case to reflect
what was detected.
This commit provides support for --with-readline=auto|libreadline|libedit|none option
for the configure script.
It allows building the NetworkManager's nmcli tool with libedit instead
of libreadline.
With --with-readline=auto the system looks for any eligible readline library
to use.
Moreover, in this commit all required defines are provided (e.g.
HAVE_EDITLINE_READLINE) to allow correct buil of the code.
The add_history function is available on both - libreadline and libedit.
The read difference between those two libraries is that for libedit the
history_set_history_state is missing.
On that basis one can assess if we do have history from libreadline or
from libedit.
Up till now the AC_LANG_CALL() was just checking if program which
uses the add_history call can be correctly linked without passing
required libraries (e.g. -libreadline to LIBS variable).
This commit fixes this issue as now the add_history can be found as it
is now available in any of passed libs.
The macro always overwrites LIBS and the result is that every binary
links against libreadline.
Instead, save the library to READLINE_LIBS.
See also: 94274c6fcd ('build: fix wrongly linking against libreadline in all applications')
Fixes: af360238be ('m4/ax_lib_readline.m4: Update after running aclocal')
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')
We no longer add these. If you use Emacs, configure it yourself.
Also, due to our "smart-tab" usage the editor anyway does a subpar
job handling our tabs. However, on the upside every user can choose
whatever tab-width he/she prefers. If "smart-tabs" are used properly
(like we do), every tab-width will work.
No manual changes, just ran commands:
F=($(git grep -l -e '-\*-'))
sed '1 { /\/\* *-\*- *[mM]ode.*\*\/$/d }' -i "${F[@]}"
sed '1,4 { /^\(#\|--\|dnl\) *-\*- [mM]ode/d }' -i "${F[@]}"
Check remaining lines with:
git grep -e '-\*-'
The ultimate purpose of this is to cleanup our files and eventually use
SPDX license identifiers. For that, first get rid of the boilerplate lines.
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
g-ir-scanner uses distutils that have an unfortunate misfeature of
inheriting the compiler flags it itself was built with.
This includes the hardening flags that don't work with without
redhat-rpm-build and break with clang every full moon.
A configure check makes it clear about what went wrong in case
introspection is desired, otherwise turns it off.
(Taken from network-manager commit 678890ed0347849990787e8893122a39f95cb708)
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 commonly only allow tabs at the beginning of a line, not
afterwards. The reason for this style is so that the code
looks formated right with tabstop=4 and tabstop=8.
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 want to embed the current commit-id in the ./configure script.
That way the generated ./configure file in the source tarball
references the commit-id from which the tarball was created.
Then, in a second step, a script can check ./configure to find
the parent commit. This is for example done by the 'makerepo.sh'
script.
This is generally useful, and also done by network-manager-applet
and libnl3 projects. Move the function to a separate m4 macro
to reuse it. It should also be re-used in NetworkManager's VPN plugins.
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.
