[PATCH, tree-ssa] Avoid -Wuninitialized warning in try_unroll_loop_completely()

[PATCH, tree-ssa] Avoid -Wuninitialized warning in try_unroll_loop_completely()

[Chung-Ju Wu]

[-- Attachment #1: Type: text/plain, Size: 404 bytes --]

Hi,

I noticed there is an uninitialized variable warning
when compiling tree-ssa-loop-ivcanon.c file.

Attached patch is a slight modification to avoid the warning
and a plaintext ChangeLog is as below.

Is it OK for trunk?


2013-04-14  Chung-Ju Wu  <jasonwucj@gmail.com>

        * tree-ssa-loop-ivcanon.c (try_unroll_loop_completely): Avoid
        -Wuninitialized warning.


Best regards,
jasonwucj

[-- Attachment #2: gcc490-tree-ssa-loop-ivcanon.svn.patch --]
[-- Type: application/octet-stream, Size: 518 bytes --]

Index: gcc/tree-ssa-loop-ivcanon.c
===================================================================
--- gcc/tree-ssa-loop-ivcanon.c	(revision 197948)
+++ gcc/tree-ssa-loop-ivcanon.c	(working copy)
@@ -652,7 +652,8 @@
 			    HOST_WIDE_INT maxiter,
 			    location_t locus)
 {
-  unsigned HOST_WIDE_INT n_unroll, ninsns, max_unroll, unr_insns;
+  unsigned HOST_WIDE_INT n_unroll = 0;
+  unsigned HOST_WIDE_INT ninsns, max_unroll, unr_insns;
   gimple cond;
   struct loop_size size;
   bool n_unroll_found = false;

Re: [PATCH, tree-ssa] Avoid -Wuninitialized warning in try_unroll_loop_completely()

[Jeff Law]

On 04/13/2013 07:17 PM, Chung-Ju Wu wrote:
> Hi,
>
> I noticed there is an uninitialized variable warning
> when compiling tree-ssa-loop-ivcanon.c file.
>
> Attached patch is a slight modification to avoid the warning
> and a plaintext ChangeLog is as below.
>
> Is it OK for trunk?
>
>
> 2013-04-14  Chung-Ju Wu  <jasonwucj@gmail.com>
>
>          * tree-ssa-loop-ivcanon.c (try_unroll_loop_completely): Avoid
>          -Wuninitialized warning.
If this is a false positive (and I think it is from a very quick scan of 
the code), can you mark the initialization as such?

/* Avoid false positive -Wuninitialized warning.  */

Ideally this will become standard practice.

jeff

Re: [PATCH, tree-ssa] Avoid -Wuninitialized warning in try_unroll_loop_completely()

[Chung-Ju Wu]

[-- Attachment #1: Type: text/plain, Size: 1507 bytes --]

2013/4/15 Jeff Law <law@redhat.com>:
> On 04/13/2013 07:17 PM, Chung-Ju Wu wrote:
>>
>> Hi,
>>
>> I noticed there is an uninitialized variable warning
>> when compiling tree-ssa-loop-ivcanon.c file.
>>
>> Attached patch is a slight modification to avoid the warning
>> and a plaintext ChangeLog is as below.
>>
>> Is it OK for trunk?
>>
>>
>> 2013-04-14  Chung-Ju Wu  <jasonwucj@gmail.com>
>>
>>          * tree-ssa-loop-ivcanon.c (try_unroll_loop_completely): Avoid
>>          -Wuninitialized warning.
>
> If this is a false positive (and I think it is from a very quick scan of the
> code), can you mark the initialization as such?
>
> /* Avoid false positive -Wuninitialized warning.  */
>
> Ideally this will become standard practice.
>
> jeff

You are right.
After doing survey on http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings
and reading related discussion thread, I realized it is a
complicated detection and this is a false positive case.

I was using gcc-4.6.3, which is provided by Ubuntu 12.04,
and the warning is displayed during the compilation process.
As I tried to build another native gcc by myself with
current main trunk and used it to compile tree-ssa-loop-ivcanon.c again,
there is no such warning at all.
(See attachment for my console output.)

So I am wondering if my patch is still valuable since
such false positive warning is already fixed on trunk.

Or do you think it is still good having the comment in the patch
and then OK to commit it? :)


Best regards,
jasonwucj

[-- Attachment #2: console-output.txt --]
[-- Type: text/plain, Size: 1174 bytes --]


[jasonwucj@sw-compiler]$ g++ --version
g++ (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3
Copyright (C) 2011 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

[jasonwucj@sw-compiler]$ g++ tree-ssa-loop-ivcanon.ii -S -O2 -Wall
/home/jasonwucj/tmp/gcc-svn-trunk/gcc/tree-ssa-loop-ivcanon.c: In function ¡¥bool canonicalize_loop_induction_variables(loop*, bool, unroll_level, bool)¡¦:
/home/jasonwucj/tmp/gcc-svn-trunk/gcc/tree-ssa-loop-ivcanon.c:866:46: warning: ¡¥n_unroll¡¦ may be used uninitialized in this function [-Wuninitialized]
/home/jasonwucj/tmp/gcc-svn-trunk/gcc/tree-ssa-loop-ivcanon.c:655:17: note: ¡¥n_unroll¡¦ was declared here

[jasonwucj@sw-compiler]$ toolchain/bin/g++ --version
g++ (20130416) 4.9.0 20130415 (experimental)
Copyright (C) 2013 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

[jasonwucj@sw-compiler]$ toolchain/bin/g++ tree-ssa-loop-ivcanon.ii -S -O2 -Wall

[jasonwucj@sw-compiler]$

Re: [PATCH, tree-ssa] Avoid -Wuninitialized warning in try_unroll_loop_completely()

[Jeff Law]

On 04/15/2013 08:35 PM, Chung-Ju Wu wrote:
> You are right.
> After doing survey on http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings
> and reading related discussion thread, I realized it is a
> complicated detection and this is a false positive case.
>
> I was using gcc-4.6.3, which is provided by Ubuntu 12.04,
> and the warning is displayed during the compilation process.
> As I tried to build another native gcc by myself with
> current main trunk and used it to compile tree-ssa-loop-ivcanon.c again,
> there is no such warning at all.
> (See attachment for my console output.)
>
> So I am wondering if my patch is still valuable since
> such false positive warning is already fixed on trunk.
Thanks for checking on this stuff.  My preference would be to not add 
the initialization unless we're currently seeing false positive warnings 
with the trunk.

Anytime we add a dummy initialization like this to avoid a false 
positive, we run the risk of missing real bugs later if the nearby code 
is changed in such a way as to expose an uninitialized use, which we'd 
like to catch, but can't if we're inserted a dummy initialization.

Jeff

Re: [PATCH, tree-ssa] Avoid -Wuninitialized warning in try_unroll_loop_completely()

[Chung-Ju Wu]

2013/4/18 Jeff Law <law@redhat.com>:
> On 04/15/2013 08:35 PM, Chung-Ju Wu wrote:
>>
>> I was using gcc-4.6.3, which is provided by Ubuntu 12.04,
>> and the warning is displayed during the compilation process.
>> As I tried to build another native gcc by myself with
>> current main trunk and used it to compile tree-ssa-loop-ivcanon.c again,
>> there is no such warning at all.
>> (See attachment for my console output.)
>>
>> So I am wondering if my patch is still valuable since
>> such false positive warning is already fixed on trunk.
>
> Thanks for checking on this stuff.  My preference would be to not add the
> initialization unless we're currently seeing false positive warnings with
> the trunk.
>
> Anytime we add a dummy initialization like this to avoid a false positive,
> we run the risk of missing real bugs later if the nearby code is changed in
> such a way as to expose an uninitialized use, which we'd like to catch, but
> can't if we're inserted a dummy initialization.
>
> Jeff
>

I see.  Leaving it unchanged indeed helps to catch possible bug
when nearby code is modified.

I will not commit this patch.
Thanks for the review and I did learn a lot from it! :)


Best regards,
jasonwucj