Stefan Weil <s...@weilnetz.de> writes: > Am 17.08.2012 17:02, schrieb Luiz Capitulino: >> On Fri, 17 Aug 2012 16:41:34 +0200 >> Markus Armbruster <arm...@redhat.com> wrote: >> >>> Luiz Capitulino <lcapitul...@redhat.com> writes: >>> >>>> On Fri, 17 Aug 2012 16:10:12 +0200 >>>> Markus Armbruster <arm...@redhat.com> wrote: >>>> >>>>> Stefan Weil <s...@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 <s...@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.