Bug 19808 - miss a warning about uninitialized member usage in member initializer list in constructor
Summary: miss a warning about uninitialized member usage in member initializer list in...
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 3.4.4
: P2 enhancement
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic, easyhack
: 26072 34307 36866 43163 65301 (view as bug list)
Depends on:
Blocks: 2972 Wuninitialized 34307
  Show dependency treegraph
 
Reported: 2005-02-07 17:40 UTC by adl
Modified: 2015-03-27 15:31 UTC (History)
16 users (show)

See Also:
Host: i686-pc-linux-gnu
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2014-09-27 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description adl 2005-02-07 17:40:55 UTC
I just ran into bogus code similar to the following.  
(The code is bogus, not GCC, but I believe GCC could have helped.)

// main.cc
#include <iostream>

struct A
{
  A(int x) : x(x) {};
  int call_me() { return x; }
  int x;
};

struct B
{
  B(A* a) : i(the_a->call_me()), the_a(a) {}
  int i;
  A* the_a;
};

int
main()
{
  A a(20);
  B b(&a);
  std::cout << b.i << std::endl;
}

% g++ -O2 -Wall main.cc
% ./a.out
1328900

This displays 1328900 (or any other value) instead of 20 because
the_a->call_me() is invoked before the_a is initialized in 
B's constructor.

It would be useful if g++ could diagnose such use of uninitialized
members in constructors, just like it does for local variables.
Comment 1 Wolfgang Bangerth 2005-02-07 21:53:37 UTC
Confirmed. We should get a warning for this code, but don't: 
---------------------- 
struct S 
{ 
    int i, j; 
    S() : i(j), j(1) {} 
}; 
 
S s; 
----------------------- 
 
W. 
Comment 2 Andrew Pinski 2005-02-07 21:56:14 UTC
This is related to PR 2972 and PR 18016.
Comment 3 Andrew Pinski 2006-02-02 12:58:41 UTC
*** Bug 26072 has been marked as a duplicate of this bug. ***
Comment 4 Jonathan-david Schroder 2007-12-04 21:03:41 UTC
*** Bug 34307 has been marked as a duplicate of this bug. ***
Comment 5 Manuel López-Ibáñez 2008-07-21 14:30:57 UTC
*** Bug 36866 has been marked as a duplicate of this bug. ***
Comment 6 Manuel López-Ibáñez 2010-02-24 12:46:36 UTC
*** Bug 43163 has been marked as a duplicate of this bug. ***
Comment 7 Manuel López-Ibáñez 2010-02-24 12:59:35 UTC
This usage could be warned in the front-end. Specially because the SSA form looks like:

S::S() (struct SD.2093 * const thisD.2102)
{
  intD.2 D.2131;

  # BLOCK 2, starting at line 6
  # PRED: ENTRY (fallthru)
  [pr19808.C : 6:20] D.2131_2 = thisD.2102_1(D)->jD.2096;
  [pr19808.C : 6:20] thisD.2102_1(D)->iD.2095 = D.2131_2;
  [pr19808.C : 6:20] thisD.2102_1(D)->jD.2096 = 1;
  [pr19808.C : 6:23] return;
  # SUCC: EXIT
}

Since *this is a pointer parameter (why?), the middle-end cannot know that j is uninitialized.
Comment 8 Jason Merrill 2010-02-24 16:43:58 UTC
What else could it be than a pointer?  It might be possible to mark it somehow so that the middle end knows to consider the referent uninitialized.

Incidentally, perhaps we should mark the this parameter as __restrict...

It does seem reasonable for the front end to detect uses of a member in a previous mem-initializer, though that wouldn't handle uses in the body.
Comment 9 Richard Biener 2010-02-24 18:04:00 UTC
(In reply to comment #8)
> What else could it be than a pointer?  It might be possible to mark it somehow
> so that the middle end knows to consider the referent uninitialized.
> 
> Incidentally, perhaps we should mark the this parameter as __restrict...
> 
> It does seem reasonable for the front end to detect uses of a member in a
> previous mem-initializer, though that wouldn't handle uses in the body.

I think there is no reasonable way to get reliable uninitialized variable
warnings for incoming pointed-to memory.  The only way we can warn here
is by inlining the constructor into the caller to see either a static
variable or the memory allocation.

But this all boils down to the middle-end not doing uninitialized variable
warnings for memory at all.

I'd close this one as WONTFIX or mark it as dup of a "do uninitialized
variable warnings for memory".

Note that for unrelated reasons we had the idea of introducing a way to
tell the middle-end that previous content of memory becomes undefined,
sort-of adding an assignment from an undefined value like

  A = <error-mark>;

for the reason to preserve aggregate lifetime information after BIND
expression lowering.  The above might also be used carefully in
constructors to mark pieces that the constructor is supposed to initialize
as having undefined content.  Well - if the language standard allows
this.

Of course using <error-mark> is just a random (and probably bad) idea.
Introducing a SSA name default definition for A (even though not of
register type) might be another trick.

The frontend could also use the above at the end of destructors to help
dead code elimination.
Comment 10 Bart Van Assche 2010-02-24 19:10:40 UTC
(In reply to comment #9)
> I think there is no reasonable way to get reliable uninitialized variable
> warnings for incoming pointed-to memory.  The only way we can warn here
> is by inlining the constructor into the caller to see either a static
> variable or the memory allocation.

Why would inlining the constructor be necessary ? Does this mean that it is not possible to mark member variables as uninitialized in gcc ?
Comment 11 Manuel López-Ibáñez 2010-02-24 19:44:44 UTC
(In reply to comment #8)
> What else could it be than a pointer?  It might be possible to mark it somehow
> so that the middle end knows to consider the referent uninitialized.

This is because (this) must be allocated outside the constructor, isn't it? 

Is there nothing pointed by this that could be initialized before calling the constructor?

> Incidentally, perhaps we should mark the this parameter as __restrict...

Yes, with the new __restrict support, this may bring some benefit. Should I open a PR about this?
Comment 12 Jason Merrill 2010-02-24 20:12:56 UTC
(In reply to comment #11)
> (In reply to comment #8)
> > What else could it be than a pointer?  It might be possible to mark it somehow
> > so that the middle end knows to consider the referent uninitialized.
> 
> This is because (this) must be allocated outside the constructor, isn't it? 

Yes.  The storage for the object is allocated separately, either on the stack or on the heap, and then the constructor is called to initialize it.

> Is there nothing pointed by this that could be initialized before calling the
> constructor?

Nothing.

> > Incidentally, perhaps we should mark the this parameter as __restrict...
> 
> Yes, with the new __restrict support, this may bring some benefit. Should I
> open a PR about this?

Sure.  Only in constructors, of course.
Comment 13 Manuel López-Ibáñez 2010-02-24 20:14:49 UTC
(In reply to comment #9)
> 
> I'd close this one as WONTFIX or mark it as dup of a "do uninitialized
> variable warnings for memory".
> 

Please no, this perhaps could still be implemented in the C++ front-end for this particular case. Maybe there is a way to keep track of which initializers have been seen already and which members are not initialized elsewhere.
Comment 14 Richard Biener 2010-02-24 20:22:22 UTC
(In reply to comment #11)
> (In reply to comment #8)
> > What else could it be than a pointer?  It might be possible to mark it somehow
> > so that the middle end knows to consider the referent uninitialized.
> 
> This is because (this) must be allocated outside the constructor, isn't it? 
> 
> Is there nothing pointed by this that could be initialized before calling the
> constructor?
> 
> > Incidentally, perhaps we should mark the this parameter as __restrict...
> 
> Yes, with the new __restrict support, this may bring some benefit. Should I
> open a PR about this?

I don't see how this would be correct (or useful).
Comment 15 Manuel López-Ibáñez 2010-02-24 20:30:02 UTC
(In reply to comment #12)
> Is there nothing pointed by this that could be initialized before calling the
> > constructor?
> 
> Nothing.

Then for sure someone can write a generic/gimple pass that detects this case before lowering. Maybe it won't handle something more convoluted in the body of the constructor, but at least mem-initializers will be warned. One only needs to keep track which members have been initialized already, and if we use one before it has been initialized, then warn. No dataflow needed. 

Alternatively, the C++ front-end could create an uninitialized variable for each member variable. Initialize those, then, at the very end of the constructor, assigned each clone variable to the appropriate member. This will also enabled uninitialized warnings with the current infrastructure and it will work in the body of the constructor.
Comment 16 Jason Merrill 2010-02-24 20:44:13 UTC
(In reply to comment #14)
> > (In reply to comment #8)
> > > Incidentally, perhaps we should mark the this parameter as __restrict...
> 
> I don't see how this would be correct (or useful).

Hmm, I suppose it is possible to have another identical pointer in the case of placement new.
Comment 17 Jason Merrill 2010-02-24 20:45:18 UTC
(In reply to comment #15)
> Alternatively, the C++ front-end could create an uninitialized variable for
> each member variable. Initialize those, then, at the very end of the
> constructor, assigned each clone variable to the appropriate member.

That would break if the constructor calls any other member functions.
Comment 18 Bart Van Assche 2010-02-25 07:00:50 UTC
(In reply to comment #16)
> (In reply to comment #14)
> > > (In reply to comment #8)
> > > > Incidentally, perhaps we should mark the this parameter as __restrict...
> > 
> > I don't see how this would be correct (or useful).
> 
> Hmm, I suppose it is possible to have another identical pointer in the case of
> placement new.

Invoking a constructor twice successively via placement new is questionable programming style. One should use plain functions instead of abusing constructors.
Comment 19 Bart Van Assche 2010-02-25 07:05:46 UTC
(In reply to comment #17)
> (In reply to comment #15)
> > Alternatively, the C++ front-end could create an uninitialized variable for
> > each member variable. Initialize those, then, at the very end of the
> > constructor, assigned each clone variable to the appropriate member.
> 
> That would break if the constructor calls any other member functions.

It is even possible to trigger member function calls from inside the initializer list. This will result in member functions being invoked for a partially initialized object. Some compilers emit a warning when the 'this' pointer is passed as a function argument from inside the initializer list.
Comment 20 Jonathan Wakely 2013-07-17 09:24:07 UTC
*** Bug 57917 has been marked as a duplicate of this bug. ***
Comment 21 Manuel López-Ibáñez 2014-09-27 20:29:45 UTC
I just got hit by this bug. This can obviously be warned in the FE as clang does:

test.cc:4:11: warning: field 'j' is uninitialized when used here [-Wuninitialized]
  S() : i(j), j(1) {} 
          ^

simply by marking the members somehow as initialized or not, and when using them realizing that they are still uninitialized.

Marek, Paolo, Jason? Any idea how to do this?
Comment 22 Jason Merrill 2014-09-29 13:06:06 UTC
(In reply to Manuel López-Ibáñez from comment #21)
> I just got hit by this bug. This can obviously be warned in the FE as clang
> does:
> 
> test.cc:4:11: warning: field 'j' is uninitialized when used here
> [-Wuninitialized]
>   S() : i(j), j(1) {} 
>           ^
> 
> simply by marking the members somehow as initialized or not, and when using
> them realizing that they are still uninitialized.
> 
> Marek, Paolo, Jason? Any idea how to do this?

It could be done specifically for uses in mem-initializers by walking the initializer in perform_mem_init to look for any references to members that haven't been marked yet.

A more general warning that would cover, say,

X x;
x.x = x.y;

would need support in the existing back end -Wuninitialized code.
Comment 23 Manuel López-Ibáñez 2014-09-29 14:35:15 UTC
(In reply to Jason Merrill from comment #22)
> It could be done specifically for uses in mem-initializers by walking the
> initializer in perform_mem_init to look for any references to members that
> haven't been marked yet.

Great! If I find some time in the following weeks, I will give it a try.

> A more general warning that would cover, say,
> 
> X x;
> x.x = x.y;
> 
> would need support in the existing back end -Wuninitialized code.

That is PR2972 and I think fixing this one is a prerequisite for fixing that one. So let's go step by step.
Comment 24 Anthony Brandon 2014-11-15 23:11:36 UTC
Hi,

> It could be done specifically for uses in mem-initializers by walking the
> initializer in perform_mem_init to look for any references to members that
> haven't been marked yet.

I've been working on this using this approach.
At the moment I can detect and give a warning for something like:

struct S 
{ 
    int i, j; 
    S() : i(j), j(1) {} 
}; 

However it doesn't cover cases like i(j+1), and in the case of i(i) there would currently be two warnings (due to -Winit-self).
Lastly, the warning is given like so:
a.cpp:8:2: warning: ‘S::i’ is initialized with uninitialized field ‘S::j’ [-Wuninitialized]
  S() : i(j), j(b) {}
  ^

So I have a couple of questions:
* How to get the right location for the mark. In other words:
  S() : i(j), j(b) {}
          ^
* Is there a function to traverse a tree structure looking for a particular tree (or some other way to get things like i(j+1) to work)?

* Should this give a warning in the case of i(i)?
Comment 25 Anthony Brandon 2014-11-16 19:14:48 UTC
Never mind the second question, I found walk_tree.
Comment 26 Manuel López-Ibáñez 2015-03-03 17:45:27 UTC
*** Bug 65301 has been marked as a duplicate of this bug. ***