[PATCH 0/5] bulk: Replace assert(0) -> g_assert_not_reached()

[PATCH 0/5] bulk: Replace assert(0) -> g_assert_not_reached()

[Philippe Mathieu-Daudé]

Save contributors to post a patch each time clang
produce a -Werror=maybe-uninitialized warning on
assert(0). Replace by g_assert_not_reached()() and
prohibit '[g_]assert(0)'. Remove NDEBUG.

Philippe Mathieu-Daudé (4):
  scripts/checkpatch.pl: Do not allow assert(0)
  bulk: Replace [g_]assert(0) -> g_assert_not_reached()
  block/vvfat: Remove pointless check of NDEBUG
  hw: Remove mentions of NDEBUG

Pierrick Bouvier (1):
  target/ppc: fix warning with clang-15

 block/vvfat.c                       |   3 -
 docs/spin/aio_notify_accept.promela |   6 +-
 docs/spin/aio_notify_bug.promela    |   6 +-
 hw/acpi/aml-build.c                 |   3 +-
 hw/arm/highbank.c                   |   2 +-
 hw/char/avr_usart.c                 |   2 +-
 hw/core/numa.c                      |   2 +-
 hw/net/i82596.c                     |   2 +-
 hw/scsi/mptsas.c                    |   2 -
 hw/virtio/virtio.c                  |   2 -
 hw/watchdog/watchdog.c              |   2 +-
 migration/migration-hmp-cmds.c      |   2 +-
 migration/postcopy-ram.c            |  21 ++----
 migration/ram.c                     |   8 +--
 qobject/qlit.c                      |   2 +-
 qobject/qnum.c                      |  12 ++--
 scripts/checkpatch.pl               |   3 +
 softmmu/rtc.c                       |   2 +-
 target/mips/sysemu/physaddr.c       |   3 +-
 target/mips/tcg/msa_helper.c        | 104 ++++++++++++++--------------
 target/ppc/dfp_helper.c             |  12 ++--
 target/ppc/mmu_helper.c             |   2 +-
 tests/qtest/ipmi-bt-test.c          |   2 +-
 tests/qtest/ipmi-kcs-test.c         |   4 +-
 tests/qtest/rtl8139-test.c          |   2 +-
 25 files changed, 96 insertions(+), 115 deletions(-)

-- 
2.38.1

[PATCH 1/5] target/ppc: fix warning with clang-15

[Philippe Mathieu-Daudé]

From: Pierrick Bouvier <pierrick.bouvier@linaro.org>

When compiling for windows-arm64 using clang-15, it reports a sometimes
uninitialized variable. This seems to be a false positive, as a default
case guards switch expressions, preventing to return an uninitialized
value, but clang seems unhappy with assert(0) definition.

Change code to g_assert_not_reached() fix the warning.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-Id: <20230221153006.20300-5-pierrick.bouvier@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/ppc/dfp_helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/ppc/dfp_helper.c b/target/ppc/dfp_helper.c
index cc024316d5..5967ea07a9 100644
--- a/target/ppc/dfp_helper.c
+++ b/target/ppc/dfp_helper.c
@@ -121,7 +121,7 @@ static void dfp_set_round_mode_from_immediate(uint8_t r, uint8_t rmc,
         case 3: /* use FPSCR rounding mode */
             return;
         default:
-            assert(0); /* cannot get here */
+            g_assert_not_reached();
         }
     } else { /* r == 1 */
         switch (rmc & 3) {
@@ -138,7 +138,7 @@ static void dfp_set_round_mode_from_immediate(uint8_t r, uint8_t rmc,
             rnd = DEC_ROUND_HALF_DOWN;
             break;
         default:
-            assert(0); /* cannot get here */
+            g_assert_not_reached();
         }
     }
     decContextSetRounding(&dfp->context, rnd);
-- 
2.38.1

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

[Philippe Mathieu-Daudé]

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(+)

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|
-- 
2.38.1

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

[Richard Henderson]

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~

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

[Thomas Huth]

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

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

[Philippe Mathieu-Daudé]

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")].

[PATCH 3/5] bulk: Replace [g_]assert(0) -> g_assert_not_reached()

[Philippe Mathieu-Daudé]

In order to avoid warnings such commit c0a6665c3c ("target/i386:
Remove compilation errors when -Werror=maybe-uninitialized"),
replace all assert(0) and g_assert(0) by g_assert_not_reached().

Remove any code following g_assert_not_reached().

See previous commit for rationale.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 docs/spin/aio_notify_accept.promela |   6 +-
 docs/spin/aio_notify_bug.promela    |   6 +-
 hw/acpi/aml-build.c                 |   3 +-
 hw/arm/highbank.c                   |   2 +-
 hw/char/avr_usart.c                 |   2 +-
 hw/core/numa.c                      |   2 +-
 hw/net/i82596.c                     |   2 +-
 hw/watchdog/watchdog.c              |   2 +-
 migration/migration-hmp-cmds.c      |   2 +-
 migration/postcopy-ram.c            |  21 ++----
 migration/ram.c                     |   8 +--
 qobject/qlit.c                      |   2 +-
 qobject/qnum.c                      |  12 ++--
 softmmu/rtc.c                       |   2 +-
 target/mips/sysemu/physaddr.c       |   3 +-
 target/mips/tcg/msa_helper.c        | 104 ++++++++++++++--------------
 target/ppc/dfp_helper.c             |   8 +--
 target/ppc/mmu_helper.c             |   2 +-
 tests/qtest/ipmi-bt-test.c          |   2 +-
 tests/qtest/ipmi-kcs-test.c         |   4 +-
 tests/qtest/rtl8139-test.c          |   2 +-
 21 files changed, 91 insertions(+), 106 deletions(-)

diff --git a/docs/spin/aio_notify_accept.promela b/docs/spin/aio_notify_accept.promela
index 9cef2c955d..f929d30328 100644
--- a/docs/spin/aio_notify_accept.promela
+++ b/docs/spin/aio_notify_accept.promela
@@ -118,7 +118,7 @@ accept_if_req_not_eventually_false:
     if
         :: req -> goto accept_if_req_not_eventually_false;
     fi;
-    assert(0);
+    g_assert_not_reached();
 }
 
 #else
@@ -141,12 +141,12 @@ accept_if_event_not_eventually_true:
         :: !event && notifier_done  -> do :: true -> skip; od;
         :: !event && !notifier_done -> goto accept_if_event_not_eventually_true;
     fi;
-    assert(0);
+    g_assert_not_reached();
 
 accept_if_event_not_eventually_false:
     if
         :: event     -> goto accept_if_event_not_eventually_false;
     fi;
-    assert(0);
+    g_assert_not_reached();
 }
 #endif
diff --git a/docs/spin/aio_notify_bug.promela b/docs/spin/aio_notify_bug.promela
index b3bfca1ca4..ce6f5177ed 100644
--- a/docs/spin/aio_notify_bug.promela
+++ b/docs/spin/aio_notify_bug.promela
@@ -106,7 +106,7 @@ accept_if_req_not_eventually_false:
     if
         :: req -> goto accept_if_req_not_eventually_false;
     fi;
-    assert(0);
+    g_assert_not_reached();
 }
 
 #else
@@ -129,12 +129,12 @@ accept_if_event_not_eventually_true:
         :: !event && notifier_done  -> do :: true -> skip; od;
         :: !event && !notifier_done -> goto accept_if_event_not_eventually_true;
     fi;
-    assert(0);
+    g_assert_not_reached();
 
 accept_if_event_not_eventually_false:
     if
         :: event     -> goto accept_if_event_not_eventually_false;
     fi;
-    assert(0);
+    g_assert_not_reached();
 }
 #endif
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index ea331a20d1..97dfdcdd2f 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -534,8 +534,7 @@ void aml_append(Aml *parent_ctx, Aml *child)
     case AML_NO_OPCODE:
         break;
     default:
-        assert(0);
-        break;
+        g_assert_not_reached();
     }
     build_append_array(parent_ctx->buf, buf);
     build_free_array(buf);
diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
index f12aacea6b..fc212195ca 100644
--- a/hw/arm/highbank.c
+++ b/hw/arm/highbank.c
@@ -198,7 +198,7 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
         machine->cpu_type = ARM_CPU_TYPE_NAME("cortex-a15");
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     for (n = 0; n < smp_cpus; n++) {
diff --git a/hw/char/avr_usart.c b/hw/char/avr_usart.c
index 5bcf9db0b7..e738a2ca97 100644
--- a/hw/char/avr_usart.c
+++ b/hw/char/avr_usart.c
@@ -86,7 +86,7 @@ static void update_char_mask(AVRUsartState *usart)
         usart->char_mask = 0b11111111;
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 }
 
diff --git a/hw/core/numa.c b/hw/core/numa.c
index d8d36b16d8..26ef02792a 100644
--- a/hw/core/numa.c
+++ b/hw/core/numa.c
@@ -381,7 +381,7 @@ void parse_numa_hmat_lb(NumaState *numa_state, NumaHmatLBOptions *node,
         }
         lb_data.data = node->bandwidth;
     } else {
-        assert(0);
+        g_assert_not_reached();
     }
 
     g_array_append_val(hmat_lb->list, lb_data);
diff --git a/hw/net/i82596.c b/hw/net/i82596.c
index ec21e2699a..eda0f586fb 100644
--- a/hw/net/i82596.c
+++ b/hw/net/i82596.c
@@ -285,7 +285,7 @@ static void command_loop(I82596State *s)
         case CmdDump:
         case CmdDiagnose:
             printf("FIXME Command %d !!\n", cmd & 7);
-            assert(0);
+            g_assert_not_reached();
         }
 
         /* update status */
diff --git a/hw/watchdog/watchdog.c b/hw/watchdog/watchdog.c
index 955046161b..d0ce3c4ac5 100644
--- a/hw/watchdog/watchdog.c
+++ b/hw/watchdog/watchdog.c
@@ -85,7 +85,7 @@ void watchdog_perform_action(void)
         break;
 
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 }
 
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index ef25bc8929..4e07c95801 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -602,7 +602,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
                    "through QMP");
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     if (err) {
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index f54f44d899..59c8032a21 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -1347,49 +1347,42 @@ int postcopy_ram_incoming_init(MigrationIncomingState *mis)
 
 int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
 {
-    assert(0);
-    return -1;
+    g_assert_not_reached();
 }
 
 int postcopy_ram_prepare_discard(MigrationIncomingState *mis)
 {
-    assert(0);
-    return -1;
+    g_assert_not_reached();
 }
 
 int postcopy_request_shared_page(struct PostCopyFD *pcfd, RAMBlock *rb,
                                  uint64_t client_addr, uint64_t rb_offset)
 {
-    assert(0);
-    return -1;
+    g_assert_not_reached();
 }
 
 int postcopy_ram_incoming_setup(MigrationIncomingState *mis)
 {
-    assert(0);
-    return -1;
+    g_assert_not_reached();
 }
 
 int postcopy_place_page(MigrationIncomingState *mis, void *host, void *from,
                         RAMBlock *rb)
 {
-    assert(0);
-    return -1;
+    g_assert_not_reached();
 }
 
 int postcopy_place_page_zero(MigrationIncomingState *mis, void *host,
                         RAMBlock *rb)
 {
-    assert(0);
-    return -1;
+    g_assert_not_reached();
 }
 
 int postcopy_wake_shared(struct PostCopyFD *pcfd,
                          uint64_t client_addr,
                          RAMBlock *rb)
 {
-    assert(0);
-    return -1;
+    g_assert_not_reached();
 }
 #endif
 
diff --git a/migration/ram.c b/migration/ram.c
index 96e8a19a58..93f5a4a416 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2061,19 +2061,17 @@ bool ram_write_tracking_available(void)
 
 bool ram_write_tracking_compatible(void)
 {
-    assert(0);
-    return false;
+    g_assert_not_reached();
 }
 
 int ram_write_tracking_start(void)
 {
-    assert(0);
-    return -1;
+    g_assert_not_reached();
 }
 
 void ram_write_tracking_stop(void)
 {
-    assert(0);
+    g_assert_not_reached();
 }
 #endif /* defined(__linux__) */
 
diff --git a/qobject/qlit.c b/qobject/qlit.c
index be8332136c..a62865b642 100644
--- a/qobject/qlit.c
+++ b/qobject/qlit.c
@@ -118,7 +118,7 @@ QObject *qobject_from_qlit(const QLitObject *qlit)
     case QTYPE_QBOOL:
         return QOBJECT(qbool_from_bool(qlit->value.qbool));
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     return NULL;
diff --git a/qobject/qnum.c b/qobject/qnum.c
index 2bbeaedc7b..dd8ea49565 100644
--- a/qobject/qnum.c
+++ b/qobject/qnum.c
@@ -85,8 +85,7 @@ bool qnum_get_try_int(const QNum *qn, int64_t *val)
         return false;
     }
 
-    assert(0);
-    return false;
+    g_assert_not_reached();
 }
 
 /**
@@ -123,8 +122,7 @@ bool qnum_get_try_uint(const QNum *qn, uint64_t *val)
         return false;
     }
 
-    assert(0);
-    return false;
+    g_assert_not_reached();
 }
 
 /**
@@ -156,8 +154,7 @@ double qnum_get_double(QNum *qn)
         return qn->u.dbl;
     }
 
-    assert(0);
-    return 0.0;
+    g_assert_not_reached();
 }
 
 char *qnum_to_string(QNum *qn)
@@ -172,8 +169,7 @@ char *qnum_to_string(QNum *qn)
         return g_strdup_printf("%.17g", qn->u.dbl);
     }
 
-    assert(0);
-    return NULL;
+    g_assert_not_reached();
 }
 
 /**
diff --git a/softmmu/rtc.c b/softmmu/rtc.c
index f7114bed7d..a2145da23d 100644
--- a/softmmu/rtc.c
+++ b/softmmu/rtc.c
@@ -63,7 +63,7 @@ static time_t qemu_ref_timedate(QEMUClockType clock)
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
     return value;
 }
diff --git a/target/mips/sysemu/physaddr.c b/target/mips/sysemu/physaddr.c
index 2970df8a09..05990aa5bb 100644
--- a/target/mips/sysemu/physaddr.c
+++ b/target/mips/sysemu/physaddr.c
@@ -70,8 +70,7 @@ static int is_seg_am_mapped(unsigned int am, bool eu, int mmu_idx)
         /* is this AM mapped in current execution mode */
         return ((adetlb_mask << am) < 0);
     default:
-        assert(0);
-        return TLBRET_BADADDR;
+        g_assert_not_reached();
     };
 }
 
diff --git a/target/mips/tcg/msa_helper.c b/target/mips/tcg/msa_helper.c
index 736283e2af..29b31d70fe 100644
--- a/target/mips/tcg/msa_helper.c
+++ b/target/mips/tcg/msa_helper.c
@@ -5333,7 +5333,7 @@ void helper_msa_shf_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
     msa_move_v(pwd, pwx);
 }
@@ -5368,7 +5368,7 @@ void helper_msa_ ## helper ## _df(CPUMIPSState *env, uint32_t df,       \
         }                                                               \
         break;                                                          \
     default:                                                            \
-        assert(0);                                                      \
+        g_assert_not_reached();                                         \
     }                                                                   \
 }
 
@@ -5413,7 +5413,7 @@ void helper_msa_ldi_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
         }
        break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 }
 
@@ -5461,7 +5461,7 @@ void helper_msa_ ## helper ## _df(CPUMIPSState *env, uint32_t df, uint32_t wd, \
         }                                                               \
         break;                                                          \
     default:                                                            \
-        assert(0);                                                      \
+        g_assert_not_reached();                                         \
     }                                                                   \
 }
 
@@ -5511,7 +5511,7 @@ void helper_msa_ ## helper ## _df(CPUMIPSState *env, uint32_t df,       \
         }                                                               \
         break;                                                          \
     default:                                                            \
-        assert(0);                                                      \
+        g_assert_not_reached();                                         \
     }                                                                   \
 }
 
@@ -5557,7 +5557,7 @@ static inline void msa_sld_df(uint32_t df, wr_t *pwd,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 }
 
@@ -5632,7 +5632,7 @@ void helper_msa_ ## func ## _df(CPUMIPSState *env, uint32_t df,         \
         pwd->d[1] = msa_ ## func ## _df(df, pws->d[1], pwt->d[1]);      \
         break;                                                          \
     default:                                                            \
-        assert(0);                                                      \
+        g_assert_not_reached();                                         \
     }                                                                   \
 }
 
@@ -5771,7 +5771,7 @@ void helper_msa_ ## func ## _df(CPUMIPSState *env, uint32_t df, uint32_t wd,  \
         pwd->d[1] = msa_ ## func ## _df(df, pwd->d[1], pws->d[1], pwt->d[1]); \
         break;                                                                \
     default:                                                                  \
-        assert(0);                                                            \
+        g_assert_not_reached();                                               \
     }                                                                         \
 }
 
@@ -5811,7 +5811,7 @@ static inline void msa_splat_df(uint32_t df, wr_t *pwd,
         }
        break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 }
 
@@ -5869,7 +5869,7 @@ void helper_msa_##FUNC(CPUMIPSState *env, uint32_t df, uint32_t wd, \
         MSA_LOOP_D;                                                 \
         break;                                                      \
     default:                                                        \
-        assert(0);                                                  \
+        g_assert_not_reached();                                     \
     }                                                               \
     msa_move_v(pwd, pwx);                                           \
 }
@@ -6090,7 +6090,7 @@ void helper_msa_insve_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
         pwd->d[n] = (int64_t)pws->d[0];
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 }
 
@@ -6150,7 +6150,7 @@ void helper_msa_fill_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
         }
        break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 }
 
@@ -6565,7 +6565,7 @@ static inline void compare_af(CPUMIPSState *env, wr_t *pwd, wr_t *pws,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, retaddr);
@@ -6596,7 +6596,7 @@ static inline void compare_un(CPUMIPSState *env, wr_t *pwd, wr_t *pws,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, retaddr);
@@ -6625,7 +6625,7 @@ static inline void compare_eq(CPUMIPSState *env, wr_t *pwd, wr_t *pws,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, retaddr);
@@ -6654,7 +6654,7 @@ static inline void compare_ueq(CPUMIPSState *env, wr_t *pwd, wr_t *pws,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, retaddr);
@@ -6683,7 +6683,7 @@ static inline void compare_lt(CPUMIPSState *env, wr_t *pwd, wr_t *pws,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, retaddr);
@@ -6712,7 +6712,7 @@ static inline void compare_ult(CPUMIPSState *env, wr_t *pwd, wr_t *pws,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, retaddr);
@@ -6741,7 +6741,7 @@ static inline void compare_le(CPUMIPSState *env, wr_t *pwd, wr_t *pws,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, retaddr);
@@ -6770,7 +6770,7 @@ static inline void compare_ule(CPUMIPSState *env, wr_t *pwd, wr_t *pws,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, retaddr);
@@ -6799,7 +6799,7 @@ static inline void compare_or(CPUMIPSState *env, wr_t *pwd, wr_t *pws,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, retaddr);
@@ -6828,7 +6828,7 @@ static inline void compare_une(CPUMIPSState *env, wr_t *pwd, wr_t *pws,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, retaddr);
@@ -6857,7 +6857,7 @@ static inline void compare_ne(CPUMIPSState *env, wr_t *pwd, wr_t *pws,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, retaddr);
@@ -7107,7 +7107,7 @@ void helper_msa_fadd_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, GETPC());
@@ -7137,7 +7137,7 @@ void helper_msa_fsub_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, GETPC());
@@ -7167,7 +7167,7 @@ void helper_msa_fmul_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, GETPC());
@@ -7198,7 +7198,7 @@ void helper_msa_fdiv_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, GETPC());
@@ -7245,7 +7245,7 @@ void helper_msa_fmadd_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, GETPC());
@@ -7280,7 +7280,7 @@ void helper_msa_fmsub_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, GETPC());
@@ -7317,7 +7317,7 @@ void helper_msa_fexp2_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, GETPC());
@@ -7371,7 +7371,7 @@ void helper_msa_fexdo_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, GETPC());
@@ -7417,7 +7417,7 @@ void helper_msa_ftq_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, GETPC());
@@ -7526,7 +7526,7 @@ void helper_msa_fmin_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
 
     } else {
 
-        assert(0);
+        g_assert_not_reached();
 
     }
 
@@ -7555,7 +7555,7 @@ void helper_msa_fmin_a_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
         FMAXMIN_A(min, max, pwx->d[0], pws->d[0], pwt->d[0], 64, status);
         FMAXMIN_A(min, max, pwx->d[1], pws->d[1], pwt->d[1], 64, status);
     } else {
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, GETPC());
@@ -7628,7 +7628,7 @@ void helper_msa_fmax_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
 
     } else {
 
-        assert(0);
+        g_assert_not_reached();
 
     }
 
@@ -7657,7 +7657,7 @@ void helper_msa_fmax_a_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
         FMAXMIN_A(max, min, pwx->d[0], pws->d[0], pwt->d[0], 64, status);
         FMAXMIN_A(max, min, pwx->d[1], pws->d[1], pwt->d[1], 64, status);
     } else {
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, GETPC());
@@ -7681,7 +7681,7 @@ void helper_msa_fclass_df(CPUMIPSState *env, uint32_t df,
         pwd->d[0] = float_class_d(pws->d[0], status);
         pwd->d[1] = float_class_d(pws->d[1], status);
     } else {
-        assert(0);
+        g_assert_not_reached();
     }
 }
 
@@ -7723,7 +7723,7 @@ void helper_msa_ftrunc_s_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, GETPC());
@@ -7753,7 +7753,7 @@ void helper_msa_ftrunc_u_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, GETPC());
@@ -7783,7 +7783,7 @@ void helper_msa_fsqrt_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, GETPC());
@@ -7832,7 +7832,7 @@ void helper_msa_frsqrt_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, GETPC());
@@ -7862,7 +7862,7 @@ void helper_msa_frcp_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, GETPC());
@@ -7892,7 +7892,7 @@ void helper_msa_frint_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, GETPC());
@@ -7946,7 +7946,7 @@ void helper_msa_flog2_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, GETPC());
@@ -7983,7 +7983,7 @@ void helper_msa_fexupl_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, GETPC());
@@ -8019,7 +8019,7 @@ void helper_msa_fexupr_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, GETPC());
@@ -8046,7 +8046,7 @@ void helper_msa_ffql_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     msa_move_v(pwd, pwx);
@@ -8072,7 +8072,7 @@ void helper_msa_ffqr_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     msa_move_v(pwd, pwx);
@@ -8100,7 +8100,7 @@ void helper_msa_ftint_s_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, GETPC());
@@ -8130,7 +8130,7 @@ void helper_msa_ftint_u_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, GETPC());
@@ -8166,7 +8166,7 @@ void helper_msa_ffint_s_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, GETPC());
@@ -8196,7 +8196,7 @@ void helper_msa_ffint_u_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
         }
         break;
     default:
-        assert(0);
+        g_assert_not_reached();
     }
 
     check_msacsr_cause(env, GETPC());
diff --git a/target/ppc/dfp_helper.c b/target/ppc/dfp_helper.c
index 5967ea07a9..ecc3f79326 100644
--- a/target/ppc/dfp_helper.c
+++ b/target/ppc/dfp_helper.c
@@ -249,7 +249,7 @@ static void dfp_set_FPRF_from_FRT_with_context(struct PPC_DFP *dfp,
         fprf = 0x05;
         break;
     default:
-        assert(0); /* should never get here */
+        g_assert_not_reached();
     }
     dfp->env->fpscr &= ~FP_FPRF;
     dfp->env->fpscr |= (fprf << FPSCR_FPRF);
@@ -1243,7 +1243,7 @@ void helper_##op(CPUPPCState *env, ppc_fprp_t *t, ppc_fprp_t *b) \
         } else if (decNumberIsQNaN(&dfp.b)) {                  \
             vt.VsrD(1) = -2;                                   \
         } else {                                               \
-            assert(0);                                         \
+            g_assert_not_reached();                            \
         }                                                      \
         set_dfp64(t, &vt);                                     \
     } else {                                                   \
@@ -1252,7 +1252,7 @@ void helper_##op(CPUPPCState *env, ppc_fprp_t *t, ppc_fprp_t *b) \
         } else if ((size) == 128) {                            \
             vt.VsrD(1) = dfp.b.exponent + 6176;                \
         } else {                                               \
-            assert(0);                                         \
+            g_assert_not_reached();                            \
         }                                                      \
         set_dfp64(t, &vt);                                     \
     }                                                          \
@@ -1300,7 +1300,7 @@ void helper_##op(CPUPPCState *env, ppc_fprp_t *t, ppc_fprp_t *a,          \
         raw_inf = 0x1e000;                                                \
         bias = 6176;                                                      \
     } else {                                                              \
-        assert(0);                                                        \
+        g_assert_not_reached();                                           \
     }                                                                     \
                                                                           \
     if (unlikely((exp < 0) || (exp > max_exp))) {                         \
diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
index 64e30435f5..b064f3bd04 100644
--- a/target/ppc/mmu_helper.c
+++ b/target/ppc/mmu_helper.c
@@ -353,7 +353,7 @@ void ppc_tlb_invalidate_one(CPUPPCState *env, target_ulong addr)
         break;
     default:
         /* Should never reach here with other MMU models */
-        assert(0);
+        g_assert_not_reached();
     }
 #else
     ppc_tlb_invalidate_all(env);
diff --git a/tests/qtest/ipmi-bt-test.c b/tests/qtest/ipmi-bt-test.c
index ed431e34e6..5bd6b769d3 100644
--- a/tests/qtest/ipmi-bt-test.c
+++ b/tests/qtest/ipmi-bt-test.c
@@ -251,7 +251,7 @@ static void emu_msg_handler(void)
         msg[msg_len++] = 0xa0;
         write_emu_msg(msg, msg_len);
     } else {
-        g_assert(0);
+        g_assert_not_reached();
     }
 }
 
diff --git a/tests/qtest/ipmi-kcs-test.c b/tests/qtest/ipmi-kcs-test.c
index afc24dd3e4..3186c6ad64 100644
--- a/tests/qtest/ipmi-kcs-test.c
+++ b/tests/qtest/ipmi-kcs-test.c
@@ -145,7 +145,7 @@ static void kcs_cmd(uint8_t *cmd, unsigned int cmd_len,
         break;
 
     default:
-        g_assert(0);
+        g_assert_not_reached();
     }
     *rsp_len = j;
 }
@@ -184,7 +184,7 @@ static void kcs_abort(uint8_t *cmd, unsigned int cmd_len,
         break;
 
     default:
-        g_assert(0);
+        g_assert_not_reached();
     }
 
     /* Start the abort here */
diff --git a/tests/qtest/rtl8139-test.c b/tests/qtest/rtl8139-test.c
index 8fa3313cc3..37e352c651 100644
--- a/tests/qtest/rtl8139-test.c
+++ b/tests/qtest/rtl8139-test.c
@@ -59,7 +59,7 @@ PORT(IntrMask, w, 0x3c)
 PORT(IntrStatus, w, 0x3E)
 PORT(TimerInt, l, 0x54)
 
-#define fatal(...) do { g_test_message(__VA_ARGS__); g_assert(0); } while (0)
+#define fatal(...) do { g_test_message(__VA_ARGS__); g_assert_not_reached(); } while (0)
 
 static void test_timer(void)
 {
-- 
2.38.1

Re: [PATCH 3/5] bulk: Replace [g_]assert(0) -> g_assert_not_reached()

[Richard Henderson]

On 2/21/23 13:25, Philippe Mathieu-Daudé wrote:
> In order to avoid warnings such commit c0a6665c3c ("target/i386:
> Remove compilation errors when -Werror=maybe-uninitialized"),
> replace all assert(0) and g_assert(0) by g_assert_not_reached().
> 
> Remove any code following g_assert_not_reached().
> 
> See previous commit for rationale.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   docs/spin/aio_notify_accept.promela |   6 +-
>   docs/spin/aio_notify_bug.promela    |   6 +-

C only.  Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 3/5] bulk: Replace [g_]assert(0) -> g_assert_not_reached()

[Thomas Huth]

On 22/02/2023 00.25, Philippe Mathieu-Daudé wrote:
> In order to avoid warnings such commit c0a6665c3c ("target/i386:
> Remove compilation errors when -Werror=maybe-uninitialized"),
> replace all assert(0) and g_assert(0) by g_assert_not_reached().
> 
> Remove any code following g_assert_not_reached().
> 
> See previous commit for rationale.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> 
> diff --git a/docs/spin/aio_notify_accept.promela b/docs/spin/aio_notify_accept.promela
> index 9cef2c955d..f929d30328 100644
> --- a/docs/spin/aio_notify_accept.promela
> +++ b/docs/spin/aio_notify_accept.promela
> @@ -118,7 +118,7 @@ accept_if_req_not_eventually_false:
>       if
>           :: req -> goto accept_if_req_not_eventually_false;
>       fi;
> -    assert(0);
> +    g_assert_not_reached();
>   }

This does not look like C code ... is it safe to replace the statement here?

> diff --git a/docs/spin/aio_notify_bug.promela b/docs/spin/aio_notify_bug.promela
> index b3bfca1ca4..ce6f5177ed 100644
> --- a/docs/spin/aio_notify_bug.promela
> +++ b/docs/spin/aio_notify_bug.promela
> @@ -106,7 +106,7 @@ accept_if_req_not_eventually_false:
>       if
>           :: req -> goto accept_if_req_not_eventually_false;
>       fi;
> -    assert(0);
> +    g_assert_not_reached();
>   }

dito

> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index f54f44d899..59c8032a21 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -1347,49 +1347,42 @@ int postcopy_ram_incoming_init(MigrationIncomingState *mis)
>   
>   int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
>   {
> -    assert(0);
> -    return -1;
> +    g_assert_not_reached();
>   }
>   
>   int postcopy_ram_prepare_discard(MigrationIncomingState *mis)
>   {
> -    assert(0);
> -    return -1;
> +    g_assert_not_reached();
>   }
>   
>   int postcopy_request_shared_page(struct PostCopyFD *pcfd, RAMBlock *rb,
>                                    uint64_t client_addr, uint64_t rb_offset)
>   {
> -    assert(0);
> -    return -1;
> +    g_assert_not_reached();
>   }
>   
>   int postcopy_ram_incoming_setup(MigrationIncomingState *mis)
>   {
> -    assert(0);
> -    return -1;
> +    g_assert_not_reached();
>   }
>   
>   int postcopy_place_page(MigrationIncomingState *mis, void *host, void *from,
>                           RAMBlock *rb)
>   {
> -    assert(0);
> -    return -1;
> +    g_assert_not_reached();
>   }
>   
>   int postcopy_place_page_zero(MigrationIncomingState *mis, void *host,
>                           RAMBlock *rb)
>   {
> -    assert(0);
> -    return -1;
> +    g_assert_not_reached();
>   }
>   
>   int postcopy_wake_shared(struct PostCopyFD *pcfd,
>                            uint64_t client_addr,
>                            RAMBlock *rb)
>   {
> -    assert(0);
> -    return -1;
> +    g_assert_not_reached();
>   }
>   #endif

If we ever reconsider to allow compiling with G_DISABLE_ASSERT again, this 
will fail to compile since the return is missing now, so this is kind of 
ugly ... would it make sense to replace this with g_assert_true(0) instead? 
Or use abort() directly?

  Thomas

Re: [PATCH 3/5] bulk: Replace [g_]assert(0) -> g_assert_not_reached()

[Richard Henderson]

On 2/21/23 18:06, Thomas Huth wrote:
>>   int postcopy_wake_shared(struct PostCopyFD *pcfd,
>>                            uint64_t client_addr,
>>                            RAMBlock *rb)
>>   {
>> -    assert(0);
>> -    return -1;
>> +    g_assert_not_reached();
>>   }
>>   #endif
> 
> If we ever reconsider to allow compiling with G_DISABLE_ASSERT again,

... and we shouldn't [1] ...

> this will fail to compile since the return is missing now, so this is kind of ugly ... would it make sense to replace this with g_assert_true(0) instead? Or use abort() directly?

With g_assert_true(0), definitely not.
That is a testing-only item which can be disabled at runtime.

With abort(), no, since g_assert_not_reached() prints file:line.
Indeed, I was suggesting the opposite -- to replace abort() without error_report() with 
g_assert_not_reached().


r~


[1] Allowing G_DISABLE_ASSERT and/or NDEBUG would only require that we invent 
qemu-specific replacements with either (1) do exactly the same thing or, (2) interact with 
__builtin_unreachable() or __builtin_trap(), so that we tell the compiler exactly what's 
going on with the expressions and flow control.

Re: [PATCH 3/5] bulk: Replace [g_]assert(0) -> g_assert_not_reached()

[Thomas Huth]

On 22/02/2023 07.29, Richard Henderson wrote:
> On 2/21/23 18:06, Thomas Huth wrote:
>>>   int postcopy_wake_shared(struct PostCopyFD *pcfd,
>>>                            uint64_t client_addr,
>>>                            RAMBlock *rb)
>>>   {
>>> -    assert(0);
>>> -    return -1;
>>> +    g_assert_not_reached();
>>>   }
>>>   #endif
>>
>> If we ever reconsider to allow compiling with G_DISABLE_ASSERT again,
> 
> ... and we shouldn't [1] ...
> 
>> this will fail to compile since the return is missing now, so this is kind 
>> of ugly ... would it make sense to replace this with g_assert_true(0) 
>> instead? Or use abort() directly?
> 
> With g_assert_true(0), definitely not.
> That is a testing-only item which can be disabled at runtime.
> 
> With abort(), no, since g_assert_not_reached() prints file:line.
> Indeed, I was suggesting the opposite -- to replace abort() without 
> error_report() with g_assert_not_reached().

OK, fair, makes sense. I now slightly remember we also had similar 
discussions in the past already, so these hunks now sound fine to me, too.

  Thomas

Re: [PATCH 3/5] bulk: Replace [g_]assert(0) -> g_assert_not_reached()

[Thomas Huth]

On 22/02/2023 00.25, Philippe Mathieu-Daudé wrote:
> In order to avoid warnings such commit c0a6665c3c ("target/i386:
> Remove compilation errors when -Werror=maybe-uninitialized"),
> replace all assert(0) and g_assert(0) by g_assert_not_reached().
> 
> Remove any code following g_assert_not_reached().
> 
> See previous commit for rationale.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
...
> diff --git a/hw/net/i82596.c b/hw/net/i82596.c
> index ec21e2699a..eda0f586fb 100644
> --- a/hw/net/i82596.c
> +++ b/hw/net/i82596.c
> @@ -285,7 +285,7 @@ static void command_loop(I82596State *s)
>           case CmdDump:
>           case CmdDiagnose:
>               printf("FIXME Command %d !!\n", cmd & 7);
> -            assert(0);
> +            g_assert_not_reached();
>           }

While looking at this patch a second time, this hunk caught my eye. It looks 
like the guest could use these commands to crash QEMU? Should this be a 
qemu_log_mask(LOG_UNIMP,...) + graceful return instead?

  Thomas

Re: [PATCH 3/5] bulk: Replace [g_]assert(0) -> g_assert_not_reached()

[Philippe Mathieu-Daudé]

On 22/2/23 12:56, Thomas Huth wrote:
> On 22/02/2023 00.25, Philippe Mathieu-Daudé wrote:
>> In order to avoid warnings such commit c0a6665c3c ("target/i386:
>> Remove compilation errors when -Werror=maybe-uninitialized"),
>> replace all assert(0) and g_assert(0) by g_assert_not_reached().
>>
>> Remove any code following g_assert_not_reached().
>>
>> See previous commit for rationale.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
> ...
>> diff --git a/hw/net/i82596.c b/hw/net/i82596.c
>> index ec21e2699a..eda0f586fb 100644
>> --- a/hw/net/i82596.c
>> +++ b/hw/net/i82596.c
>> @@ -285,7 +285,7 @@ static void command_loop(I82596State *s)
>>           case CmdDump:
>>           case CmdDiagnose:
>>               printf("FIXME Command %d !!\n", cmd & 7);
>> -            assert(0);
>> +            g_assert_not_reached();
>>           }
> 
> While looking at this patch a second time, this hunk caught my eye. It 
> looks like the guest could use these commands to crash QEMU? Should this 
> be a qemu_log_mask(LOG_UNIMP,...) + graceful return instead?

I didn't want to have to worry about that, but you are right, sigh.
I'll review each case and add a preliminary patch to clean the
dangerous ones.

[PATCH 4/5] block/vvfat: Remove pointless check of NDEBUG

[Philippe Mathieu-Daudé]

Since commit 262a69f428 ("osdep.h: Prohibit disabling
assert() in supported builds") 'NDEBUG' can not be defined,
so '#ifndef NDEBUG' is dead code. Remove it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 block/vvfat.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index d7d775bd2c..fd45e86416 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2784,13 +2784,10 @@ static int handle_commits(BDRVVVFATState* s)
             fail = -2;
             break;
         case ACTION_WRITEOUT: {
-#ifndef NDEBUG
-            /* these variables are only used by assert() below */
             direntry_t* entry = array_get(&(s->directory),
                     commit->param.writeout.dir_index);
             uint32_t begin = begin_of_direntry(entry);
             mapping_t* mapping = find_mapping_for_cluster(s, begin);
-#endif
 
             assert(mapping);
             assert(mapping->begin == begin);
-- 
2.38.1

Re: [PATCH 4/5] block/vvfat: Remove pointless check of NDEBUG

[Richard Henderson]

On 2/21/23 13:25, Philippe Mathieu-Daudé wrote:
> Since commit 262a69f428 ("osdep.h: Prohibit disabling
> assert() in supported builds") 'NDEBUG' can not be defined,
> so '#ifndef NDEBUG' is dead code. Remove it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   block/vvfat.c | 3 ---
>   1 file changed, 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

> 
> diff --git a/block/vvfat.c b/block/vvfat.c
> index d7d775bd2c..fd45e86416 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -2784,13 +2784,10 @@ static int handle_commits(BDRVVVFATState* s)
>               fail = -2;
>               break;
>           case ACTION_WRITEOUT: {
> -#ifndef NDEBUG
> -            /* these variables are only used by assert() below */
>               direntry_t* entry = array_get(&(s->directory),
>                       commit->param.writeout.dir_index);
>               uint32_t begin = begin_of_direntry(entry);
>               mapping_t* mapping = find_mapping_for_cluster(s, begin);
> -#endif
>   
>               assert(mapping);
>               assert(mapping->begin == begin);

[PATCH 5/5] hw: Remove mentions of NDEBUG

[Philippe Mathieu-Daudé]

Since commit 262a69f428 ("osdep.h: Prohibit disabling
assert() in supported builds") 'NDEBUG' can not be defined.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/scsi/mptsas.c   | 2 --
 hw/virtio/virtio.c | 2 --
 2 files changed, 4 deletions(-)

diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
index c485da792c..5b373d3ed6 100644
--- a/hw/scsi/mptsas.c
+++ b/hw/scsi/mptsas.c
@@ -1240,8 +1240,6 @@ static void *mptsas_load_request(QEMUFile *f, SCSIRequest *sreq)
     n = qemu_get_be32(f);
     /* TODO: add a way for SCSIBusInfo's load_request to fail,
      * and fail migration instead of asserting here.
-     * This is just one thing (there are probably more) that must be
-     * fixed before we can allow NDEBUG compilation.
      */
     assert(n >= 0);
 
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index f35178f5fc..c6b3e3fb08 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1898,8 +1898,6 @@ void *qemu_get_virtqueue_element(VirtIODevice *vdev, QEMUFile *f, size_t sz)
 
     /* TODO: teach all callers that this can fail, and return failure instead
      * of asserting here.
-     * This is just one thing (there are probably more) that must be
-     * fixed before we can allow NDEBUG compilation.
      */
     assert(ARRAY_SIZE(data.in_addr) >= data.in_num);
     assert(ARRAY_SIZE(data.out_addr) >= data.out_num);
-- 
2.38.1

Re: [PATCH 5/5] hw: Remove mentions of NDEBUG

[Richard Henderson]

On 2/21/23 13:25, Philippe Mathieu-Daudé wrote:
> Since commit 262a69f428 ("osdep.h: Prohibit disabling
> assert() in supported builds") 'NDEBUG' can not be defined.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/scsi/mptsas.c   | 2 --
>   hw/virtio/virtio.c | 2 --
>   2 files changed, 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

> 
> diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
> index c485da792c..5b373d3ed6 100644
> --- a/hw/scsi/mptsas.c
> +++ b/hw/scsi/mptsas.c
> @@ -1240,8 +1240,6 @@ static void *mptsas_load_request(QEMUFile *f, SCSIRequest *sreq)
>       n = qemu_get_be32(f);
>       /* TODO: add a way for SCSIBusInfo's load_request to fail,
>        * and fail migration instead of asserting here.
> -     * This is just one thing (there are probably more) that must be
> -     * fixed before we can allow NDEBUG compilation.
>        */
>       assert(n >= 0);
>   
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index f35178f5fc..c6b3e3fb08 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1898,8 +1898,6 @@ void *qemu_get_virtqueue_element(VirtIODevice *vdev, QEMUFile *f, size_t sz)
>   
>       /* TODO: teach all callers that this can fail, and return failure instead
>        * of asserting here.
> -     * This is just one thing (there are probably more) that must be
> -     * fixed before we can allow NDEBUG compilation.
>        */
>       assert(ARRAY_SIZE(data.in_addr) >= data.in_num);
>       assert(ARRAY_SIZE(data.out_addr) >= data.out_num);

Re: [PATCH 5/5] hw: Remove mentions of NDEBUG

[Michael S. Tsirkin]

On Wed, Feb 22, 2023 at 12:25:20AM +0100, Philippe Mathieu-Daudé wrote:
> Since commit 262a69f428 ("osdep.h: Prohibit disabling
> assert() in supported builds") 'NDEBUG' can not be defined.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

this exactly says NDEBUG is not allowed. why are you removing this?
  
> ---
>  hw/scsi/mptsas.c   | 2 --
>  hw/virtio/virtio.c | 2 --
>  2 files changed, 4 deletions(-)
> 
> diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
> index c485da792c..5b373d3ed6 100644
> --- a/hw/scsi/mptsas.c
> +++ b/hw/scsi/mptsas.c
> @@ -1240,8 +1240,6 @@ static void *mptsas_load_request(QEMUFile *f, SCSIRequest *sreq)
>      n = qemu_get_be32(f);
>      /* TODO: add a way for SCSIBusInfo's load_request to fail,
>       * and fail migration instead of asserting here.
> -     * This is just one thing (there are probably more) that must be
> -     * fixed before we can allow NDEBUG compilation.
>       */
>      assert(n >= 0);
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index f35178f5fc..c6b3e3fb08 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1898,8 +1898,6 @@ void *qemu_get_virtqueue_element(VirtIODevice *vdev, QEMUFile *f, size_t sz)
>  
>      /* TODO: teach all callers that this can fail, and return failure instead
>       * of asserting here.
> -     * This is just one thing (there are probably more) that must be
> -     * fixed before we can allow NDEBUG compilation.
>       */
>      assert(ARRAY_SIZE(data.in_addr) >= data.in_num);
>      assert(ARRAY_SIZE(data.out_addr) >= data.out_num);
> -- 
> 2.38.1

Re: [PATCH 5/5] hw: Remove mentions of NDEBUG

[Philippe Mathieu-Daudé]

On 22/2/23 13:05, Michael S. Tsirkin wrote:
> On Wed, Feb 22, 2023 at 12:25:20AM +0100, Philippe Mathieu-Daudé wrote:
>> Since commit 262a69f428 ("osdep.h: Prohibit disabling
>> assert() in supported builds") 'NDEBUG' can not be defined.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> this exactly says NDEBUG is not allowed. why are you removing this?

The project can not be built with NDEBUG. There is no point in
mentioning it in each individual function.

>> ---
>>   hw/scsi/mptsas.c   | 2 --
>>   hw/virtio/virtio.c | 2 --
>>   2 files changed, 4 deletions(-)
>>
>> diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
>> index c485da792c..5b373d3ed6 100644
>> --- a/hw/scsi/mptsas.c
>> +++ b/hw/scsi/mptsas.c
>> @@ -1240,8 +1240,6 @@ static void *mptsas_load_request(QEMUFile *f, SCSIRequest *sreq)
>>       n = qemu_get_be32(f);
>>       /* TODO: add a way for SCSIBusInfo's load_request to fail,
>>        * and fail migration instead of asserting here.
>> -     * This is just one thing (there are probably more) that must be
>> -     * fixed before we can allow NDEBUG compilation.
>>        */
>>       assert(n >= 0);
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index f35178f5fc..c6b3e3fb08 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -1898,8 +1898,6 @@ void *qemu_get_virtqueue_element(VirtIODevice *vdev, QEMUFile *f, size_t sz)
>>   
>>       /* TODO: teach all callers that this can fail, and return failure instead
>>        * of asserting here.
>> -     * This is just one thing (there are probably more) that must be
>> -     * fixed before we can allow NDEBUG compilation.
>>        */
>>       assert(ARRAY_SIZE(data.in_addr) >= data.in_num);
>>       assert(ARRAY_SIZE(data.out_addr) >= data.out_num);
>> -- 
>> 2.38.1
> 

Re: [PATCH 5/5] hw: Remove mentions of NDEBUG

[Michael S. Tsirkin]

On Wed, Feb 22, 2023 at 05:11:36PM +0100, Philippe Mathieu-Daudé wrote:
> On 22/2/23 13:05, Michael S. Tsirkin wrote:
> > On Wed, Feb 22, 2023 at 12:25:20AM +0100, Philippe Mathieu-Daudé wrote:
> > > Since commit 262a69f428 ("osdep.h: Prohibit disabling
> > > assert() in supported builds") 'NDEBUG' can not be defined.
> > > 
> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > 
> > this exactly says NDEBUG is not allowed. why are you removing this?
> 
> The project can not be built with NDEBUG. There is no point in
> mentioning it in each individual function.

the reason we mention it is because there are security implications
if we don't.

> > > ---
> > >   hw/scsi/mptsas.c   | 2 --
> > >   hw/virtio/virtio.c | 2 --
> > >   2 files changed, 4 deletions(-)
> > > 
> > > diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
> > > index c485da792c..5b373d3ed6 100644
> > > --- a/hw/scsi/mptsas.c
> > > +++ b/hw/scsi/mptsas.c
> > > @@ -1240,8 +1240,6 @@ static void *mptsas_load_request(QEMUFile *f, SCSIRequest *sreq)
> > >       n = qemu_get_be32(f);
> > >       /* TODO: add a way for SCSIBusInfo's load_request to fail,
> > >        * and fail migration instead of asserting here.
> > > -     * This is just one thing (there are probably more) that must be
> > > -     * fixed before we can allow NDEBUG compilation.
> > >        */
> > >       assert(n >= 0);
> > > 
> > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > index f35178f5fc..c6b3e3fb08 100644
> > > --- a/hw/virtio/virtio.c
> > > +++ b/hw/virtio/virtio.c
> > > @@ -1898,8 +1898,6 @@ void *qemu_get_virtqueue_element(VirtIODevice *vdev, QEMUFile *f, size_t sz)
> > >       /* TODO: teach all callers that this can fail, and return failure instead
> > >        * of asserting here.
> > > -     * This is just one thing (there are probably more) that must be
> > > -     * fixed before we can allow NDEBUG compilation.
> > >        */
> > >       assert(ARRAY_SIZE(data.in_addr) >= data.in_num);
> > >       assert(ARRAY_SIZE(data.out_addr) >= data.out_num);
> > > -- 
> > > 2.38.1
> > 

Re: [PATCH 5/5] hw: Remove mentions of NDEBUG

[Richard Henderson]

On 2/22/23 06:28, Michael S. Tsirkin wrote:
> On Wed, Feb 22, 2023 at 05:11:36PM +0100, Philippe Mathieu-Daudé wrote:
>> On 22/2/23 13:05, Michael S. Tsirkin wrote:
>>> On Wed, Feb 22, 2023 at 12:25:20AM +0100, Philippe Mathieu-Daudé wrote:
>>>> Since commit 262a69f428 ("osdep.h: Prohibit disabling
>>>> assert() in supported builds") 'NDEBUG' can not be defined.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>
>>> this exactly says NDEBUG is not allowed. why are you removing this?
>>
>> The project can not be built with NDEBUG. There is no point in
>> mentioning it in each individual function.
> 
> the reason we mention it is because there are security implications
> if we don't.

Yes.  However that's not what the text being removed suggests:

>>>> -     * This is just one thing (there are probably more) that must be
>>>> -     * fixed before we can allow NDEBUG compilation.

This suggests that we *will* allow NDEBUG, once a few things are fixed.

I strongly approve of this text being removed.


r~

Re: [PATCH 5/5] hw: Remove mentions of NDEBUG

[Michael S. Tsirkin]

On Wed, Feb 22, 2023 at 08:43:35AM -1000, Richard Henderson wrote:
> On 2/22/23 06:28, Michael S. Tsirkin wrote:
> > On Wed, Feb 22, 2023 at 05:11:36PM +0100, Philippe Mathieu-Daudé wrote:
> > > On 22/2/23 13:05, Michael S. Tsirkin wrote:
> > > > On Wed, Feb 22, 2023 at 12:25:20AM +0100, Philippe Mathieu-Daudé wrote:
> > > > > Since commit 262a69f428 ("osdep.h: Prohibit disabling
> > > > > assert() in supported builds") 'NDEBUG' can not be defined.
> > > > > 
> > > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > > 
> > > > this exactly says NDEBUG is not allowed. why are you removing this?
> > > 
> > > The project can not be built with NDEBUG. There is no point in
> > > mentioning it in each individual function.
> > 
> > the reason we mention it is because there are security implications
> > if we don't.
> 
> Yes.  However that's not what the text being removed suggests:
> 
> > > > > -     * This is just one thing (there are probably more) that must be
> > > > > -     * fixed before we can allow NDEBUG compilation.
> 
> This suggests that we *will* allow NDEBUG, once a few things are fixed.
> 
> I strongly approve of this text being removed.
> 
> 
> r~


OK I think it's a good idea to replace it with something like

/* Note: Do not remove this assertion, doing so will break qemu security! */

-- 
MST