Skip to contentSkip to conversation

typo in target-i386/ops_sse.h

Qemu-devel Applied Frank Mehnert 4 participants 7 messages Nov 27, 2008 – Dec 2, 2008

lore
Share & tools
lore

Frank Mehnert

author
#0 lore raw

Hi,

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

ops_sse.h (working copy) renamed from ops_sse.h (revision 5805) +2 -2
@@ -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

Signature · 1 line
Dr.-Ing. Frank Mehnert    Sun Microsystems    http://www.sun.com/
#1 lore raw

Hi,

2008/11/27 Frank Mehnert <[email protected]>:

Quoted 2 lines

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:

target-i386/ops_sse.h +6 -6
@@ -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

#2 lore raw
Quoted 29 lines · andrzej zaborowski

Hi,

2008/11/27 Frank Mehnert <[email protected]>:

Quoted 2 lines

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:

target-i386/ops_sse.h +6 -6
@@ -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);\
         }\
Quoted 1 line

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

#3 lore raw

PATCH 1/1 fix dr register typo (was: typo in target-i386/ops_sse.h)

Quoted 3 lines · Jan Kiszka

...
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 <[email protected]>
 target-i386/helper.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
target-i386/helper.c +2 -2
index 037540d..f2d91df 100644
@@ -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)
Applied
#4 lore raw

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

Quoted 7 lines · Jan Kiszka
Quoted 4 lines · Jan Kiszka

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

Quoted 9 lines

...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 <[email protected]>

Applied. Thanks.

Regards,

Anthony Liguori

#5 lore raw
In reply to Jan Kiszka

2008/11/28 Jan Kiszka <[email protected]>:

Quoted 30 lines
Quoted 29 lines · andrzej zaborowski

Hi,

2008/11/27 Frank Mehnert <[email protected]>:

Quoted 2 lines

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:

target-i386/ops_sse.h +6 -6
@@ -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);\
         }\
Quoted 7 lines
Quoted 1 line

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.

Quoted 3 lines

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

#6 lore raw
Quoted 42 lines · andrzej zaborowski

2008/11/28 Jan Kiszka <[email protected]>:

Quoted 29 lines
Quoted 28 lines · andrzej zaborowski

Hi,

2008/11/27 Frank Mehnert <[email protected]>:

Quoted 2 lines

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:

target-i386/ops_sse.h +6 -6
@@ -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);\
         }\
Quoted 6 lines
Quoted 1 line

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.

Quoted 4 lines
Quoted 2 lines

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.

Quoted 3 lines

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