[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [PATCH 0/7] clean build - eliminate warnings
From: |
Jan Kiszka |
Subject: |
[Qemu-devel] Re: [PATCH 0/7] clean build - eliminate warnings |
Date: |
Sun, 22 Feb 2009 12:09:59 +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 |
Jan Kiszka wrote:
> Laurent Desnogues wrote:
>> On Sat, Feb 21, 2009 at 9:09 PM, Jan Kiszka <address@hidden> wrote:
>>> Laurent Desnogues wrote:
>>>> On Sat, Feb 21, 2009 at 8:00 PM, Jan Kiszka <address@hidden> wrote:
>>>>> When working on larger or intrusive changes like the monitor rework, the
>>>>> number of warnings a normal build generates (here: x86-64 host, gcc 4.3)
>>>>> is still too high. And sometimes these warnings are not just of cosmetic
>>>>> nature, see (reposted) patch 3.
>>>>>
>>>>> This series reduces the number of warnings significantly, still not to
>>>>> zero (someone would have to look into the NetWinder stuff), but almost:
>>>>>
>>>>> Warning summary for 2009-02-21 (changes since 2009-02-21-base)
>>>>> generic 0 (-1)
>>>>> softmmu 0 (-39)
>>>>> x86 0 (0)
>>>>> arm 0 (-10)
>>>> This means that after applying your patch there should be no more
>>>> warning for the ARM target?
>>> At least for softmmu, at least with my compiler (depending on the
>>> precise version / distro patches, you may have different warnings
>>> enabled by default): yes.
>> I built softmmu and my Makefile has no other warning than the
>> default.
>>
>>>> On my machine (x86_64, gcc 4.1.2), I still get these:
>>>>
>>>> CC arm-softmmu/neon_helper.o
>>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c: In
>>>> function ‘helper_neon_rshl_s8’:
>>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
>>>> warning: ‘vdest.v1’ is used uninitialized in this function
>>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
>>>> warning: ‘vdest.v2’ is used uninitialized in this function
>>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
>>>> warning: ‘vdest.v3’ is used uninitialized in this function
>>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
>>>> warning: ‘vdest.v4’ is used uninitialized in this function
>>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c: In
>>>> function ‘helper_neon_rshl_s16’:
>>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:470:
>>>> warning: ‘vdest.v1’ is used uninitialized in this function
>>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:470:
>>>> warning: ‘vdest.v2’ is used uninitialized in this function
>>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c: In
>>>> function ‘helper_neon_rshl_s32’:
>>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:471:
>>>> warning: ‘vdest.v1’ is used uninitialized in this function
>>> Has this been identified as a real issue or just compiler blindness (my
>>> series contains one "fix" for such blindness, see cris patch)? I'm
>>> currently a bit lost in those macros...
>> Yes, it's a real bug: dest has not been given a value. You can
>> look at preprocessed code :-)
>
> Yeah, preprocessed and Lindent'ed, this looks really buggy.
>
>> BTW, can gcc really say a value
>> is used uninitialized? I have seen it pretending "may be used
>> uninitialized" though it is, but it was always right when it says
>> "is used uninitialized".
>
> Obviously, detecting uninitialized variables with gcc still leaves room
> for improvement. Version 4.3 actually seem to have regressed in this case.
>
>>>> Note a patch has been proposed in the past (by Aurélien IIRC).
>>> Do you have a reference at hand?
>> Try this:
>>
>> http://article.gmane.org/gmane.comp.emulators.qemu/31206
>
> OK, that makes sense, partially. My feeling is that there are some more
> typos/thinkos in this code. First, the macro for 8/16/32 bit checks for
> a shift width of -8/-16/-32, but the 64-bit version uses -63?! And then
> we have this (for signed rshl):
>
> dest = src >> (width - 1);
> dest++;
> dest >>= 1;
>
> Looks like nothing else than dest = 0, no? Paul?
One more:
[NEON_FN for rshl_s8/16/32]
...
} else if (tmp == -sizeof(src1) * 8) { \
dest = src1 >> (tmp - 1); \
ie.
if (tmp == -8/-16/-32) {
dest = src1 >> (-9/-17/-33);
Jan
Re: [Qemu-devel] [PATCH 0/7] clean build - eliminate warnings, Laurent Desnogues, 2009/02/21