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.
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).
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__)); \
^
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.
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.
Some compiler versions don't like to dereference and opaque
pointer for typeof:
gcc 8.3.1-5.1.el8 on RHEL:
In file included from ./src/libnm-glib-aux/nm-macros-internal.h:41,
from ./src/libnm-glib-aux/nm-default-glib.h:68,
from ./src/libnm-glib-aux/nm-default-glib-i18n-lib.h:13,
from ./src/libnm-core-impl/nm-default-libnm-core.h:11,
from src/libnm-core-impl/nm-setting-8021x.c:7:
src/libnm-core-impl/nm-setting-8021x.c: In function '_nm_setting_802_1x_cert_value_to_bytes':
./src/libnm-glib-aux/nm-glib.h:417:16: error: dereferencing pointer to incomplete type 'GBytes' {aka 'struct _GBytes'}
typeof(**_pp) *const _p = *_pp; \
^~~~~
src/libnm-core-impl/nm-setting-8021x.c:361:12: note: in expansion of macro 'g_steal_pointer'
return g_steal_pointer(&bytes);
^~~~~~~~~~~~~~~
./src/libnm-glib-aux/nm-glib.h:417:54: error: initialization of 'int * const' from incompatible pointer type 'GBytes *' {aka 'struct _GBytes *'} [-Werror=incompatible-pointer-types]
typeof(**_pp) *const _p = *_pp; \
^
src/libnm-core-impl/nm-setting-8021x.c:361:12: note: in expansion of macro 'g_steal_pointer'
return g_steal_pointer(&bytes);
^~~~~~~~~~~~~~~
./src/libnm-glib-aux/nm-glib.h:415:6: error: returning 'int * const' from a function with incompatible return type 'GBytes *' {aka 'struct _GBytes *'} [-Werror=incompatible-pointer-types]
({ \
~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
typeof(*(pp)) *const _pp = (pp); \
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
typeof(**_pp) *const _p = *_pp; \
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
_nm_unused const void *const _p_type_check = _p; \
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
\
~
*_pp = NULL; \
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
_p; \
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
})
~~
src/libnm-core-impl/nm-setting-8021x.c:361:12: note: in expansion of macro 'g_steal_pointer'
return g_steal_pointer(&bytes);
^~~~~~~~~~~~~~~
Fixes: 5bc511203e ('all: make nm_steal_pointer() and g_steal_pointer() more typesafe using typeof()')
The previous code would always cast the argument to "void *", and thus
loose some type information.
For example,
gulong variable = 0;
g_steal_pointer(&variable);
would compile, when it shouldn't.
In general, we try to avoid redefining glib macros and headers. But we
really want those extra compile time checks that we can get, so let's do
it.
