| Message ID | 1345210444-2292-1-git-send-email-sw@weilnetz.de |
|---|---|
| State | Rejected |
| Headers | show |
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; > } > }
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.
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.
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?
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? >
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.
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.
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
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; } }
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(-)