Compiling the following program: int printf(const char *fmt, ...); class MyCompare { public: MyCompare() { } bool operator() (const int a, const int b) const { return a > b; } }; template<class Comp> class Other { public: Other (const Comp &c) : c_(c) { } void* doSomething(); private: Comp c_; }; template<class Comp> void* Other<Comp>::doSomething() { return &c_; } int main(int argc, char **argv) { MyCompare c; Other<MyCompare> other(c); printf("%p\n", other.doSomething()); return 0; } yields the following warning: test.cc: In function 'int main(int, char**)': test.cc:14: warning: 'c' is used uninitialized in this function test.cc:27: note: 'c' was declared here when compiled with the following command line: g++ -Wall -c -O2 test.cc However, c is initialized by the no argument constructor (further class MyCompare does not actually have any member variables to initialize).
Created attachment 17104 [details] Test case program
other.c_ = c; The issue here is that MyCompare is an empty struct (but has a sizeof of 1 because C++ standard) but GCC is not deleting stores for some reason.
Confirmed for s390x. This is causing the following s390x libstdc++ testsuite failures, and is thus a regression from 4.3. As far as I can tell these warnings are spurious. http://gcc.gnu.org/ml/gcc-testresults/2009-01/msg02102.html And from the libstdc++.log file: FAIL: 27_io/ios_base/types/fmtflags/bitmask_operators.cc (test for excess errors) Excess errors: /build-extralong/BUILD/gcc/libstdc++-v3/testsuite/util/testsuite_common_types.h:401: warning: 'a' is used uninitialized in this function /build-extralong/BUILD/gcc/libstdc++-v3/testsuite/util/testsuite_common_types.h:401: warning: 'b' is used uninitialized in this function /build-extralong/BUILD/gcc/libstdc++-v3/testsuite/util/testsuite_common_types.h:413: warning: 'b' is used uninitialized in this function FAIL: 27_io/ios_base/types/iostate/bitmask_operators.cc (test for excess errors) /build-extralong/BUILD/gcc/libstdc++-v3/testsuite/util/testsuite_common_types.h:401: warning: 'a' is used uninitialized in this function /build-extralong/BUILD/gcc/libstdc++-v3/testsuite/util/testsuite_common_types.h:401: warning: 'b' is used uninitialized in this function /build-extralong/BUILD/gcc/libstdc++-v3/testsuite/util/testsuite_common_types.h:413: warning: 'b' is used uninitialized in this function FAIL: 27_io/ios_base/types/openmode/bitmask_operators.cc (test for excess errors) /build-extralong/BUILD/gcc/libstdc++-v3/testsuite/util/testsuite_common_types.h:401: warning: 'a' is used uninitialized in this function /build-extralong/BUILD/gcc/libstdc++-v3/testsuite/util/testsuite_common_types.h:401: warning: 'b' is used uninitialized in this function /build-extralong/BUILD/gcc/libstdc++-v3/testsuite/util/testsuite_common_types.h:413: warning: 'b' is used uninitialized in this function
fixed summary
I think it is the frontends business to omit the stores.
(In reply to comment #5) > I think it is the frontends business to omit the stores. Or the gimplifier like what happens for GNU C code: /* For zero sized types only gimplify the left hand side and right hand side as statements and throw away the assignment. Do this after gimplify_modify_expr_rhs so we handle TARGET_EXPRs of addressable types properly. */ The reason why it does not do it for C++ code is because the struct's size are really 1 instead of 0.
I see this as well. It triggers a lot when using boost::graph which uses empty classes as tags all over the place. A simple case with boost::graph would be this: -------------------------- #include <boost/graph/adjacency_list.hpp> using namespace boost; adjacency_list<vecS, vecS, undirectedS, property<vertex_color_t, default_color_type, property<vertex_degree_t,int> > > g; void create_graph () { add_edge(1,2, g); } -------------------------- boost/graph/detail/adjacency_list.hpp: In function 'void create_graph()': boost/graph/detail/adjacency_list.hpp:819: warning: 'p' may be used uninitialized in this function boost/graph/detail/adjacency_list.hpp:2210: note: 'p' was declared here This problem makes -Wuninitialize pretty much useless for anyone who uses boost::graph. I think it would be a shame if we shipped a compiler that has a problem with this. W.
Smaller testcase: struct Empty { Empty() {} }; struct Other { Other(const Empty& e_) : e(e_) {} Empty e; }; void bar(Other&); void foo() { Empty e; Other o(e); bar(o); } RTL expansion removes the assignment, so we should be able to use the same reasoning to disable the warning and/or to get rid of the assignment on the tree level. RTL uses expr_size() here, which yields const0_rtx for e and o. As this involves a langhook I think the correct thing is to fix the missed-optimization and remove these stores during gimplification (or from within the frontend, of course).
I am testing the following. @@ -4195,7 +4184,8 @@ gimplify_modify_expr (tree *expr_p, gimp side as statements and throw away the assignment. Do this after gimplify_modify_expr_rhs so we handle TARGET_EXPRs of addressable types properly. */ - if (zero_sized_type (TREE_TYPE (*from_p)) && !want_value) + if (int_expr_size (*from_p) == 0 + && !want_value) { gimplify_stmt (from_p, pre_p); gimplify_stmt (to_p, pre_p);
Bah, the C++ frontend can return NULL_TREE from its expr_size langhook. How bad of it.
Created attachment 17175 [details] semi-working patch Patch. Regresses === g++ tests === Running target unix FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 51) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 59) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 60) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 61) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 62) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 66) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 74) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 75) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 76) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 77) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 78) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 79) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 83) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 91) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 92) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 93) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 94) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 98) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 106) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 107) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 108) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 109) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 110) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 111) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 115) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 123) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 124) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 125) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 126) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 130) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 138) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 139) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 140) FAIL: g++.dg/cpp0x/rv1n.C (test for errors, line 141) FAIL: g++.dg/cpp0x/rv1n.C (test for excess errors) FAIL: g++.dg/cpp0x/rv1p.C (test for excess errors) ... (some more of these) FAIL: g++.dg/cpp0x/rv7n.C (test for errors, line 83) FAIL: g++.dg/cpp0x/rv7n.C (test for excess errors) FAIL: g++.dg/cpp0x/rv7p.C (test for excess errors) FAIL: g++.dg/cpp0x/rv8p.C (test for excess errors) FAIL: g++.old-deja/g++.other/empty1.C execution test
Probably because the FE looks into the gimplified code again? Well - unassigning.
*** Bug 38908 has been marked as a duplicate of this bug. ***
Richard -- I don't agree that it's necessarily the FE's job to omit all stores to such types. Our general theory is that FEs get to emit dumb code and the optimizers get to fix it up. Of course, I don't object to making the FE generate simpler code, if that's easy to do; just that if our design relies on that, I think that's a mistake. I can imagine ways this could come up in other languages as well. For example, copying a C structure with an anonymous bit-field, but no other content, or an Ada record that uses Ada's layout directives to control size. Therefore, I don't think that the key here is "zero-size". Instead, it's the fact that structure cannot be initialized. That's useful both for warnings and for optimization; it can't be initialized, so there's no point about warning about uninitialized uses, and there's no reason to actually generate code for the copies. That leads to something I do think is something that the FEs could be asked to do: set a bit on the type to indicate that it is uninitializable or, if you like, logically empty. I also don't see this as a P1 defect. It's certainly annoying, but, fundamentally, it limits the utility of a warning which has been known to give false positives for a long time. Important to fix, yes -- but as important as generating wrong code? -- Mark
Subject: Re: [4.4 regression] Compiler warns about uninitialized variable that is an object with a constructor On Sun, 25 Jan 2009, mmitchel at gcc dot gnu dot org wrote: > ------- Comment #14 from mmitchel at gcc dot gnu dot org 2009-01-25 19:45 ------- > Richard -- > > I don't agree that it's necessarily the FE's job to omit all stores to such > types. Our general theory is that FEs get to emit dumb code and the optimizers > get to fix it up. Of course, I don't object to making the FE generate simpler > code, if that's easy to do; just that if our design relies on that, I think > that's a mistake. Oh, I agree. See my attempt to fix it during gimplification. > I can imagine ways this could come up in other languages as well. For example, > copying a C structure with an anonymous bit-field, but no other content, or an > Ada record that uses Ada's layout directives to control size. > > Therefore, I don't think that the key here is "zero-size". Instead, it's the > fact that structure cannot be initialized. That's useful both for warnings and > for optimization; it can't be initialized, so there's no point about warning > about uninitialized uses, and there's no reason to actually generate code for > the copies. Ok, I think mapping cannot be initialized to zero-size is ok, as that is the only thing we can currently query (and we even specialize this for C++ to deal with the 1 byte vs. empty case). > That leads to something I do think is something that the FEs could be asked to > do: set a bit on the type to indicate that it is uninitializable or, if you > like, logically empty. > > I also don't see this as a P1 defect. It's certainly annoying, but, > fundamentally, it limits the utility of a warning which has been known to give > false positives for a long time. Important to fix, yes -- but as important as > generating wrong code? It's a P1 defect as we didn't warn for uninitialized structure uses in any previous relelase. While we can argue that it is safe to downgrade this to P2 I think we should at least try to fix this issue for 4.4.0. Richard.
Subject: Re: [4.4 regression] Compiler warns about uninitialized variable that is an object with a constructor rguenther at suse dot de wrote: >> Therefore, I don't think that the key here is "zero-size". Instead, it's the >> fact that structure cannot be initialized. That's useful both for warnings and >> for optimization; it can't be initialized, so there's no point about warning >> about uninitialized uses, and there's no reason to actually generate code for >> the copies. > > Ok, I think mapping cannot be initialized to zero-size is ok, as that is > the only thing we can currently query (and we even specialize this > for C++ to deal with the 1 byte vs. empty case). Yes, I think it's OK to approximate "logically empty" by "zero-size" at present. It might be worth either changing the zero-size documentation/name to reflect that it means "logically empty" (if we think these are the same concept) or else defining a separate LOGICALLY_EMPTY_P predicate (implemented by checking for zero size) as a hedge against separating them (if we think they are usefully distinct concepts). > It's a P1 defect as we didn't warn for uninitialized structure > uses in any previous relelase. While we can argue that it is safe > to downgrade this to P2 I think we should at least try to fix this > issue for 4.4.0. I don't mind fixing it, of course, and it would certainly be better to do so. But, at the end of the day, if everything else is ready, I'd be opposed to holding up the release for this. Thanks,
Subject: Re: [4.4 regression] Compiler warns about uninitialized variable that is an object with a constructor On Sun, 25 Jan 2009, mark at codesourcery dot com wrote: > ------- Comment #16 from mark at codesourcery dot com 2009-01-25 20:03 ------- > Subject: Re: [4.4 regression] Compiler warns about > uninitialized variable that is an object with a constructor > > rguenther at suse dot de wrote: > > > It's a P1 defect as we didn't warn for uninitialized structure > > uses in any previous relelase. While we can argue that it is safe > > to downgrade this to P2 I think we should at least try to fix this > > issue for 4.4.0. > > I don't mind fixing it, of course, and it would certainly be better to > do so. But, at the end of the day, if everything else is ready, I'd be > opposed to holding up the release for this. I agree. Sometimes having one more priority inbetween P2 and P1 would be nice ;) Richard.
I am testing another patch, improving simple-DSE instead.
Subject: Bug 38851 Author: rguenth Date: Mon Jan 26 09:52:48 2009 New Revision: 143672 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=143672 Log: 2009-01-26 Richard Guenther <rguenther@suse.de> PR middle-end/38851 * Makefile.in (tree-ssa-dse.o): Add langhooks.h. * tree-ssa-dse.c: Include langhooks.h (execute_simple_dse): Remove stores with zero size. * g++.dg/warn/Wuninitialized-1.C: New testcase. Added: trunk/gcc/testsuite/g++.dg/warn/Wuninitialized-1.C Modified: trunk/gcc/ChangeLog trunk/gcc/Makefile.in trunk/gcc/testsuite/ChangeLog trunk/gcc/tree-ssa-dse.c
Fixed.
Subject: Bug 38851 Author: hjl Date: Fri Jan 30 17:31:24 2009 New Revision: 143798 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=143798 Log: 2009-01-30 H.J. Lu <hongjiu.lu@intel.com> 2009-01-27 Richard Guenther <rguenther@suse.de> PR tree-optimization/38503 * g++.dg/warn/Wstrict-aliasing-bogus-placement-new.C: New testcase. 2009-01-26 Richard Guenther <rguenther@suse.de> PR tree-optimization/38745 * g++.dg/torture/pr38745.C: New testcase. 2009-01-26 Richard Guenther <rguenther@suse.de> PR middle-end/38851 * g++.dg/warn/Wuninitialized-1.C: New testcase. 2009-01-20 Andrew Pinski <andrew_pinski@playstation.sony.com> Richard Guenther <rguenther@suse.de> PR tree-optimization/38747 PR tree-optimization/38748 * gcc.dg/tree-ssa/struct-aliasing-1.c: New test. * gcc.c-torture/execute/struct-aliasing-1.c: Likewise. Added: branches/gcc-4_3-branch/gcc/testsuite/g++.dg/torture/pr38745.C - copied unchanged from r143797, trunk/gcc/testsuite/g++.dg/torture/pr38745.C branches/gcc-4_3-branch/gcc/testsuite/g++.dg/warn/Wstrict-aliasing-bogus-placement-new.C - copied unchanged from r143797, trunk/gcc/testsuite/g++.dg/warn/Wstrict-aliasing-bogus-placement-new.C branches/gcc-4_3-branch/gcc/testsuite/g++.dg/warn/Wuninitialized-1.C - copied unchanged from r143797, trunk/gcc/testsuite/g++.dg/warn/Wuninitialized-1.C branches/gcc-4_3-branch/gcc/testsuite/gcc.c-torture/execute/struct-aliasing-1.c - copied unchanged from r143797, trunk/gcc/testsuite/gcc.c-torture/execute/struct-aliasing-1.c branches/gcc-4_3-branch/gcc/testsuite/gcc.dg/tree-ssa/struct-aliasing-1.c - copied unchanged from r143797, trunk/gcc/testsuite/gcc.dg/tree-ssa/struct-aliasing-1.c Modified: branches/gcc-4_3-branch/gcc/testsuite/ChangeLog