[Qemu-devel] typo in target-i386/ops_sse.h

[Qemu-devel] typo in target-i386/ops_sse.h

[Frank Mehnert]

[-- Attachment #1: Type: text/plain, Size: 661 bytes --]

Hi,

I believe there is a typo in target-i386/ops_sse.h in the macro
SSE_HELPER_F:

--- ops_sse.h   (revision 5805)
+++ ops_sse.h   (working copy)
@@ -1501,10 +1501,10 @@
     d->elem(1) = F(1);\
     d->elem(2) = F(2);\
     d->elem(3) = F(3);\
-    if (num > 3) {\
+    if (num > 4) {\
         d->elem(4) = F(4);\
         d->elem(5) = F(5);\
-        if (num > 5) {\
+        if (num > 6) {\
             d->elem(6) = F(6);\
             d->elem(7) = F(7);\
         }\

If num is 4, only the elements 0...3 should be set. Same for 5/6.

Kind regards,

Frank
-- 
Dr.-Ing. Frank Mehnert    Sun Microsystems    http://www.sun.com/

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [Qemu-devel] typo in target-i386/ops_sse.h

[andrzej zaborowski]

Hi,

2008/11/27 Frank Mehnert <Frank.Mehnert@sun.com>:
> I believe there is a typo in target-i386/ops_sse.h in the macro
> SSE_HELPER_F:

Ooops, you're right about the typo, but I think it should something like this:
--- a/target-i386/ops_sse.h
+++ b/target-i386/ops_sse.h
@@ -1499,12 +1499,12 @@ void glue(name, SUFFIX) (Reg *d, Reg *s)\
 {\
     d->elem(0) = F(0);\
     d->elem(1) = F(1);\
-    d->elem(2) = F(2);\
-    d->elem(3) = F(3);\
-    if (num > 3) {\
-        d->elem(4) = F(4);\
-        d->elem(5) = F(5);\
-        if (num > 5) {\
+    if (num > 2) {\
+        d->elem(2) = F(2);\
+        d->elem(3) = F(3);\
+        if (num > 4) {\
+            d->elem(4) = F(4);\
+            d->elem(5) = F(5);\
             d->elem(6) = F(6);\
             d->elem(7) = F(7);\
         }\

I'm not sure why this didn't generate warnings.

Cheers

[Qemu-devel] Re: typo in target-i386/ops_sse.h

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1426 bytes --]

andrzej zaborowski wrote:
> Hi,
> 
> 2008/11/27 Frank Mehnert <Frank.Mehnert@sun.com>:
>> I believe there is a typo in target-i386/ops_sse.h in the macro
>> SSE_HELPER_F:
> 
> Ooops, you're right about the typo, but I think it should something like this:
> --- a/target-i386/ops_sse.h
> +++ b/target-i386/ops_sse.h
> @@ -1499,12 +1499,12 @@ void glue(name, SUFFIX) (Reg *d, Reg *s)\
>  {\
>      d->elem(0) = F(0);\
>      d->elem(1) = F(1);\
> -    d->elem(2) = F(2);\
> -    d->elem(3) = F(3);\
> -    if (num > 3) {\
> -        d->elem(4) = F(4);\
> -        d->elem(5) = F(5);\
> -        if (num > 5) {\
> +    if (num > 2) {\
> +        d->elem(2) = F(2);\
> +        d->elem(3) = F(3);\
> +        if (num > 4) {\
> +            d->elem(4) = F(4);\
> +            d->elem(5) = F(5);\
>              d->elem(6) = F(6);\
>              d->elem(7) = F(7);\
>          }\
> 
> I'm not sure why this didn't generate warnings.

It does - with gcc4 (array subscript is above array bounds). I saw them
in kvm-userspace, but there were so many (a lot likely due to
non-upstream stuff) that I ignored them for now. Now your patch just
removed 8 upstream warnings. But is this stuff already in use? Should
cause subtle guest state corruptions if actually executed.

That reminds me that we should have a "zero new warnings policy" for
changes. But reality still looks different...

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] [PATCH] fix dr register typo (was: typo in target-i386/ops_sse.h)

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1403 bytes --]

Jan Kiszka wrote:
> ...
> That reminds me that we should have a "zero new warnings policy" for
> changes. But reality still looks different...

...and I'm the first one to shoot (I should really start building also
upstream qemu with gcc4):

--------->

Fix register name typo in dumping debug registers.

Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
 target-i386/helper.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/target-i386/helper.c b/target-i386/helper.c
index 037540d..f2d91df 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -647,7 +647,7 @@ void cpu_dump_state(CPUState *env, FILE *f,
         for(i = 0; i < 4; i++)
             cpu_fprintf(f, "DR%d=%016" PRIx64 " ", i, env->dr[i]);
         cpu_fprintf(f, "\nDR6=%016" PRIx64 " DR7=%016" PRIx64 "\n",
-                    env->dr[6], env->cr[7]);
+                    env->dr[6], env->dr[7]);
     } else
 #endif
     {
@@ -681,7 +681,7 @@ void cpu_dump_state(CPUState *env, FILE *f,
                     (uint32_t)env->cr[4]);
         for(i = 0; i < 4; i++)
             cpu_fprintf(f, "DR%d=%08x ", i, env->dr[i]);
-        cpu_fprintf(f, "\nDR6=%08x DR7=%08x\n", env->dr[6], env->cr[7]);
+        cpu_fprintf(f, "\nDR6=%08x DR7=%08x\n", env->dr[6], env->dr[7]);
     }
     if (flags & X86_DUMP_CCOP) {
         if ((unsigned)env->cc_op < CC_OP_NB)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Qemu-devel] [PATCH] fix dr register typo

[Anthony Liguori]

Jan Kiszka wrote:
> Jan Kiszka wrote:
>   
>> ...
>> That reminds me that we should have a "zero new warnings policy" for
>> changes. But reality still looks different...
>>     
>
> ...and I'm the first one to shoot (I should really start building also
> upstream qemu with gcc4):
>
> --------->
>
> Fix register name typo in dumping debug registers.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
>   
Applied.  Thanks.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: typo in target-i386/ops_sse.h

[andrzej zaborowski]

2008/11/28 Jan Kiszka <jan.kiszka@web.de>:
> andrzej zaborowski wrote:
>> Hi,
>>
>> 2008/11/27 Frank Mehnert <Frank.Mehnert@sun.com>:
>>> I believe there is a typo in target-i386/ops_sse.h in the macro
>>> SSE_HELPER_F:
>>
>> Ooops, you're right about the typo, but I think it should something like this:
>> --- a/target-i386/ops_sse.h
>> +++ b/target-i386/ops_sse.h
>> @@ -1499,12 +1499,12 @@ void glue(name, SUFFIX) (Reg *d, Reg *s)\
>>  {\
>>      d->elem(0) = F(0);\
>>      d->elem(1) = F(1);\
>> -    d->elem(2) = F(2);\
>> -    d->elem(3) = F(3);\
>> -    if (num > 3) {\
>> -        d->elem(4) = F(4);\
>> -        d->elem(5) = F(5);\
>> -        if (num > 5) {\
>> +    if (num > 2) {\
>> +        d->elem(2) = F(2);\
>> +        d->elem(3) = F(3);\
>> +        if (num > 4) {\
>> +            d->elem(4) = F(4);\
>> +            d->elem(5) = F(5);\
>>              d->elem(6) = F(6);\
>>              d->elem(7) = F(7);\
>>          }\
>>
>> I'm not sure why this didn't generate warnings.
>
> It does - with gcc4 (array subscript is above array bounds). I saw them
> in kvm-userspace, but there were so many (a lot likely due to
> non-upstream stuff) that I ignored them for now. Now your patch just
> removed 8 upstream warnings. But is this stuff already in use? Should
> cause subtle guest state corruptions if actually executed.

It is enabled if you specify SSE4.1 support through -cpu, currenlty no
predefined cpu uses it.  I think it went unnoticed because I only
tested the first of the 12 instructions using the macro, which wasn't
affected.

>
> That reminds me that we should have a "zero new warnings policy" for
> changes. But reality still looks different...

Well, the subscripts above array bounds here are okay.  Similarly
there are other warnings that generate lots of annoying
false-positives and you would end up working around your compiler,
sometimes sacrificing readability or performance.

Cheers

[Qemu-devel] Re: typo in target-i386/ops_sse.h

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2933 bytes --]

andrzej zaborowski wrote:
> 2008/11/28 Jan Kiszka <jan.kiszka@web.de>:
>> andrzej zaborowski wrote:
>>> Hi,
>>>
>>> 2008/11/27 Frank Mehnert <Frank.Mehnert@sun.com>:
>>>> I believe there is a typo in target-i386/ops_sse.h in the macro
>>>> SSE_HELPER_F:
>>> Ooops, you're right about the typo, but I think it should something like this:
>>> --- a/target-i386/ops_sse.h
>>> +++ b/target-i386/ops_sse.h
>>> @@ -1499,12 +1499,12 @@ void glue(name, SUFFIX) (Reg *d, Reg *s)\
>>>  {\
>>>      d->elem(0) = F(0);\
>>>      d->elem(1) = F(1);\
>>> -    d->elem(2) = F(2);\
>>> -    d->elem(3) = F(3);\
>>> -    if (num > 3) {\
>>> -        d->elem(4) = F(4);\
>>> -        d->elem(5) = F(5);\
>>> -        if (num > 5) {\
>>> +    if (num > 2) {\
>>> +        d->elem(2) = F(2);\
>>> +        d->elem(3) = F(3);\
>>> +        if (num > 4) {\
>>> +            d->elem(4) = F(4);\
>>> +            d->elem(5) = F(5);\
>>>              d->elem(6) = F(6);\
>>>              d->elem(7) = F(7);\
>>>          }\
>>>
>>> I'm not sure why this didn't generate warnings.
>> It does - with gcc4 (array subscript is above array bounds). I saw them
>> in kvm-userspace, but there were so many (a lot likely due to
>> non-upstream stuff) that I ignored them for now. Now your patch just
>> removed 8 upstream warnings. But is this stuff already in use? Should
>> cause subtle guest state corruptions if actually executed.
> 
> It is enabled if you specify SSE4.1 support through -cpu, currenlty no
> predefined cpu uses it.  I think it went unnoticed because I only
> tested the first of the 12 instructions using the macro, which wasn't
> affected.
> 
>> That reminds me that we should have a "zero new warnings policy" for
>> changes. But reality still looks different...
> 
> Well, the subscripts above array bounds here are okay.  Similarly

Nope, they aren't. These warning are _directly_ related to the incorrect
element number checks you fixed.

> there are other warnings that generate lots of annoying
> false-positives and you would end up working around your compiler,
> sometimes sacrificing readability or performance.

The only "unfixable" warnings that I currently see with qemu's CFLAGS
and my compiler defaults are either related to will-die-soon dyngen
fragments or gcc's blindness when it comes to understanding

	if (condition)
		var = initialized;
	...
	if (condition)
		use(var);

and not warning about potentially uninitialized 'var' (even if
'condition' is stable). The rest is fixable or even pointing to code
worth a second look. To give another example for a bug one could have
found with the help of the compiler (to be fair: unsupported gcc4):
gdbstub.c accesses fpr 32..63 instead of 0..31 on PPC (patch will follow).

And don't underestimate the value of a warning free build, specifically
when you have to apply changes with broad impact!

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]