[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: typo in target-i386/ops_sse.h
From: |
Jan Kiszka |
Subject: |
[Qemu-devel] Re: typo in target-i386/ops_sse.h |
Date: |
Fri, 28 Nov 2008 09:23:45 +0100 |
User-agent: |
Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.8.1.12) Gecko/20080226 SUSE/2.0.0.12-1.1 Thunderbird/2.0.0.12 Mnenhy/0.7.5.666 |
andrzej zaborowski wrote:
> 2008/11/28 Jan Kiszka <address@hidden>:
>> andrzej zaborowski wrote:
>>> Hi,
>>>
>>> 2008/11/27 Frank Mehnert <address@hidden>:
>>>> 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
signature.asc
Description: OpenPGP digital signature