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

[Stefan Weil <sw@weilnetz.de>]

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.