LTO without assertion enabled, thinks that certain code paths
result in uninitialized code. Technically, it's not wrong, in practice
those are only in cases where we already failed an assertion.
In function 'nm_ip_addr_is_null',
inlined from 'canonicalize_ip_binary' at src/libnm-core-impl/nm-setting-ip-config.c:67:21,
inlined from 'nm_ip_route_set_next_hop_binary' at src/libnm-core-impl/nm-setting-ip-config.c:1062:23:
./src/libnm-glib-aux/nm-inet-utils.h:80:12: error: 'a' may be used uninitialized [-Werror=maybe-uninitialized]
80 | return IN6_IS_ADDR_UNSPECIFIED(&a.addr6);
| ^
src/libnm-core-impl/nm-setting-ip-config.c: In function 'nm_ip_route_set_next_hop_binary':
./src/libnm-glib-aux/nm-inet-utils.h:73:14: note: 'a' declared here
73 | NMIPAddr a;
| ^
Try to workaround that by letting nm_utils_addr_family_to_size() always
return a non-zero size. This is ugly, because in the assertion case fail
we might now also get an additional memory corruption that could have
been avoided by returning zero. However, it probably doesn't matter, because
in this scenario we are already in a bad situation.
Fixes: b02aeaf2f3 ('glib-aux: fix various nm_ip_addr_*() functions for unaligned addresses')
If you do:
nm_assert_addr_family(NMP_OBJECT_CAST_MPTCP_ADDR(obj)->addr_family));
then there are two nested NM_IN_SET() macro invocations. Once,
NMP_OBJECT_CAST_MPTCP_ADDR() checks that the object type is one of
a few selected (using NM_IN_SET()). Then, that is passed to
nm_assert_addr_family(), which checks NM_IN_SET(addr_family, AF_INET,
AF_INET6).
In general, it's easy to end up in a situation like this.
And it mostly works just fine. The only problem was that NM_IN_SET()
uses an internal, local variable "_x". The compiler will emit a very
verbose failure about the shadowed variable:
./src/libnm-std-aux/nm-std-aux.h:802:14: error: declaration of '_x' shadows a previous local [-Werror=shadow]
802 | type _x = (x); \
NM_UNIQ_T() exists for this purpose. Use it. NM_IN_SET() is
popular enough to warrant a special treatment to avoid this pitfall.
The strength of CList is of course to use it as a stack of queue,
and only append/remove from the front/tail.
However, since this is an intrusive list, it can also be useful to
just use it to track elements, and -- when necessary -- sort them
via c_list_sort().
If we have a sorted list, we might want to insert a new element
honoring the sort order. This function achieves that.
We have our own copy of linux kernel headers, and we must never
directly include the corresponding versions from the system.
Avoid that, by only including the clones via "libnm-std-aux/nm-linux-compat.h"
and by including the compat wrapper header before other system headers.
NM_CMP_DIRECT_MEMCMP() gets called by NM_CMP_FIELD_MEMCMP_LEN().
For example, if you want to compare a NMIPAddr, it seems sensible
to call
NM_CMP_FIELD_MEMCMP_LEN(obj1, obj2, addr, nm_utils_addr_family_to_size(obj1->addr_family));
Granted, nm_utils_addr_family_to_size() asserts that addr_family is
either AF_INET or AF_INET6. However, if the assertion fails, we don't
want yet another undefined behavior here and do the sensible thing
about n zero.
In general, for a low-level function that uses memcmp(), it's non
obvious to ensure that the caller does not accidentally invoke undefined
behavior. nm_memcmp() avoids that.
- add code comments explaining some things.
- for NM_CMP_FIELD*() variants have a corresponding NM_CMP_DIRECT*()
macro and use it (aside the "memcmp" variants, which don't translate
directly).
NM_STR_BUF_INIT() and nm_str_buf_init() were pretty much redundant. Drop one of
them.
Usually our pattern is that we don't have functions that return structs.
But NM_STR_BUF_INIT() returns a struct, because it's convenient to use
with
nm_auto_str_buf NMStrBuf strbuf = NM_STR_BUF_INIT(...);
So use that variant instead.
Clang 14 has a new warning "-Wbitwise-instead-of-logical", and it warns
about our usage with NM_IN_SET_SE()/NM_IN_STRSET_SE(). It complains that we
are using '|' with boolean operands. Which is true (and intended), as we bitwise-or
the result of the '==' comparisons.
Work around the warning by casting the operands to "int". Note that
in C, the comparison operators have already a type "int", so this cast
should not result in any changes in the compiled code.
../src/libnm-core-impl/tests/test-general.c:9415:17: error: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
_ASSERT(2, !NM_IN_SET_SE(-1, G(1), G(2)));
~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../src/libnm-std-aux/nm-std-aux.h:800:30: note: expanded from macro 'NM_IN_SET_SE'
#define NM_IN_SET_SE(x, ...) _NM_IN_SET(|, typeof(x), x, __VA_ARGS__)
^
../src/libnm-std-aux/nm-std-aux.h:789:39: note: expanded from macro '_NM_IN_SET'
!!(NM_VA_ARGS_FOREACH(, , op, _NM_IN_SET_OP, __VA_ARGS__)); \
^
../src/libnm-std-aux/nm-std-aux.h:772:20: note: expanded from macro 'NM_VA_ARGS_FOREACH'
op, \
^
note: (skipping 7 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
../src/libnm-glib-aux/nm-macros-internal.h:1603:47: note: expanded from macro '_G_BOOLEAN_EXPR'
#define _G_BOOLEAN_EXPR(expr) NM_BOOLEAN_EXPR(expr)
~~~~~~~~~~~~~~~~^~~~~
../src/libnm-std-aux/nm-std-aux.h:167:62: note: expanded from macro 'NM_BOOLEAN_EXPR'
#define NM_BOOLEAN_EXPR(expr) _NM_BOOLEAN_EXPR_IMPL(NM_UNIQ, expr)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~
../src/libnm-std-aux/nm-std-aux.h:161:13: note: expanded from macro '_NM_BOOLEAN_EXPR_IMPL'
if (expr) \
^~~~
../src/libnm-core-impl/tests/test-general.c:9415:17: note: cast one or both operands to int to silence this warning
../src/libnm-std-aux/nm-std-aux.h:800:30: note: expanded from macro 'NM_IN_SET_SE'
#define NM_IN_SET_SE(x, ...) _NM_IN_SET(|, typeof(x), x, __VA_ARGS__)
^
../src/libnm-std-aux/nm-std-aux.h:789:39: note: expanded from macro '_NM_IN_SET'
!!(NM_VA_ARGS_FOREACH(, , op, _NM_IN_SET_OP, __VA_ARGS__)); \
^
We use clang-format for automatic formatting of our source files.
Since clang-format is actively maintained software, the actual
formatting depends on the used version of clang-format. That is
unfortunate and painful, but really unavoidable unless clang-format
would be strictly bug-compatible.
So the version that we must use is from the current Fedora release, which
is also tested by our gitlab-ci. Previously, we were using Fedora 34 with
clang-tools-extra-12.0.1-1.fc34.x86_64.
As Fedora 35 comes along, we need to update our formatting as Fedora 35
comes with version "13.0.0~rc1-1.fc35".
An alternative would be to freeze on version 12, but that has different
problems (like, it's cumbersome to rebuild clang 12 on Fedora 35 and it
would be cumbersome for our developers which are on Fedora 35 to use a
clang that they cannot easily install).
The (differently painful) solution is to reformat from time to time, as we
switch to a new Fedora (and thus clang) version.
Usually we would expect that such a reformatting brings minor changes.
But this time, the changes are huge. That is mentioned in the release
notes [1] as
Makes PointerAligment: Right working with AlignConsecutiveDeclarations. (Fixes https://llvm.org/PR27353)
[1] https://releases.llvm.org/13.0.0/tools/clang/docs/ReleaseNotes.html#clang-format
LLD 13 adds -z start-stop-gc and makes it the default, resulting in:
CCLD src/core/NetworkManager-all-sym
ld.lld: error: undefined symbol: __stop_connection_defaults
>>> referenced by nm-config.c:0 (src/core/nm-config.c:0)
>>> libNetworkManager_la-nm-config.o:(read_config) in archive src/core/.libs/libNetworkManager.a
>>> referenced by nm-config-data.c:1598 (src/core/nm-config-data.c:1598)
>>> libNetworkManager_la-nm-config-data.o:(nm_config_data_get_connection_default) in archive src/core/.libs/libNetworkManager.a
>>> referenced by nm-config-data.c:0 (src/core/nm-config-data.c:0)
>>> libNetworkManager_la-nm-config-data.o:(nm_config_data_get_connection_default) in archive src/core/.libs/libNetworkManager.a
ld.lld: error: undefined symbol: __start_connection_defaults
>>> referenced by nm-config.c:0 (src/core/nm-config.c:0)
>>> libNetworkManager_la-nm-config.o:(read_config) in archive src/core/.libs/libNetworkManager.a
>>> referenced by nm-config.c:0 (src/core/nm-config.c:0)
>>> libNetworkManager_la-nm-config.o:(read_config) in archive src/core/.libs/libNetworkManager.a
>>> referenced by nm-config.c:0 (src/core/nm-config.c:0)
>>> libNetworkManager_la-nm-config.o:(read_config) in archive src/core/.libs/libNetworkManager.a
>>> referenced 2 more times
clang: error: linker command failed with exit code 1 (use -v to see invocation)
Add __attribute__((__retain__)) to prevent GC of the connection
defaults.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1008
We use the cleanup attribute heavily. It's useful for deferring
deallocation. For example, we have code like:
gs_unref_object NMBluezManager *self_keep_alive = g_object_ref(self);
where we don't use the variable otherwise, except for owning (and
freeing) the reference. This already lead to a compiler warning about
unused variable, which we would workaround with
_nm_unused gs_unref_object NMBluezManager *self_keep_alive = g_object_ref(self);
With clang 13.0.0~rc1-1.fc35, this got worse. Now for example also
static inline void
nm_strvarray_set_strv(GArray **array, const char *const *strv)
{
gs_unref_array GArray *array_old = NULL;
array_old = g_steal_pointer(array);
if (!strv || !strv[0])
return;
nm_strvarray_ensure(array);
for (; strv[0]; strv++)
nm_strvarray_add(*array, strv[0]);
}
leads to a warning
./src/libnm-glib-aux/nm-shared-utils.h:3078:28: error: variable array_old set but not used [-Werror,-Wunused-but-set-variable]
gs_unref_array GArray *array_old = NULL;
^
This is really annoying. We don't want to plaster our code with _nm_unused,
because that might hide actual issues. But we also want to keep using this
pattern and need to avoid the warning.
A problem is also that GCC usually does not warn about truly unused
variables with cleanup attribute. Clang was very useful here to flag
such variables. But now clang warns about cases which are no bugs, which
is a problem. So this does loose some useful warnings. On the other hand,
a truly unused variable (with cleanup attribute) is ugly, but not an actual
problem.
Now, with clang 13, automatically mark nm_auto() variables as _nm_unused
as workaround.
We have a copy of a few linux user space headers in `src/linux-headers`.
The idea is that we want to use recent kernel API, and not depend on the
kernel UAPI headers installed on the build system (and not need to
workaround that).
However, we may not be able to simply compile them, because they too
have dependencies. For example,
../src/linux-headers/ethtool.h:1389:2: error: implicit declaration of function '__KERNEL_DIV_ROUND_UP' [-Werror=implicit-function-declaration]
__u32 queue_mask[__KERNEL_DIV_ROUND_UP(MAX_NUM_QUEUE, 32)];
^
As workaround, don't include headers from "linux-headers" directly,
but only include the new "libnm-std-aux/nm-linux-compat.h" adapter
header, which tries to solve these incompatibilities.
Fixes: 34d48d2596 ('platform: clear all BASE types when setting advertised modes for ethernet autoneg')
The formatting produced by clang-format depends on the version of the
tool. The version that we use is the one of the current Fedora release.
Fedora 34 recently updated clang (and clang-tools-extra) from version
12.0.0 to 12.0.1. This brings some changes.
Update the formatting.
Like nm_utils_addr_family_to_char(), but gives a different treatment to
AF_UNSPEC to return "" instead of 'X'. As such, it also needs to
return a string and not a char.
We use NM_PTRARRAY_LEN(), and I find it a bit ugly that a macro does so
much. Maybe, it's better to have it as a function.
But the macro currently lives in "libnm-std-aux/nm-std-aux.h", which
is header-only. To add it to a C source file, we would have to move
it to another header, but "libnm-std-aux/nm-std-aux.h" is nice because
it gets included by default already.
Keep it in "libnm-std-aux/nm-std-aux.h", but implement it as an inline
function.
The macro now only does (as before) some type checking shenanigans to ensure
that the argument is a pointer to pointers.
In practice, there is probably very little difference compared to
the macro before, likely the code will anyway be inlined.
Replace NM_STATIC_ASSERT_EXPR() by NM_STATIC_ASSERT_EXPR_1() and
NM_STATIC_ASSERT_EXPR_VOID(). NM_STATIC_ASSERT_EXPR_VOID() can be
used as an expression that returns void (that is, a simple statement).
On the other hand, NM_STATIC_ASSERT_EXPR_1() itself retuns
a compile time constant of int value 1. The latter is useful, because
we can use the 1 to combine static assertions in expressions that
are themself compile time constants, like
#define STATIC_CHECK_AND_GET(cond, value) \
(NM_STATIC_ASSERT_EXPR_1(cond) ? (value) : (value))
This is itself a compile time constant if value is a compile
time constant. Also, it does the compile time check that "cond"
is true.
We have variadic macros like NM_UTILS_ENUM2STR() that create a switch
statement. Their implementation relies on the way how __VA_ARGS__
gets expanded to a comma separated list. But that implementation is
not great. Let's instead add (and later use) NM_VA_ARGS_JOIN() which
can join variadic arguments by a configurable separator.
