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

[Jan Kiszka]

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