[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] monitor: Fix warning from clang
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH] monitor: Fix warning from clang |
Date: |
Mon, 20 Aug 2012 09:17:51 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) |
Stefan Weil <address@hidden> writes:
> Am 17.08.2012 17:02, schrieb Luiz Capitulino:
>> On Fri, 17 Aug 2012 16:41:34 +0200
>> Markus Armbruster <address@hidden> wrote:
>>
>>> Luiz Capitulino <address@hidden> writes:
>>>
>>>> On Fri, 17 Aug 2012 16:10:12 +0200
>>>> Markus Armbruster <address@hidden> wrote:
>>>>
>>>>> Stefan Weil <address@hidden> 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 <address@hidden>
>>>>>> ---
>>>>>> 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.