Bug 19808

Summary: miss a warning about uninitialized members in constructor
Product: gcc Reporter: adl
Component: c++Assignee: Not yet assigned to anyone <unassigned>
Status: NEW ---    
Severity: enhancement CC: bart.vanassche, dushistov, fang, gcc-bugs, jason, jwakely.gcc, manu, myselfhimself, nishant.031, pentek.imre, pluto
Priority: P2 Keywords: diagnostic
Version: 3.4.4   
Target Milestone: ---   
Host: i686-pc-linux-gnu Target:
Build: Known to work:
Known to fail: Last reconfirmed: 2005-05-09 00:52:15
Bug Depends on:    
Bug Blocks: 2972, 24639, 34307    

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. ***