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

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

[Stefan Weil]

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

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;
         }
     }
-- 
1.7.10

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

[Luiz Capitulino]

On Fri, 17 Aug 2012 15:34:04 +0200
Stefan Weil <sw@weilnetz.de> wrote:

> 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).
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>

Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>

Although I wonder how far we're going "fixing" clang warnings/false positives...

> ---
>  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;
>          }
>      }

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

[Markus Armbruster]

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.

If expr_error() returned, the code would be just as wrong after your
patch as before.

Perhaps the checker can be shut up by making expr_error() QEMU_NORETURN.

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

[Luiz Capitulino]

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.

> Perhaps the checker can be shut up by making expr_error() QEMU_NORETURN.

That's indeed a better solution.

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

[Markus Armbruster]

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.

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

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

[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?
> 

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

[Stefan Weil]

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.

What about using above patch in addition? IMHO it
improves readability, and it fixes the coding style.

Regards,

Stefan W.

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

[Markus Armbruster]

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.

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

[Stefan Hajnoczi]

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