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

[andrzej zaborowski]

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