1. Patchew
  2. QEMU
  3. [PATCH 00/39] Use g_assert_not_reached instead of (g_)assert(0, false)
  4. View patch

[PATCH 01/39] docs/spin: replace assert(0) with g_assert_not_reached()

Pierrick Bouvier posted 39 patches 2 months, 2 weeks ago
There is a newer version of this series
[PATCH 01/39] docs/spin: replace assert(0) with g_assert_not_reached()
Posted by Pierrick Bouvier 2 months, 2 weeks ago 
Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 docs/spin/aio_notify_accept.promela | 6 +++---
 docs/spin/aio_notify_bug.promela    | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/docs/spin/aio_notify_accept.promela b/docs/spin/aio_notify_accept.promela
index 9cef2c955dd..f929d303281 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 b3bfca1ca4f..ce6f5177ed5 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
-- 
2.39.2
Re: [PATCH 01/39] docs/spin: replace assert(0) with g_assert_not_reached()
Posted by Eric Blake 2 months, 1 week ago 
On Tue, Sep 10, 2024 at 03:15:28PM GMT, Pierrick Bouvier wrote:
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---

A general suggestion for the entire series: please use a commit
message that explains why this is a good idea.  Even something as
boiler-plate as "refer to commit XXX for rationale" that can be
copy-pasted into all the other commits is better than nothing,
although a self-contained message is best.  Maybe:

This patch is part of a series that moves towards a consistent use of
g_assert_not_reached() rather than an ad hoc mix of different
assertion mechanisms.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org
Re: [PATCH 01/39] docs/spin: replace assert(0) with g_assert_not_reached()
Posted by Eric Blake 2 months, 1 week ago 
On Wed, Sep 11, 2024 at 07:33:59AM GMT, Eric Blake wrote:
> On Tue, Sep 10, 2024 at 03:15:28PM GMT, Pierrick Bouvier wrote:
> > Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> > ---
> 
> A general suggestion for the entire series: please use a commit
> message that explains why this is a good idea.  Even something as
> boiler-plate as "refer to commit XXX for rationale" that can be
> copy-pasted into all the other commits is better than nothing,
> although a self-contained message is best.  Maybe:
> 
> This patch is part of a series that moves towards a consistent use of
> g_assert_not_reached() rather than an ad hoc mix of different
> assertion mechanisms.

Or summarize your cover letter:

Use of assert(false) can trip spurious control flow warnings from some
versions of gcc:
https://lore.kernel.org/qemu-devel/54bb02a6-1b12-460a-97f6-3f478ef766c6@linaro.org/
Solve that by unifying the code base on g_assert_not_reached()
instead.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org
Re: [PATCH 01/39] docs/spin: replace assert(0) with g_assert_not_reached()
Posted by Pierrick Bouvier 2 months, 1 week ago 
On 9/11/24 05:37, Eric Blake wrote:
> On Wed, Sep 11, 2024 at 07:33:59AM GMT, Eric Blake wrote:
>> On Tue, Sep 10, 2024 at 03:15:28PM GMT, Pierrick Bouvier wrote:
>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>> ---
>>
>> A general suggestion for the entire series: please use a commit
>> message that explains why this is a good idea.  Even something as
>> boiler-plate as "refer to commit XXX for rationale" that can be
>> copy-pasted into all the other commits is better than nothing,
>> although a self-contained message is best.  Maybe:
>>
>> This patch is part of a series that moves towards a consistent use of
>> g_assert_not_reached() rather than an ad hoc mix of different
>> assertion mechanisms.
> 
> Or summarize your cover letter:
> 
> Use of assert(false) can trip spurious control flow warnings from some
> versions of gcc:
> https://lore.kernel.org/qemu-devel/54bb02a6-1b12-460a-97f6-3f478ef766c6@linaro.org/
> Solve that by unifying the code base on g_assert_not_reached()
> instead.
> 

I'll add this to messages. Thanks.
Re: [PATCH 01/39] docs/spin: replace assert(0) with g_assert_not_reached()
Posted by Maciej S. Szmigiero 2 months, 1 week ago 
On 11.09.2024 14:37, Eric Blake wrote:
> On Wed, Sep 11, 2024 at 07:33:59AM GMT, Eric Blake wrote:
>> On Tue, Sep 10, 2024 at 03:15:28PM GMT, Pierrick Bouvier wrote:
>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>> ---
>>
>> A general suggestion for the entire series: please use a commit
>> message that explains why this is a good idea.  Even something as
>> boiler-plate as "refer to commit XXX for rationale" that can be
>> copy-pasted into all the other commits is better than nothing,
>> although a self-contained message is best.  Maybe:
>>
>> This patch is part of a series that moves towards a consistent use of
>> g_assert_not_reached() rather than an ad hoc mix of different
>> assertion mechanisms.
> 
> Or summarize your cover letter:
> 
> Use of assert(false) can trip spurious control flow warnings from some
> versions of gcc:
> https://lore.kernel.org/qemu-devel/54bb02a6-1b12-460a-97f6-3f478ef766c6@linaro.org/
> Solve that by unifying the code base on g_assert_not_reached()
> instead.
> 

If using g_assert_not_reached() instead of assert(false) silences
the warning about missing return value in such impossible to reach
locations should we also be deleting the now-unnecessary "return"
statements after g_assert_not_reached()?

Thanks,
Maciej
Re: [PATCH 01/39] docs/spin: replace assert(0) with g_assert_not_reached()
Posted by Richard W.M. Jones 2 months, 1 week ago 
On Wed, Sep 11, 2024 at 02:46:18PM +0200, Maciej S. Szmigiero wrote:
> On 11.09.2024 14:37, Eric Blake wrote:
> >On Wed, Sep 11, 2024 at 07:33:59AM GMT, Eric Blake wrote:
> >>On Tue, Sep 10, 2024 at 03:15:28PM GMT, Pierrick Bouvier wrote:
> >>>Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> >>>---
> >>
> >>A general suggestion for the entire series: please use a commit
> >>message that explains why this is a good idea.  Even something as
> >>boiler-plate as "refer to commit XXX for rationale" that can be
> >>copy-pasted into all the other commits is better than nothing,
> >>although a self-contained message is best.  Maybe:
> >>
> >>This patch is part of a series that moves towards a consistent use of
> >>g_assert_not_reached() rather than an ad hoc mix of different
> >>assertion mechanisms.
> >
> >Or summarize your cover letter:
> >
> >Use of assert(false) can trip spurious control flow warnings from some
> >versions of gcc:
> >https://lore.kernel.org/qemu-devel/54bb02a6-1b12-460a-97f6-3f478ef766c6@linaro.org/
> >Solve that by unifying the code base on g_assert_not_reached()
> >instead.
> >
> 
> If using g_assert_not_reached() instead of assert(false) silences
> the warning about missing return value in such impossible to reach
> locations should we also be deleting the now-unnecessary "return"
> statements after g_assert_not_reached()?

Although it's unlikely to be used on any compiler that can also
compile qemu, there is a third implementation of g_assert_not_reached
that does nothing, see:

https://gitlab.gnome.org/GNOME/glib/-/blob/927683ebd94eb66c0d7868b77863f57ce9c5bc76/glib/gtestutils.h#L269

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
Re: [PATCH 01/39] docs/spin: replace assert(0) with g_assert_not_reached()
Posted by Thomas Huth 2 months, 1 week ago 
On 11/09/2024 14.51, Richard W.M. Jones wrote:
> On Wed, Sep 11, 2024 at 02:46:18PM +0200, Maciej S. Szmigiero wrote:
>> On 11.09.2024 14:37, Eric Blake wrote:
>>> On Wed, Sep 11, 2024 at 07:33:59AM GMT, Eric Blake wrote:
>>>> On Tue, Sep 10, 2024 at 03:15:28PM GMT, Pierrick Bouvier wrote:
>>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>>>> ---
>>>>
>>>> A general suggestion for the entire series: please use a commit
>>>> message that explains why this is a good idea.  Even something as
>>>> boiler-plate as "refer to commit XXX for rationale" that can be
>>>> copy-pasted into all the other commits is better than nothing,
>>>> although a self-contained message is best.  Maybe:
>>>>
>>>> This patch is part of a series that moves towards a consistent use of
>>>> g_assert_not_reached() rather than an ad hoc mix of different
>>>> assertion mechanisms.
>>>
>>> Or summarize your cover letter:
>>>
>>> Use of assert(false) can trip spurious control flow warnings from some
>>> versions of gcc:
>>> https://lore.kernel.org/qemu-devel/54bb02a6-1b12-460a-97f6-3f478ef766c6@linaro.org/
>>> Solve that by unifying the code base on g_assert_not_reached()
>>> instead.
>>>
>>
>> If using g_assert_not_reached() instead of assert(false) silences
>> the warning about missing return value in such impossible to reach
>> locations should we also be deleting the now-unnecessary "return"
>> statements after g_assert_not_reached()?
> 
> Although it's unlikely to be used on any compiler that can also
> compile qemu, there is a third implementation of g_assert_not_reached
> that does nothing, see:
> 
> https://gitlab.gnome.org/GNOME/glib/-/blob/927683ebd94eb66c0d7868b77863f57ce9c5bc76/glib/gtestutils.h#L269

That's only in the #ifdef G_DISABLE_ASSERT case ... and we forbid that in 
QEMU, see osdep.h:

#ifdef G_DISABLE_ASSERT
#error building with G_DISABLE_ASSERT is not supported
#endif

So in QEMU, g_assert_not_reached() should always abort.

  Thomas
Re: [PATCH 01/39] docs/spin: replace assert(0) with g_assert_not_reached()
Posted by Pierrick Bouvier 2 months, 1 week ago 
On 9/11/24 09:13, Thomas Huth wrote:
> On 11/09/2024 14.51, Richard W.M. Jones wrote:
>> On Wed, Sep 11, 2024 at 02:46:18PM +0200, Maciej S. Szmigiero wrote:
>>> On 11.09.2024 14:37, Eric Blake wrote:
>>>> On Wed, Sep 11, 2024 at 07:33:59AM GMT, Eric Blake wrote:
>>>>> On Tue, Sep 10, 2024 at 03:15:28PM GMT, Pierrick Bouvier wrote:
>>>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>>>>> ---
>>>>>
>>>>> A general suggestion for the entire series: please use a commit
>>>>> message that explains why this is a good idea.  Even something as
>>>>> boiler-plate as "refer to commit XXX for rationale" that can be
>>>>> copy-pasted into all the other commits is better than nothing,
>>>>> although a self-contained message is best.  Maybe:
>>>>>
>>>>> This patch is part of a series that moves towards a consistent use of
>>>>> g_assert_not_reached() rather than an ad hoc mix of different
>>>>> assertion mechanisms.
>>>>
>>>> Or summarize your cover letter:
>>>>
>>>> Use of assert(false) can trip spurious control flow warnings from some
>>>> versions of gcc:
>>>> https://lore.kernel.org/qemu-devel/54bb02a6-1b12-460a-97f6-3f478ef766c6@linaro.org/
>>>> Solve that by unifying the code base on g_assert_not_reached()
>>>> instead.
>>>>
>>>
>>> If using g_assert_not_reached() instead of assert(false) silences
>>> the warning about missing return value in such impossible to reach
>>> locations should we also be deleting the now-unnecessary "return"
>>> statements after g_assert_not_reached()?
>>
>> Although it's unlikely to be used on any compiler that can also
>> compile qemu, there is a third implementation of g_assert_not_reached
>> that does nothing, see:
>>
>> https://gitlab.gnome.org/GNOME/glib/-/blob/927683ebd94eb66c0d7868b77863f57ce9c5bc76/glib/gtestutils.h#L269
> 
> That's only in the #ifdef G_DISABLE_ASSERT case ... and we forbid that in
> QEMU, see osdep.h:
> 
> #ifdef G_DISABLE_ASSERT
> #error building with G_DISABLE_ASSERT is not supported
> #endif
> 
> So in QEMU, g_assert_not_reached() should always abort.
> 
>    Thomas
> 

Yes indeed.

For further information:

g_assert_not_reached() expand to g_assertion_message_expr(), [1]
which is a function marked noreturn [2][3], so indeed, it always abort.

[1] 
https://gitlab.gnome.org/GNOME/glib/-/blob/927683ebd94eb66c0d7868b77863f57ce9c5bc76/glib/gtestutils.h#L274
[2] 
https://gitlab.gnome.org/GNOME/glib/-/blob/927683ebd94eb66c0d7868b77863f57ce9c5bc76/glib/gtestutils.h#L592
[3] https://docs.gtk.org/glib/macros.html#compiler
Re: [PATCH 01/39] docs/spin: replace assert(0) with g_assert_not_reached()
Posted by Pierrick Bouvier 2 months, 1 week ago 
On 9/11/24 05:51, Richard W.M. Jones wrote:
> On Wed, Sep 11, 2024 at 02:46:18PM +0200, Maciej S. Szmigiero wrote:
>> On 11.09.2024 14:37, Eric Blake wrote:
>>> On Wed, Sep 11, 2024 at 07:33:59AM GMT, Eric Blake wrote:
>>>> On Tue, Sep 10, 2024 at 03:15:28PM GMT, Pierrick Bouvier wrote:
>>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>>>> ---
>>>>
>>>> A general suggestion for the entire series: please use a commit
>>>> message that explains why this is a good idea.  Even something as
>>>> boiler-plate as "refer to commit XXX for rationale" that can be
>>>> copy-pasted into all the other commits is better than nothing,
>>>> although a self-contained message is best.  Maybe:
>>>>
>>>> This patch is part of a series that moves towards a consistent use of
>>>> g_assert_not_reached() rather than an ad hoc mix of different
>>>> assertion mechanisms.
>>>
>>> Or summarize your cover letter:
>>>
>>> Use of assert(false) can trip spurious control flow warnings from some
>>> versions of gcc:
>>> https://lore.kernel.org/qemu-devel/54bb02a6-1b12-460a-97f6-3f478ef766c6@linaro.org/
>>> Solve that by unifying the code base on g_assert_not_reached()
>>> instead.
>>>
>>
>> If using g_assert_not_reached() instead of assert(false) silences
>> the warning about missing return value in such impossible to reach
>> locations should we also be deleting the now-unnecessary "return"
>> statements after g_assert_not_reached()?
> 
> Although it's unlikely to be used on any compiler that can also
> compile qemu, there is a third implementation of g_assert_not_reached
> that does nothing, see:
> 
> https://gitlab.gnome.org/GNOME/glib/-/blob/927683ebd94eb66c0d7868b77863f57ce9c5bc76/glib/gtestutils.h#L269
> 
> Rich.
> 

Interesting.
At least gcc, clang and msvc are covered, this should be ok for most of 
the builds.

Thanks for sharing,
Pierrick
Re: [PATCH 01/39] docs/spin: replace assert(0) with g_assert_not_reached()
Posted by Richard Henderson 2 months, 1 week ago 
On 9/11/24 08:25, Pierrick Bouvier wrote:
> On 9/11/24 05:51, Richard W.M. Jones wrote:
>> Although it's unlikely to be used on any compiler that can also
>> compile qemu, there is a third implementation of g_assert_not_reached
>> that does nothing, see:
>>
>> https://gitlab.gnome.org/GNOME/glib/-/blob/927683ebd94eb66c0d7868b77863f57ce9c5bc76/ 
>> glib/gtestutils.h#L269
>>
>> Rich.
>>
> 
> Interesting.
> At least gcc, clang and msvc are covered, this should be ok for most of the builds.

All of that is inside #ifdef G_DISABLE_ASSERT, which we will never set.


r~
Re: [PATCH 01/39] docs/spin: replace assert(0) with g_assert_not_reached()
Posted by Richard Henderson 2 months, 2 weeks ago 
On 9/10/24 15:15, Pierrick Bouvier wrote:
> Signed-off-by: Pierrick Bouvier<pierrick.bouvier@linaro.org>
> ---
>   docs/spin/aio_notify_accept.promela | 6 +++---
>   docs/spin/aio_notify_bug.promela    | 6 +++---
>   2 files changed, 6 insertions(+), 6 deletions(-)

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

r~

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.