[Qemu-devel] fix unsigned comparison warning in TCG

[Qemu-devel] fix unsigned comparison warning in TCG

[Hollis Blanchard]

TCGOpcode is an enum, which apparently can be unsigned.

Signed-off-by: Hollis Blanchard <hollis@penguinppc.org>
---

% ./configure --target-list=ppcemb-softmmu --enable-debug
% make
  ...
  CC    ppcemb-softmmu/tcg/tcg.o
cc1: warnings being treated as errors
/home/hollisb/source/qemu.git/tcg/tcg.c: In function
‘tcg_add_target_add_op_defs’:
/home/hollisb/source/qemu.git/tcg/tcg.c:1030: error: comparison of
unsigned expression >= 0 is always true
% gcc -v
gcc version 4.4.4 20100630 (Red Hat 4.4.4-10) (GCC)

diff --git a/tcg/tcg.c b/tcg/tcg.c
index e0a9030..7e96859 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -1027,7 +1027,7 @@ void tcg_add_target_add_op_defs(const TCGTargetOpDef *tdef
         if (tdefs->op == (TCGOpcode)-1)
             break;
         op = tdefs->op;
-        assert(op >= 0 && op < NB_OPS);
+        assert(op < NB_OPS);
         def = &tcg_op_defs[op];
 #if defined(CONFIG_DEBUG_TCG)
         /* Duplicate entry in op definitions? */

Re: [Qemu-devel] fix unsigned comparison warning in TCG

[Blue Swirl]

On Sun, Sep 26, 2010 at 5:40 PM, Hollis Blanchard <hollis@penguinppc.org> wrote:
> TCGOpcode is an enum, which apparently can be unsigned.
>
> Signed-off-by: Hollis Blanchard <hollis@penguinppc.org>
> ---
>
> % ./configure --target-list=ppcemb-softmmu --enable-debug
> % make
>  ...
>  CC    ppcemb-softmmu/tcg/tcg.o
> cc1: warnings being treated as errors
> /home/hollisb/source/qemu.git/tcg/tcg.c: In function
> ‘tcg_add_target_add_op_defs’:
> /home/hollisb/source/qemu.git/tcg/tcg.c:1030: error: comparison of
> unsigned expression >= 0 is always true
> % gcc -v
> gcc version 4.4.4 20100630 (Red Hat 4.4.4-10) (GCC)
>
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index e0a9030..7e96859 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -1027,7 +1027,7 @@ void tcg_add_target_add_op_defs(const TCGTargetOpDef *tdef
>         if (tdefs->op == (TCGOpcode)-1)
>             break;
>         op = tdefs->op;
> -        assert(op >= 0 && op < NB_OPS);
> +        assert(op < NB_OPS);

Please add int cast, like 95ee3914bfd551aeec49932a400530141865acad.

Re: [Qemu-devel] fix unsigned comparison warning in TCG

[Venkateswararao Jujjuri (JV)]

On 9/26/2010 11:33 AM, Blue Swirl wrote:
> On Sun, Sep 26, 2010 at 5:40 PM, Hollis Blanchard <hollis@penguinppc.org> wrote:
>> TCGOpcode is an enum, which apparently can be unsigned.
>>
>> Signed-off-by: Hollis Blanchard <hollis@penguinppc.org>
>> ---
>>
>> % ./configure --target-list=ppcemb-softmmu --enable-debug
>> % make
>>  ...
>>  CC    ppcemb-softmmu/tcg/tcg.o
>> cc1: warnings being treated as errors
>> /home/hollisb/source/qemu.git/tcg/tcg.c: In function
>> ‘tcg_add_target_add_op_defs’:
>> /home/hollisb/source/qemu.git/tcg/tcg.c:1030: error: comparison of
>> unsigned expression >= 0 is always true
>> % gcc -v
>> gcc version 4.4.4 20100630 (Red Hat 4.4.4-10) (GCC)
>>
>> diff --git a/tcg/tcg.c b/tcg/tcg.c
>> index e0a9030..7e96859 100644
>> --- a/tcg/tcg.c
>> +++ b/tcg/tcg.c
>> @@ -1027,7 +1027,7 @@ void tcg_add_target_add_op_defs(const TCGTargetOpDef *tdef
>>         if (tdefs->op == (TCGOpcode)-1)
>>             break;
>>         op = tdefs->op;
>> -        assert(op >= 0 && op < NB_OPS);
>> +        assert(op < NB_OPS);
> 
> Please add int cast, like 95ee3914bfd551aeec49932a400530141865acad.
> 

What is the latest on this? Waiting for this fix as I use --enable-debug a lot. :)

- JV

[Qemu-devel] [PATCH] tcg: Fix compiler error (comparison of unsigned expression)

[Stefan Weil]

When qemu is configured with --enable-debug-tcg,
gcc throws this warning (or error with -Werror):

tcg/tcg.c:1030: error: comparison of unsigned expression >= 0 is always true

Fix it by removing the >= 0 part.
The type cast to 'unsigned' catches negative values of op
(which should never happen).

This is a modification of Hollis Blanchard's patch.

Cc: Hollis Blanchard <hollis@penguinppc.org>
Cc: Blue Swirl <blauwirbel@gmail.com>
Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 tcg/tcg.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tcg/tcg.c b/tcg/tcg.c
index e0a9030..0cdef0d 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -1027,7 +1027,7 @@ void tcg_add_target_add_op_defs(const TCGTargetOpDef *tdefs)
         if (tdefs->op == (TCGOpcode)-1)
             break;
         op = tdefs->op;
-        assert(op >= 0 && op < NB_OPS);
+        assert((unsigned)op < NB_OPS);
         def = &tcg_op_defs[op];
 #if defined(CONFIG_DEBUG_TCG)
         /* Duplicate entry in op definitions? */
-- 
1.7.1

[Qemu-devel] Re: [PATCH] tcg: Fix compiler error (comparison of unsigned expression)

[Hollis Blanchard]

On Fri, Oct 8, 2010 at 1:32 AM, Stefan Weil <weil@mail.berlios.de> wrote:
> When qemu is configured with --enable-debug-tcg,
> gcc throws this warning (or error with -Werror):
>
> tcg/tcg.c:1030: error: comparison of unsigned expression >= 0 is always true
>
> Fix it by removing the >= 0 part.
> The type cast to 'unsigned' catches negative values of op
> (which should never happen).
>
> This is a modification of Hollis Blanchard's patch.
>
> Cc: Hollis Blanchard <hollis@penguinppc.org>
> Cc: Blue Swirl <blauwirbel@gmail.com>
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> ---
>  tcg/tcg.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index e0a9030..0cdef0d 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -1027,7 +1027,7 @@ void tcg_add_target_add_op_defs(const TCGTargetOpDef *tdefs)
>         if (tdefs->op == (TCGOpcode)-1)
>             break;
>         op = tdefs->op;
> -        assert(op >= 0 && op < NB_OPS);
> +        assert((unsigned)op < NB_OPS);
>         def = &tcg_op_defs[op];
>  #if defined(CONFIG_DEBUG_TCG)
>         /* Duplicate entry in op definitions? */

According to the warning, op is already unsigned, so this simply
removes the >=0 test, which was my original patch.

In contrast, Blue wanted a cast to int, as seen in
95ee3914bfd551aeec49932a400530141865acad.

-Hollis

[Qemu-devel] Re: [PATCH] tcg: Fix compiler error (comparison of unsigned expression)

[Stefan Weil]

Am 08.10.2010 18:57, schrieb Hollis Blanchard:
> On Fri, Oct 8, 2010 at 1:32 AM, Stefan Weil <weil@mail.berlios.de> wrote:
>> When qemu is configured with --enable-debug-tcg,
>> gcc throws this warning (or error with -Werror):
>>
>> tcg/tcg.c:1030: error: comparison of unsigned expression >= 0 is 
>> always true
>>
>> Fix it by removing the >= 0 part.
>> The type cast to 'unsigned' catches negative values of op
>> (which should never happen).
>>
>> This is a modification of Hollis Blanchard's patch.
>>
>> Cc: Hollis Blanchard <hollis@penguinppc.org>
>> Cc: Blue Swirl <blauwirbel@gmail.com>
>> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
>> ---
>>  tcg/tcg.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/tcg/tcg.c b/tcg/tcg.c
>> index e0a9030..0cdef0d 100644
>> --- a/tcg/tcg.c
>> +++ b/tcg/tcg.c
>> @@ -1027,7 +1027,7 @@ void tcg_add_target_add_op_defs(const 
>> TCGTargetOpDef *tdefs)
>>         if (tdefs->op == (TCGOpcode)-1)
>>             break;
>>         op = tdefs->op;
>> -        assert(op >= 0 && op < NB_OPS);
>> +        assert((unsigned)op < NB_OPS);
>>         def = &tcg_op_defs[op];
>>  #if defined(CONFIG_DEBUG_TCG)
>>         /* Duplicate entry in op definitions? */
>
> According to the warning, op is already unsigned, so this simply
> removes the >=0 test, which was my original patch.
>
> In contrast, Blue wanted a cast to int, as seen in
> 95ee3914bfd551aeec49932a400530141865acad.
>
> -Hollis

I read the original thread. Michael already proposed the
unsigned cast in that thread.

Your patch is correct as long as we use gcc and as long
as gcc uses unsigned enums when possible (it is possible here).

Other compilers or new versions of gcc might use signed enums.
For those (and also for current gcc versions), my patch works.
Blue's solution also works but is a bit more code without
having advantages.

There are even more solutions which are accepted by the
compiler: ((op > first && op <= last) || (op == first))
with first, last being the first or last enum member.

- Stefan

Re: [Qemu-devel] Re: [PATCH] tcg: Fix compiler error (comparison of unsigned expression)

[Stefan Weil]

Am 08.10.2010 23:43, schrieb Stefan Weil:
> Am 08.10.2010 18:57, schrieb Hollis Blanchard:
>> On Fri, Oct 8, 2010 at 1:32 AM, Stefan Weil <weil@mail.berlios.de> 
>> wrote:
>>> When qemu is configured with --enable-debug-tcg,
>>> gcc throws this warning (or error with -Werror):
>>>
>>> tcg/tcg.c:1030: error: comparison of unsigned expression >= 0 is 
>>> always true
>>>
>>> Fix it by removing the >= 0 part.
>>> The type cast to 'unsigned' catches negative values of op
>>> (which should never happen).
>>>
>>> This is a modification of Hollis Blanchard's patch.
>>>
>>> Cc: Hollis Blanchard <hollis@penguinppc.org>
>>> Cc: Blue Swirl <blauwirbel@gmail.com>
>>> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
>>> ---
>>>  tcg/tcg.c |    2 +-
>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/tcg/tcg.c b/tcg/tcg.c
>>> index e0a9030..0cdef0d 100644
>>> --- a/tcg/tcg.c
>>> +++ b/tcg/tcg.c
>>> @@ -1027,7 +1027,7 @@ void tcg_add_target_add_op_defs(const 
>>> TCGTargetOpDef *tdefs)
>>>         if (tdefs->op == (TCGOpcode)-1)
>>>             break;
>>>         op = tdefs->op;
>>> -        assert(op >= 0 && op < NB_OPS);
>>> +        assert((unsigned)op < NB_OPS);
>>>         def = &tcg_op_defs[op];
>>>  #if defined(CONFIG_DEBUG_TCG)
>>>         /* Duplicate entry in op definitions? */
>>
>> According to the warning, op is already unsigned, so this simply
>> removes the >=0 test, which was my original patch.
>>
>> In contrast, Blue wanted a cast to int, as seen in
>> 95ee3914bfd551aeec49932a400530141865acad.
>>
>> -Hollis
>
> I read the original thread. Michael already proposed the
> unsigned cast in that thread.
>
> Your patch is correct as long as we use gcc and as long
> as gcc uses unsigned enums when possible (it is possible here).
>
> Other compilers or new versions of gcc might use signed enums.
> For those (and also for current gcc versions), my patch works.
> Blue's solution also works but is a bit more code without
> having advantages.
>
> There are even more solutions which are accepted by the
> compiler: ((op > first && op <= last) || (op == first))
> with first, last being the first or last enum member.
>
> - Stefan

Hollis, do you still see problems with my patch?
Or can it be committed?

Regards,
Stefan

Re: [Qemu-devel] Re: [PATCH] tcg: Fix compiler error (comparison of unsigned expression)

[Hollis Blanchard]

On Wed, Oct 13, 2010 at 11:58 AM, Stefan Weil <weil@mail.berlios.de> wrote:
>
> Hollis, do you still see problems with my patch?
> Or can it be committed?

I have no objection; I was just anticipating Blue's objection when I
commented previously. It's just a style question really...

-Hollis

Re: [Qemu-devel] Re: [PATCH] tcg: Fix compiler error (comparison of unsigned expression)

[Blue Swirl]

On Wed, Oct 13, 2010 at 7:22 PM, Hollis Blanchard <hollis@penguinppc.org> wrote:
> On Wed, Oct 13, 2010 at 11:58 AM, Stefan Weil <weil@mail.berlios.de> wrote:
>>
>> Hollis, do you still see problems with my patch?
>> Or can it be committed?
>
> I have no objection; I was just anticipating Blue's objection when I
> commented previously. It's just a style question really...

Both ((int)op >= 0 && op < NB_OPS) and ((unsigned int)op < NB_OPS)
produce same binary (if NB_OPS < MAX_INT). During the earlier, long
discussion, the int version was preferred.

[Qemu-devel] Re: [PATCH] tcg: Fix compiler error (comparison of unsigned expression)

[Blue Swirl]

Thanks, applied.

On Fri, Oct 8, 2010 at 8:32 AM, Stefan Weil <weil@mail.berlios.de> wrote:
> When qemu is configured with --enable-debug-tcg,
> gcc throws this warning (or error with -Werror):
>
> tcg/tcg.c:1030: error: comparison of unsigned expression >= 0 is always true
>
> Fix it by removing the >= 0 part.
> The type cast to 'unsigned' catches negative values of op
> (which should never happen).
>
> This is a modification of Hollis Blanchard's patch.
>
> Cc: Hollis Blanchard <hollis@penguinppc.org>
> Cc: Blue Swirl <blauwirbel@gmail.com>
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> ---
>  tcg/tcg.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index e0a9030..0cdef0d 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -1027,7 +1027,7 @@ void tcg_add_target_add_op_defs(const TCGTargetOpDef *tdefs)
>         if (tdefs->op == (TCGOpcode)-1)
>             break;
>         op = tdefs->op;
> -        assert(op >= 0 && op < NB_OPS);
> +        assert((unsigned)op < NB_OPS);
>         def = &tcg_op_defs[op];
>  #if defined(CONFIG_DEBUG_TCG)
>         /* Duplicate entry in op definitions? */
> --
> 1.7.1
>
>