Re: [patch] Add a new warning flag -Wself-assign

[Le-Chun Wu <lcwu at google dot com>]

Andrew,

On Fri, May 28, 2010 at 4:52 PM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Fri, May 28, 2010 at 4:47 PM, Le-Chun Wu <lcwu@google.com> wrote:
>> As mentioned above, this flag warns about self-initialization as well.
>> Unlike the existing ?-Winit-self flag, this new flag does not require
>> the use of -Wuninitialized.
>
> Well I think we should not warn about local variables with this option
> because that was designed to get rid of the uninitialized warning.
> The reason why I mention this is because -Wuninitialized is now
> supported at -O0.

It should be pretty easy to modify this patch to not warn about
self-initialization of local variables with this flag. However, what I
don't understand is why a user would want to work around (or suppress)
an uninitialized warning by doing self-initialization. They could have
just initialized a local variable to some other more meaningful value.
Initializing a variable with itself doesn't seem like a good way to
fix an uninitialized warning.

>
>
>> ? ? ? ?class Foo {
>> ? ? ? ? private:
>> ? ? ? ? ?int a_;
>>
>> ? ? ? ? public:
>> ? ? ? ? ?Foo() : a_(a_) {} // warns with -Wself-assign, but not with
>> -Winit-self
>> ? ? ? ?};
>
> I think the above should warn under a different flag and it is
> recorded as http://gcc.gnu.org/PR18016 .

I am not sure whether the main purpose of -Winit-self is to work
around uninitialized warnings, but if we are going to warn this case
under a different flag, it seems to me that -Winit-self is the most
natural choice. We could easily modify the part of the code in my
patch that warns about self-initialization (of global variables, local
variables, and class members) to be controlled under -Winit-self. And
in that case I think that -Winit-self should not be gated by
-Wuninitialized.

>
> I think this warning (except for auto variables) should be turned on with -Wall.

I am all for that. In fact, if we are going to make -Winit-self
independent of -Wuninitialized, I would think that -Winit-self should
also be enabled with -Wall.

>>        (set_expr_folded_flag): New helper function.
>>        (fold_unary_loc_1): Renamed from fold_unary_loc.
>>        (fold_unary_loc): A wrapper around fold_unary_loc_1 function that sets
>>        the EXPR_FOLDED flag of the folded expression if folding is
>>        successful.
>>        (fold_binary_loc_1): Renamed from fold_binary_loc.
>>        (fold_binary_loc): A wrapper around fold_binary_loc_1 function that
>>        sets the EXPR_FOLDED flag of the folded expression if folding is
>>        successful.
>>        (fold_ternary_loc_1): Renamed from fold_ternary_loc.
>>        (fold_ternary_loc): A wrapper around fold_ternary_loc_1 function that
>>        sets the EXPR_FOLDED flag of the folded expression if folding is
>>        successful.
>
> This seems wrong and really bad idea.  There seems like you should
> have a better way of implementing this without this much extra code to
> fold-const.c which is called all through out the middle-end.  I think
> if you do add this code, I think you should do some compile time
> comparison to make sure it does not slow down the compiler this much.

The reason why I had to add a flag in expr and set it after folding an
expression is because the C frontend is too eager to constant-fold an
expression at parse time. For example, when parsing an assignment x =
x + 0 (which we don't want to warn under -Wself-assign), what we get
after paring the statement is already x = x (while, on the other hand,
C++ parser would return x = x + 0 first, and then call the folding
routines). While I would welcome any other better ideas, I did some
compile time tests with my patch and the results show that the code
does not slow down the compiler. Diego pointed me to the GCC
PerformanceTesting wiki page that contains some compile-time test
instructions, but there are too many packages to download and set up,
so I picked some of the largest GCC source files to measure the
compile time with and without this patch. Here are the results:

The source trees were sync'ed to revision 160300. The experiments were
conducted on an Intel Core2 Duo (2.4 GHz) system with 8GB of memory
running Ubuntu (Linux kernel 2.6.24). The data shown below are
user+system time in seconds. Each number is the average of 3 runs. The
first column (New) shows the time with the patch, the second column
(Old)  without, and the third column (Diff) the diff percentage. (Note
the source files were all preprocessed before measuring the compile
time.)

                New     Old     Diff
-O1
combine.c       4.29    4.30    -0.06%
expr.c          4.77    4.81    -0.72%
fold-const.c   16.13   16.19    -0.37%
parser.c        5.90    5.91    -0.18%
pt.c           12.70   12.63     0.52%

-O2
combine.c       6.01    6.01    -0.02%
expr.c          6.51    6.43     1.14%
fold-const.c   22.35   22.45    -0.46%
parser.c        8.09    8.11    -0.25%
pt.c           16.65   16.74    -0.53%

-O3
combine.c       7.14    7.22    -1.07%
expr.c          7.71    7.73    -0.28%
fold-const.c   26.22   26.32    -0.40%
parser.c       10.02   10.02    -0.05%
pt.c           20.23   20.25    -0.11%


The compile time for both cases are practically the same (although
somehow with my patch the compile time appears to be slightly better,
which I don't know why. :-))). If there are any other publicly
available source files that you think I should measure their compile
time, I'd be happy to do it.

Thanks,

Le-chun