diff mbox series

[2/5] scripts/checkpatch.pl: Do not allow assert(0)

Message ID 20230221232520.14480-3-philmd@linaro.org
State Superseded
Headers show
Series bulk: Replace assert(0) -> g_assert_not_reached() | expand

Commit Message

Philippe Mathieu-Daudé Feb. 21, 2023, 11:25 p.m. UTC
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(+)

Comments

Richard Henderson Feb. 22, 2023, 12:08 a.m. UTC | #1
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~
Thomas Huth Feb. 22, 2023, 3:53 a.m. UTC | #2
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
Philippe Mathieu-Daudé Feb. 23, 2023, 2:51 p.m. UTC | #3
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 mbox series

Patch

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|