Re: [Qemu-devel] [PATCH] monitor: Fix warning from clang

[Stefan Hajnoczi <stefanha@gmail.com>]

On Mon, Aug 20, 2012 at 09:17:51AM +0200, Markus Armbruster wrote:
> Stefan Weil <sw@weilnetz.de> writes:
> 
> > Am 17.08.2012 17:02, schrieb Luiz Capitulino:
> >> On Fri, 17 Aug 2012 16:41:34 +0200
> >> Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >>> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >>>
> >>>> On Fri, 17 Aug 2012 16:10:12 +0200
> >>>> Markus Armbruster <armbru@redhat.com> wrote:
> >>>>
> >>>>> Stefan Weil <sw@weilnetz.de> writes:
> >>>>>
> >>>>>> ccc-analyzer reports these warnings:
> >>>>>>
> >>>>>> monitor.c:3532:21: warning: Division by zero
> >>>>>>                  val %= val2;
> >>>>>>                      ^
> >>>>>> monitor.c:3530:21: warning: Division by zero
> >>>>>>                  val /= val2;
> >>>>>>                      ^
> >>>>>>
> >>>>>> Rewriting the code fixes this (and also a style issue).
> >>>>>
> >>>>> I'm afraid this doesn't actually fix anything, because...
> >>>>>
> >>>>>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> >>>>>> ---
> >>>>>>   monitor.c |    7 ++++---
> >>>>>>   1 file changed, 4 insertions(+), 3 deletions(-)
> >>>>>>
> >>>>>> diff --git a/monitor.c b/monitor.c
> >>>>>> index 0c34934..0ea2c14 100644
> >>>>>> --- a/monitor.c
> >>>>>> +++ b/monitor.c
> >>>>>> @@ -3524,12 +3524,13 @@ static int64_t expr_prod(Monitor *mon)
> >>>>>>               break;
> >>>>>>           case '/':
> >>>>>>           case '%':
> >>>>>> -            if (val2 == 0)
> >>>>>> +            if (val2 == 0) {
> >>>>>>                   expr_error(mon, "division by zero");
> >>>>>> -            if (op == '/')
> >>>>>> +            } else if (op == '/') {
> >>>>>>                   val /= val2;
> >>>>>> -            else
> >>>>>> +            } else {
> >>>>>>                   val %= val2;
> >>>>>> +            }
> >>>>>>               break;
> >>>>>>           }
> >>>>>>       }
> >>>>>
> >>>>> ... expr_error() longjmp()s out.  The expression evaluator commonly
> >>>>> exploits that.
> >>>>
> >>>> And that's correct. As far far I understood it's fixing clang, not qemu.
> >>>>
> >>>>> If expr_error() returned, the code would be just as wrong after your
> >>>>> patch as before.
> >>>>
> >>>> Hmm, how? It checks for val2 == 0 first.
> >>>
> >>> It would evaluate A % 0 into A, which is wrong.
> >>
> >> Oh, you're talking about the result that would be returned by expr_prod().
> >> I thought you were saying that val2 == 0 was still possible.
> >>
> >>>
> >>>>> Perhaps the checker can be shut up by making expr_error() QEMU_NORETURN.
> >>>>
> >>>> That's indeed a better solution.
> >>>
> >>> Stefan, could you try that for us?
> >
> >
> > Adding QEMU_NORETURN to function expr_error also
> > fixes the warning from ccc-analyzer.
> >
> > I'll send a patch series which adds this and some more
> > QEMU_NORETURN attributes.
> 
> Thanks!
> 
> > What about using above patch in addition? IMHO it
> > improves readability, and it fixes the coding style.
> 
> Readability: debatable.  The code depends on expr_error() not returning.
> The current code makes that fairly obvious locally.  I think your patch
> makes it less obvious.  Moreover, it changes the way exp_error() is used
> in just one place, making it inconsistent with all the other places.
> 
> Coding style: we generally make coding style changes only to code we
> touch anyway, not just for the sake of it.
> 
> TL;DR: let's drop this patch.

I agree.  Let's add QEMU_NORETURN and leave this code as-is.

Stefan