Bug 2972 - -Wuninitialized could warn about uninitialized member variable usage in constructors
-Wuninitialized could warn about uninitialized member variable usage in const...
Status: NEW
Product: gcc
Classification: Unclassified
Component: c++
3.4.0
: P3 enhancement
: ---
Assigned To: Not yet assigned to anyone
: diagnostic
Depends on: 19808
Blocks: Wuninitialized
  Show dependency treegraph
 
Reported: 2001-05-27 04:56 UTC by pere
Modified: 2014-09-29 14:53 UTC (History)
9 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2008-09-10 15:30:28


Attachments
gcc-member-init.cpp (555 bytes, text/plain)
2003-05-21 15:16 UTC, pere
Details
better -Wmeminit patch (1.62 KB, patch)
2010-06-03 11:24 UTC, Jonathan Wakely
Details | Diff
updated -Wmeminit patch for trunk (4.6.0) (1.72 KB, patch)
2010-12-03 19:07 UTC, Jonathan Wakely
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description pere 2001-05-27 04:56:00 UTC
If I read clause 12.6.2, 8.5 and 3.9 correctly, all scalar
member variables shall be default initialized to 0 unless
there is an explicit initialization in the constructor. The attached source demonstrate the problem.  The behaviour
is the same for gcc 2.95.2 and gcc 3.0 (using the internet
test compiler.)

A workaround is to initialize all member variables
explicitly, adding ': i(), p()' to the ctor in the example.

Release:
2.95 -> 3.4

Environment:
Debian GNU/Linux 2.2

How-To-Repeat:
Compile and watch the assert() fail.
Comment 1 Nathan Sidwell 2001-05-27 09:15:03 UTC
State-Changed-From-To: open->suspended
State-Changed-Why: you are incorrect. [12.6.2]/4 specifies that POD types (of
    which scalar types are), are not initialized.
    
    However, we should warn, if the -Wuninitialized flag is
    given, so I've made this a suspended change-request.
Comment 2 Nathan Sidwell 2001-05-27 16:15:03 UTC
From: nathan@gcc.gnu.org
To: gcc-gnats@gcc.gnu.org, nobody@gcc.gnu.org, pere@hungry.com
Cc:  
Subject: Re: c++/2972
Date: 27 May 2001 16:15:03 -0000

 Synopsis: Uninitialized member variables are not default initialized
 
 State-Changed-From-To: open->suspended
 State-Changed-By: nathan
 State-Changed-When: Sun May 27 09:15:03 2001
 State-Changed-Why:
     you are incorrect. [12.6.2]/4 specifies that POD types (of
     which scalar types are), are not initialized.
     
     However, we should warn, if the -Wuninitialized flag is
     given, so I've made this a suspended change-request.
 
 http://gcc.gnu.org/cgi-bin/gnatsweb.pl?cmd=view&pr=2972&database=gcc
Comment 3 Giovanni Bajo 2003-05-05 01:45:55 UTC
State-Changed-From-To: suspended->analyzed
State-Changed-Why: As far as I can see, this PR is not waiting for the 
    resolution of any issue, so its state should be "analyzed".
    
    What Nathan is suggesting is to add a warning for this:
    --------------------------
    struct A
    {
        int f,g;
    
        A()
        {
            f = g;
        }
    };
    --------------------------
    
    -Wuninitialized could detect the use of g before
    initialization within the constructor.
Comment 4 Giovanni Bajo 2003-05-09 19:04:23 UTC
Responsible-Changed-From-To: unassigned->gdr
Responsible-Changed-Why: Diagnostic mantainer
Comment 5 eric-gcc 2004-01-07 03:35:52 UTC
Also, it should detect any scalar member variables that are not assigned to in
any way in the constructor.
Comment 6 Gabriel Dos Reis 2006-01-22 03:43:27 UTC
(In reply to comment #5)
> Also, it should detect any scalar member variables that are not assigned to in
> any way in the constructor.

Agreed.
However -Wunitialized is taken over by the middle-end.  This is
one more case where I agre with Mark Mitchell that the middl-end
should not try those things.  Long term, we should move
such warnings to the front-end.  Yes I know, that is not a 
universally held opinon.
Comment 7 Wolfgang Bangerth 2008-09-10 15:30:27 UTC
Still the same with gcc version 4.4.0 20080801 (experimental) [trunk revision 138448]
Comment 8 Manuel López-Ibáñez 2010-02-24 13:05:15 UTC
Related to PR 19808.

(In reply to comment #6)
> However -Wunitialized is taken over by the middle-end.  This is

That doesn't mean that you cannot produce uninitialized warnings in the front-end for clear-cut cases like this. In fact, it is impossible to detect this case in the middle-end because of the code produced by the C++ front-end,
Comment 9 Jonathan Wakely 2010-06-03 01:13:55 UTC
I've been experimenting with this patch, which warns if there is a missing mem-initializer for a scalar.

It gives a false positive for cases were the member is assigned to in the constructor body, or otherwise initialized before use, but it's a start, and has already helped me find some missing mem-initializers in real code.


--- c.opt.orig  2010-06-02 13:47:02.120129255 +0000
+++ c.opt       2010-06-02 13:07:51.944440072 +0000
@@ -304,6 +304,10 @@
 C ObjC C++ ObjC++ Var(warn_main) Init(-1) Warning
 Warn about suspicious declarations of \"main\"

+Wmeminit
+C++ Var(warn_meminit) Init(-1) Warning
+Warn about POD members which are not initialized in a constructor
initialization list
+
 Wmissing-braces
 C ObjC C++ ObjC++ Var(warn_missing_braces) Warning
 Warn about possibly missing braces around initializers
--- c-opts.c.orig       2010-06-02 13:46:49.774040694 +0000
+++ c-opts.c    2010-06-02 13:11:57.829233564 +0000
@@ -492,6 +492,10 @@
      cpp_opts->warn_invalid_pch = value;
      break;

+    case OPT_Wmeminit:
+      warn_meminit = value;
+      break;
+
    case OPT_Wmissing_include_dirs:
      cpp_opts->warn_missing_include_dirs = value;
      break;
--- cp/init.c.orig      2010-06-02 13:46:31.125713124 +0000
+++ cp/init.c   2010-06-02 13:21:01.473640135 +0000
@@ -424,6 +424,12 @@
  tree decl;
  tree type = TREE_TYPE (member);

+  /* warn if there is no initializer for a POD member */
+  if (warn_meminit && init == NULL_TREE && layout_pod_type_p
(strip_array_types (type)))
+    warning_at (DECL_SOURCE_LOCATION (current_function_decl), OPT_Wmeminit,
+               "%qD is not initialized in the member initialization list",
+               member);
+
  /* Effective C++ rule 12 requires that all data members be
     initialized.  */
  if (warn_ecpp && init == NULL_TREE && TREE_CODE (type) != ARRAY_TYPE)
Comment 10 Manuel López-Ibáñez 2010-06-03 08:47:27 UTC
(In reply to comment #9)
> I've been experimenting with this patch, which warns if there is a missing
> mem-initializer for a scalar.
> 
> It gives a false positive for cases were the member is assigned to in the
> constructor body, or otherwise initialized before use, but it's a start, and
> has already helped me find some missing mem-initializers in real code.

Nice but I am afraid there may be too many false positives. And what is different between this and the -Weffc++ warning given just below it?


> 
> +    case OPT_Wmeminit:
> +      warn_meminit = value;
> +      break;
> +

You do not need this. This is handled automatically when you defined Var in the opt files.

Comment 11 Jonathan Wakely 2010-06-03 09:18:04 UTC
(In reply to comment #10)
> (In reply to comment #9)
> > I've been experimenting with this patch, which warns if there is a missing
> > mem-initializer for a scalar.
> > 
> > It gives a false positive for cases were the member is assigned to in the
> > constructor body, or otherwise initialized before use, but it's a start, and
> > has already helped me find some missing mem-initializers in real code.
> 
> Nice but I am afraid there may be too many false positives.

Yes, I'm not proposing it in that state. It's just an experiment but has been useful for me already.

> And what is
> different between this and the -Weffc++ warning given just below it?

-Weffc++ also enables dozens of other warnings, most of which I don't find useful.  It is about good style, not dangerous code, and I don't like the compiler to enforce style.

In this specific case, the Weffc++ warning complains if *any* member has no mem-initializer. I only care about POD members which will be uninitialized, I don't want to add a mem-initializer for e.g. a std::string member which has a default constructor.

> > +    case OPT_Wmeminit:
> > +      warn_meminit = value;
> > +      break;
> > +
> 
> You do not need this. This is handled automatically when you defined Var in the
> opt files.

Ah OK, thanks!

Comment 12 Jonathan Wakely 2010-06-03 09:27:29 UTC
Apart from the false positives, another problem is that the check for layout_pod_type_p is not right.  An empty class is a POD but doesn't need initialising.
Comment 13 Paolo Carlini 2010-06-03 10:16:24 UTC
About -Weffc++, we also have a PR (16166) about splitting it... Not that I think we should really do that - adding a dozen of -Weffc++-type warnings - but I believe it would be a good idea to finally resolve one way or the other all those long standing -Weffc++ PRs (we have 5 or 6)... If you ask my opinion, we should probably *not* split the option, and instead reduce *a lot* the false positives for the various warnings, even if that means adding false negatives.
Comment 14 Jonathan Wakely 2010-06-03 11:24:41 UTC
Created attachment 20817 [details]
better -Wmeminit patch

This version ignores empty classes and checks for a nontrivial default ctor instead of layout_pod_type.

This patch doesn't enable the warning unless explicity requested. I realise that this warning is about enforcing style ("members should be initialised in the mem-initializer-list not in the ctor body") but that's ok because it's my preferred style, I just don't want the compiler to enforce other people's preferred style ;)
Comment 15 Jonathan Wakely 2010-12-03 19:07:03 UTC
Created attachment 22623 [details]
updated -Wmeminit patch for trunk (4.6.0)
Comment 16 Jonathan Wakely 2011-11-07 21:45:44 UTC
new patch http://gcc.gnu.org/ml/gcc-patches/2011-11/msg01068.html
Comment 17 Paolo Carlini 2012-08-21 17:20:06 UTC
Jon, are you actively working on this? I found this last message:

  http://gcc.gnu.org/ml/gcc-patches/2011-11/msg01477.html

Let me know if I can help...
Comment 18 Jonathan Wakely 2012-08-21 17:26:16 UTC
No, not at present. I tried using default_init_uninitialized_part but it either missed cases or produce ICEs, and I never solved the problems. I can send you my work-in-progress when I get home, it would be great if you could help me with the issues I don't understand.
Comment 19 Paolo Carlini 2012-08-21 17:54:12 UTC
Eh, I'm of course not sure that I can help but I quickly went through the exchange on gcc-patches and got the impression that your work was already in an advanced stage, thus we should try to finish it! I'm pretty sure that we can make it in time for 4.8.0 (maybe with one or two more rounds of focused tips from Jason)
Comment 20 Manuel López-Ibáñez 2014-09-27 20:40:03 UTC
(In reply to Jonathan Wakely from comment #14)
> Created attachment 20817 [details]
> better -Wmeminit patch
> 
> This version ignores empty classes and checks for a nontrivial default ctor
> instead of layout_pod_type.
> 
> This patch doesn't enable the warning unless explicity requested. I realise
> that this warning is about enforcing style ("members should be initialised
> in the mem-initializer-list not in the ctor body") but that's ok because
> it's my preferred style, I just don't want the compiler to enforce other
> people's preferred style ;)

Perhaps a better alternative is to warn only if the uninitialized member is used in a mem-initializer. Then, when building the constructor call, mark the uninitialized members somehow as uninitialized for the middle-end, and let the middle-end handle the cases in the body of the constructor. The first part would already fix PR19808. The second part will fix this bug with fewer false positives than the proposed patch.
Comment 21 Manuel López-Ibáñez 2014-09-29 14:53:56 UTC
(In reply to Manuel López-Ibáñez from comment #20)
> Perhaps a better alternative is to warn only if the uninitialized member is
> used in a mem-initializer. Then, when building the constructor call, mark
> the uninitialized members somehow as uninitialized for the middle-end, and
> let the middle-end handle the cases in the body of the constructor. The
> first part would already fix PR19808. The second part will fix this bug with
> fewer false positives than the proposed patch.

And this comment gives some ideas:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19808#c9

Richard B, you say that: "using <error-mark> [for marking undefined memory] is just a random (and probably bad) idea."  Intuitively this seems more scalable, since one could propagate the undefined value like VRP does.

The other alternative "Introducing a SSA name default definition for A (even though not of register type)", however, seems possible already. no? We do already check for default definitions in virtual operands, tree-ssa-uninit.c says:

	  /* For memory the only cheap thing we can do is see if we
	     have a use of the default def of the virtual operand.

So what is missing here?