[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Stefan Weil
Subject: Re: [Qemu-trivial] [Qemu-devel] [PATCH] monitor: Fix warning from clang
Date: Sat, 18 Aug 2012 13:49:56 +0200
User-agent: Mozilla/5.0 (X11; Linux i686; rv:14.0) Gecko/20120714 Thunderbird/14.0

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

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


Stefan W.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]