User account creation filtered due to spam.

Bug 38851 - [4.4 regression] Compiler warns about uninitialized variable that is an object with a constructor
Summary: [4.4 regression] Compiler warns about uninitialized variable that is an objec...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 4.4.0
: P1 normal
Target Milestone: 4.4.0
Assignee: Richard Biener
URL:
Keywords: diagnostic, missed-optimization
Depends on:
Blocks:
 
Reported: 2009-01-14 21:04 UTC by Neil Vachharajani
Modified: 2009-01-26 09:53 UTC (History)
7 users (show)

See Also:
Host: i686-unknown-linux-gnu
Target: i686-unknown-linux-gnu
Build: i686-unknown-linux-gnu
Known to work: 4.3.2
Known to fail:
Last reconfirmed: 2009-01-25 22:02:56


Attachments
Test case program (261 bytes, text/x-c++src)
2009-01-14 21:05 UTC, Neil Vachharajani
Details
semi-working patch (1.04 KB, patch)
2009-01-24 14:23 UTC, Richard Biener
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Neil Vachharajani 2009-01-14 21:04:47 UTC
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).
Comment 1 Neil Vachharajani 2009-01-14 21:05:06 UTC
Created attachment 17104 [details]
Test case program
Comment 2 Andrew Pinski 2009-01-14 21:58:22 UTC
  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.
Comment 3 Benjamin Kosnik 2009-01-21 22:19:31 UTC
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
Comment 4 Benjamin Kosnik 2009-01-21 22:41:50 UTC
fixed summary
Comment 5 Richard Biener 2009-01-21 22:49:11 UTC
I think it is the frontends business to omit the stores.
Comment 6 Andrew Pinski 2009-01-21 22:59:06 UTC
(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.

Comment 7 Wolfgang Bangerth 2009-01-23 19:31:49 UTC
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.
Comment 8 Richard Biener 2009-01-24 09:22:13 UTC
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).
Comment 9 Richard Biener 2009-01-24 09:27:19 UTC
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);
Comment 10 Richard Biener 2009-01-24 10:06:19 UTC
Bah, the C++ frontend can return NULL_TREE from its expr_size langhook.  How
bad of it.
Comment 11 Richard Biener 2009-01-24 14:23:18 UTC
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
Comment 12 Richard Biener 2009-01-24 14:24:01 UTC
Probably because the FE looks into the gimplified code again?  Well - unassigning.
Comment 13 Richard Biener 2009-01-25 18:02:09 UTC
*** Bug 38908 has been marked as a duplicate of this bug. ***
Comment 14 Mark Mitchell 2009-01-25 19:45:07 UTC
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
Comment 15 rguenther@suse.de 2009-01-25 19:59:58 UTC
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.
Comment 16 Mark Mitchell 2009-01-25 20:03:56 UTC
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,

Comment 17 rguenther@suse.de 2009-01-25 20:45:38 UTC
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.
Comment 18 Richard Biener 2009-01-25 22:02:56 UTC
I am testing another patch, improving simple-DSE instead.
Comment 19 Richard Biener 2009-01-26 09:53:00 UTC
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

Comment 20 Richard Biener 2009-01-26 09:53:20 UTC
Fixed.
Comment 21 hjl@gcc.gnu.org 2009-01-30 17:32:06 UTC
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