th/static_asserts #40

thom311 · 2017-12-11T20:10:57Z

Compile NM with -Wvla and don't use variable length arrays.

It makes static-asserts against a non-compile-time constant work. Otherwise, the assertion is silently ignored.

bengal · 2017-12-14T15:10:05Z

build: add "-Wvla" to warn about uses of variable-length arrays

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.

I struggle to understand how this works; can you make an example of where G_STATIC_ASSERT is ignored due to VLAs?

thom311 · 2017-12-14T17:38:06Z

int a = 0;

G_STATIC_ASSERT (a != 0);

both expressions are not static, hence they compile to nothing.

Sometimes it's less obvious that an expression is not static, especially when you use G_STATIC_ASSERT in a macro

#define XX(arg) \
    ({ \
        G_STATIC_ASSERT (sizeof (arg) == 5); \
        memset ((arg), 0, 5); \
    })
{
    int LEN = 3;
    char xx[LEN];
    XX (xx);
}

NM_FLAGS_HAS() uses a static-assert that the second argument is a single flag (power of two). With a single flag, NM_FLAGS_HAS(), NM_FLAGS_ANY() and NM_FLAGS_ALL() are all identical. The second argument must be a compile time constant, and if that is not the case, one must not use NM_FLAGS_HAS(). Use NM_FLAGS_ANY() in these cases.

Let's compile with -Wvla, so that G_STATIC_ASSERT() is really static. But then we cannot use variable length arrays.

The expression is not const. Use a run-time assert instead

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.

When NM is restarted and a new object-manager is created, ensure that signal handlers are disconnected from the old one. Fixes the following: assertion failed: (object_manager == priv->object_manager) #0 __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56 #1 __GI_abort () at abort.c:90 #2 g_assertion_message (domain=domain@entry=0x7fcac0b845ff "libnm", file=file@entry=0x7fcac0b84c95 "libnm/nm-client.c", line=line@entry=2506, func=func@entry=0x7fcac0b863a0 <__func__.34881> "name_owner_changed", message=message@entry=0x7fcac20b05f0 "assertion failed: (object_manager == priv->object_manager)") at gtestutils.c:2429 #3 g_assertion_message_expr (domain=domain@entry=0x7fcac0b845ff "libnm", file=file@entry=0x7fcac0b84c95 "libnm/nm-client.c", line=line@entry=2506, func=func@entry=0x7fcac0b863a0 <__func__.34881> "name_owner_changed", expr=expr@entry=0x7fcac0b856a0 "object_manager == priv->object_manager") at gtestutils.c:2444 #4 name_owner_changed (object=<optimized out>, pspec=<optimized out>, user_data=0x7fcac204e480) at libnm/nm-client.c:2506 #8 <emit signal notify:name-owner on instance 0x7fcac2053c60 [GDBusObjectManagerClient]> (instance=instance@entry=0x7fcac2053c60, signal_id=<optimized out>, detail=<optimized out>) at gsignal.c:3439 #5 g_closure_invoke (closure=0x7fcac20af390, return_value=return_value@entry=0x0, n_param_values=2, param_values=param_values@entry=0x7ffde58d9ec0, invocation_hint=invocation_hint@entry=0x7ffde58d9e60) at gclosure.c:801 #6 signal_emit_unlocked_R (node=node@entry=0x7fcac2052090, detail=detail@entry=185, instance=instance@entry=0x7fcac2053c60, emission_return=emission_return@entry=0x0, instance_and_params=instance_and_params@entry=0x7ffde58d9ec0) at gsignal.c:3627 #7 g_signal_emit_valist (instance=<optimized out>, signal_id=<optimized out>, detail=<optimized out>, var_args=var_args@entry=0x7ffde58da050) at gsignal.c:3383 #9 g_object_dispatch_properties_changed (object=0x7fcac2053c60 [GDBusObjectManagerClient], n_pspecs=<optimized out>, pspecs=<optimized out>) at gobject.c:1061 #10 g_object_notify (pspec=<optimized out>, object=0x7fcac2053c60 [GDBusObjectManagerClient]) at gobject.c:1155 #11 g_object_notify (object=object@entry=0x7fcac2053c60 [GDBusObjectManagerClient], property_name=property_name@entry=0x7fcabe9d2b29 "name-owner") at gobject.c:1202 #12 on_notify_g_name_owner (object=<optimized out>, pspec=<optimized out>, user_data=0x7fcac2053c60) at gdbusobjectmanagerclient.c:1262 #16 <emit signal notify:g-name-owner on instance 0x7fcaa8004440 [GDBusProxy]> (instance=instance@entry=0x7fcaa8004440, signal_id=<optimized out>, detail=<optimized out>) at gsignal.c:3439 #13 g_closure_invoke (closure=0x7fcaa80194f0, return_value=return_value@entry=0x0, n_param_values=2, param_values=param_values@entry=0x7ffde58da370, invocation_hint=invocation_hint@entry=0x7ffde58da310) at gclosure.c:801 #14 signal_emit_unlocked_R (node=node@entry=0x7fcac2052090, detail=detail@entry=299, instance=instance@entry=0x7fcaa8004440, emission_return=emission_return@entry=0x0, instance_and_params=instance_and_params@entry=0x7ffde58da370) at gsignal.c:3627 #15 g_signal_emit_valist (instance=<optimized out>, signal_id=<optimized out>, detail=<optimized out>, var_args=var_args@entry=0x7ffde58da500) at gsignal.c:3383 #17 g_object_dispatch_properties_changed (object=0x7fcaa8004440 [GDBusProxy], n_pspecs=<optimized out>, pspecs=<optimized out>) at gobject.c:1061 #18 g_object_notify (pspec=<optimized out>, object=0x7fcaa8004440 [GDBusProxy]) at gobject.c:1155 #19 g_object_notify (object=object@entry=0x7fcaa8004440 [GDBusProxy], property_name=property_name@entry=0x7fcabe9d2b27 "g-name-owner") at gobject.c:1202 #20 on_name_owner_changed (connection=<optimized out>, sender_name=<optimized out>, object_path=<optimized out>, interface_name=<optimized out>, signal_name=<optimized out>, parameters=<optimized out>, user_data=0x7fcaa8015b50) at gdbusproxy.c:1353 #21 emit_signal_instance_in_idle_cb (data=0x7fcaa40ff400) at gdbusconnection.c:3701 #22 g_main_context_dispatch (context=0x7fcac204eea0) at gmain.c:3152 #23 g_main_context_dispatch (context=context@entry=0x7fcac204eea0) at gmain.c:3767 #24 g_main_context_iterate (context=context@entry=0x7fcac204eea0, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at gmain.c:3838 #25 g_main_context_iteration (context=0x7fcac204eea0, context@entry=0x0, may_block=may_block@entry=1) at gmain.c:3899 #26 nmc_readline_helper (prompt=prompt@entry=0x7fcac2087f60 "The connection is not saved. Do you really want to quit? (yes/no) [no] ") at clients/cli/common.c:986 #27 nmc_readline (prompt_fmt=<optimized out>) at clients/cli/common.c:1055 #28 confirm_quit () at clients/cli/connections.c:6459 #29 do_connection_edit (connection_type=<optimized out>, connection=0x7fcac208eca0, nmc=0x7fcac12a3a60 <nm_cli>) at clients/cli/connections.c:7611 #30 do_connection_edit (nmc=0x7fcac12a3a60 <nm_cli>, argc=1, argv=0x7ffde58db0b0) at clients/cli/connections.c:7948 #31 call_cmd (nmc=0x7fcac12a3a60 <nm_cli>, simple=0x7fcac2052490 [GSimpleAsyncResult], cmd=0x7fcac1291ae0 <connection_cmds+128>, argc=2, argv=0x7ffde58db0a8) at clients/cli/common.c:1315 #32 got_client (source_object=<optimized out>, res=<optimized out>, user_data=0x7fcac2051830) at clients/cli/common.c:1297 #33 g_simple_async_result_complete (simple=0x7fcac2052500 [GSimpleAsyncResult]) at gsimpleasyncresult.c:801 #34 client_inited (source=0x7fcac204e480 [NMClient], result=0x7fcac20565f0, user_data=0x7fcac2052500) at libnm/nm-client.c:1839 #35 g_simple_async_result_complete (simple=0x7fcac20565f0 [GSimpleAsyncResult]) at gsimpleasyncresult.c:801 #36 init_async_complete (init_data=init_data@entry=0x7fcac2046820) at libnm/nm-client.c:2339 #37 async_inited_obj_nm (init_data=0x7fcac2046820) at libnm/nm-client.c:2337 #38 async_inited_obj_nm (object=0x7fcac2073080 [NMRemoteConnection], result=0x7fcac2089ed0, user_data=0x7fcac2046820) at libnm/nm-client.c:2357 #39 g_simple_async_result_complete (simple=0x7fcac2089ed0 [GSimpleAsyncResult]) at gsimpleasyncresult.c:801 #40 init_async_parent_inited (error=0x0, init_data=0x7fcaa408a0f0) at libnm/nm-remote-connection.c:677 #41 init_async_parent_inited (source=<optimized out>, result=<optimized out>, user_data=0x7fcaa408a0f0) at libnm/nm-remote-connection.c:689 #42 g_simple_async_result_complete (simple=0x7fcac2089c30 [GSimpleAsyncResult]) at gsimpleasyncresult.c:801 #43 complete_in_idle_cb (data=<optimized out>) at gsimpleasyncresult.c:813 #44 g_main_context_dispatch (context=0x7fcac204eea0) at gmain.c:3152 #45 g_main_context_dispatch (context=context@entry=0x7fcac204eea0) at gmain.c:3767 #46 g_main_context_iterate (context=0x7fcac204eea0, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at gmain.c:3838 #47 g_main_loop_run (loop=0x7fcac2045ab0) at gmain.c:4032 #48 main (argc=<optimized out>, argv=<optimized out>) at clients/cli/nmcli.c:642 https://bugzilla.redhat.com/show_bug.cgi?id=1471245 (cherry picked from commit 7758071)

lkundrak force-pushed the th/static-asserts branch from 7ddc8c9 to 635693d Compare December 12, 2017 14:30

thom311 force-pushed the th/static-asserts branch from 635693d to 927fbe9 Compare December 12, 2017 14:38

lkundrak force-pushed the th/static-asserts branch from 927fbe9 to 8d45b95 Compare December 12, 2017 14:50

thom311 force-pushed the th/static-asserts branch 3 times, most recently from 8d133bc to 6ed8faa Compare December 12, 2017 15:48

lkundrak force-pushed the th/static-asserts branch 2 times, most recently from e661c94 to 7a26a83 Compare December 13, 2017 15:50

thom311 force-pushed the th/static-asserts branch 2 times, most recently from 69c7532 to 14eb06b Compare December 14, 2017 11:36

lkundrak force-pushed the th/static-asserts branch 2 times, most recently from b0b589e to 5014edc Compare December 14, 2017 14:20

thom311 added 6 commits December 15, 2017 11:48

core: don't use variable length arrays

ea1f77f

Let's compile with -Wvla, so that G_STATIC_ASSERT() is really static. But then we cannot use variable length arrays.

tests: fix invalid static-asserts with non-const expression

08d41ad

The expression is not const. Use a run-time assert instead

core: don't use const integers where const expression is needed

96ae930

shared: add static assert for nm_g_slice_free_fcn() argument

974501f

lkundrak force-pushed the th/static-asserts branch from 5014edc to 974501f Compare December 15, 2017 10:50

lkundrak merged commit 974501f into master Dec 15, 2017

lkundrak deleted the th/static-asserts branch December 15, 2017 11:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

th/static_asserts #40

th/static_asserts #40

Uh oh!

thom311 commented Dec 11, 2017

Uh oh!

bengal commented Dec 14, 2017

Uh oh!

thom311 commented Dec 14, 2017 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

th/static_asserts #40

th/static_asserts #40

Uh oh!

Conversation

thom311 commented Dec 11, 2017

Uh oh!

bengal commented Dec 14, 2017

Uh oh!

thom311 commented Dec 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

thom311 commented Dec 14, 2017 •

edited

Loading