| Message ID | 20240910221606.1817478-2-pierrick.bouvier@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Use g_assert_not_reached instead of (g_)assert(0, false) | expand |
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.
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.
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
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.
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.
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
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
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~
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
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
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(-)