| Message ID | 20230221232520.14480-3-philmd@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | bulk: Replace assert(0) -> g_assert_not_reached() | expand |
On 2/21/23 13:25, Philippe Mathieu-Daudé wrote: > Since commit 262a69f428 ("osdep.h: Prohibit disabling assert() > in supported builds") we can not build QEMU with NDEBUG (or > G_DISABLE_ASSERT) defined, thus 'assert(0)' always aborts QEMU. > > However some static analyzers / compilers doesn't notice NDEBUG > can't be defined and emit warnings if code is used after an > 'assert(0)' call. See for example commit c0a6665c3c ("target/i386: > Remove compilation errors when -Werror=maybe-uninitialized"). > > Apparently such compiler isn't as clever with G_DISABLE_ASSERT, > so we can silent these warnings by using g_assert_not_reached() > which is easier to read anyway. > > In order to avoid these annoying warnings, add a checkpatch rule > to prohibit 'assert(0)'. Suggest using g_assert_not_reached() > instead. For example when reverting the previous patch we get: > > ERROR: use g_assert_not_reached() instead of assert(0) > #21: FILE: target/ppc/dfp_helper.c:124: > + assert(0); /* cannot get here */ > > ERROR: use g_assert_not_reached() instead of assert(0) > #30: FILE: target/ppc/dfp_helper.c:141: > + assert(0); /* cannot get here */ > > total: 2 errors, 0 warnings, 16 lines checked > > Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org> > --- > scripts/checkpatch.pl | 3 +++ > 1 file changed, 3 insertions(+) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 22/02/2023 00.25, Philippe Mathieu-Daudé wrote: > Since commit 262a69f428 ("osdep.h: Prohibit disabling assert() > in supported builds") we can not build QEMU with NDEBUG (or > G_DISABLE_ASSERT) defined, thus 'assert(0)' always aborts QEMU. > > However some static analyzers / compilers doesn't notice NDEBUG > can't be defined and emit warnings if code is used after an > 'assert(0)' call. See for example commit c0a6665c3c ("target/i386: > Remove compilation errors when -Werror=maybe-uninitialized"). commit c0a6665c3c only uses g_assert_not_reached(), so that looks like a bad example for your asset(0) case? Thomas
On 22/2/23 04:53, Thomas Huth wrote: > On 22/02/2023 00.25, Philippe Mathieu-Daudé wrote: >> Since commit 262a69f428 ("osdep.h: Prohibit disabling assert() >> in supported builds") we can not build QEMU with NDEBUG (or >> G_DISABLE_ASSERT) defined, thus 'assert(0)' always aborts QEMU. >> >> However some static analyzers / compilers doesn't notice NDEBUG >> can't be defined and emit warnings if code is used after an >> 'assert(0)' call. See for example commit c0a6665c3c ("target/i386: >> Remove compilation errors when -Werror=maybe-uninitialized"). > > commit c0a6665c3c only uses g_assert_not_reached(), so that looks like a > bad example for your asset(0) case? Oh right. I'll simply remove [See for example commit xxx ("xxx")].
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 6ecabfb2b5..d768171dcf 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -2982,6 +2982,9 @@ sub process { if ($line =~ /\bsysconf\(_SC_PAGESIZE\)/) { ERROR("use qemu_real_host_page_size() instead of sysconf(_SC_PAGESIZE)\n" . $herecurr); } + if ($line =~ /\b(g_)?assert\(0\)/) { + ERROR("use g_assert_not_reached() instead of assert(0)\n" . $herecurr); + } my $non_exit_glib_asserts = qr{g_assert_cmpstr| g_assert_cmpint| g_assert_cmpuint|
Since commit 262a69f428 ("osdep.h: Prohibit disabling assert() in supported builds") we can not build QEMU with NDEBUG (or G_DISABLE_ASSERT) defined, thus 'assert(0)' always aborts QEMU. However some static analyzers / compilers doesn't notice NDEBUG can't be defined and emit warnings if code is used after an 'assert(0)' call. See for example commit c0a6665c3c ("target/i386: Remove compilation errors when -Werror=maybe-uninitialized"). Apparently such compiler isn't as clever with G_DISABLE_ASSERT, so we can silent these warnings by using g_assert_not_reached() which is easier to read anyway. In order to avoid these annoying warnings, add a checkpatch rule to prohibit 'assert(0)'. Suggest using g_assert_not_reached() instead. For example when reverting the previous patch we get: ERROR: use g_assert_not_reached() instead of assert(0) #21: FILE: target/ppc/dfp_helper.c:124: + assert(0); /* cannot get here */ ERROR: use g_assert_not_reached() instead of assert(0) #30: FILE: target/ppc/dfp_helper.c:141: + assert(0); /* cannot get here */ total: 2 errors, 0 warnings, 16 lines checked Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- scripts/checkpatch.pl | 3 +++ 1 file changed, 3 insertions(+)