Bug 19808

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. 

This is related to PR 2972 and PR 18016.

*** Bug 26072 has been marked as a duplicate of this bug. ***

*** Bug 34307 has been marked as a duplicate of this bug. ***

*** Bug 36866 has been marked as a duplicate of this bug. ***

*** Bug 43163 has been marked as a duplicate of this bug. ***

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.

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.

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

(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 ?

(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?

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

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

(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).

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

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

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

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

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

*** Bug 57917 has been marked as a duplicate of this bug. ***

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?

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

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

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)?

Never mind the second question, I found walk_tree.

*** Bug 65301 has been marked as a duplicate of this bug. ***

Created attachment 36706 [details]
First version of patch for PR19808

This is the current version of my patch.
It still needs some work.
The Wuninitialized part should be combined with the Winit_self part,
and it should be made to detect multiple uninitialized values in a single initializer.

Hi Anthony, adding testcases to the patch will help clarify what is working and what you expect to work but it isn't: https://gcc.gnu.org/wiki/HowToPrepareATestcase

Created attachment 36851 [details]
Updated patch for PR19808.

I updated my patch to include a testcase, and I also fixed the case for anonymous unions.
Things not working right (as far as i know) in this patch yet are:
* references
* pointers
* multiple uninitialized values being used to initialize a single field. (I'm also not sure how to test for this in the testsuite)

*** Bug 70647 has been marked as a duplicate of this bug. ***

This is really a dup of PR2972.

*** This bug has been marked as a duplicate of bug 2972 ***

(In reply to Richard Biener from comment #31)
> This is really a dup of PR2972.
> 
> *** This bug has been marked as a duplicate of bug 2972 ***

No, it is  not. The difference is explained by Jason in comment #22 and the existing bug by me in comment #23.

There is not only a conceptual difference but a difference in difficulty. This bug can be solved entirely by the C++ FE (and this is what the patch in comment #29 does) while PR2972 needs middle-end support.

(In reply to Manuel López-Ibáñez from comment #32)
> (In reply to Richard Biener from comment #31)
> > This is really a dup of PR2972.
> > 
> > *** This bug has been marked as a duplicate of bug 2972 ***
> 
> No, it is  not. The difference is explained by Jason in comment #22 and the
> existing bug by me in comment #23.
> 
> There is not only a conceptual difference but a difference in difficulty.
> This bug can be solved entirely by the C++ FE (and this is what the patch in
> comment #29 does) while PR2972 needs middle-end support.

I have a fix for PR2972 and it also correctly handles this case so why is it
"conceptually different"?

(In reply to Richard Biener from comment #33)
> I have a fix for PR2972 and it also correctly handles this case so why is it
> "conceptually different"?

Because to detect uninitialized member usage in member initializer list one only needs to traverse the member initializer list and keep track of what has been initialized so far, so there is no need for ME support and there is no room for false positives/negatives and location info can be perfect.

My understanding is that the consensus has always been to warn in the FE and avoid ME warnings as much as possible because they tend to be less reliable. But if you have a patch that works as well as the FE warning, then that is excellent and I'm happy to see both bugs fixed in one stroke.

*** Bug 82552 has been marked as a duplicate of this bug. ***

*** Bug 85691 has been marked as a duplicate of this bug. ***

N.B. we should also warn using uninitialized members in default member initializers, e.g. both of these should produce a warning:

struct X {
  int x1 = x2;
  int x2 = 0;
};

X x{};  // causes definition of default constructor

struct Y {
  int y1 = y2;
  int y2;
  Y() : y2(0) { }
};

Clang warns about both:

defmeminit.cc:2:12: warning: field 'x2' is uninitialized when used here [-Wuninitialized]
  int x1 = x2;
           ^
defmeminit.cc:6:3: note: in implicit default constructor for 'X' first required here
X x{};
  ^
defmeminit.cc:1:8: note: during field initialization in the implicit default constructor
struct X {
       ^
defmeminit.cc:9:12: warning: field 'y2' is uninitialized when used here [-Wuninitialized]
  int y1 = y2;
           ^
defmeminit.cc:11:3: note: during field initialization in this constructor
  Y() : y2(0) { }
  ^
2 warnings generated.

(In reply to Jonathan Wakely from comment #37)

If you add a

Y y{};

GCC warns about the Y constructor.

We don't warn about the implicit X constructor because we don't clobber the object at the beginning of an implicit constructor, because value-initialization zero-initializes the object before calling the implicit constructor, and we mustn't clobber that initialization (bug 68006).  The middle end relies on the clobber to know what's uninitialized, so we don't get the warning here.

It would be appropriate to give a maybe-uninitialized warning here, though.  I don't know how complicated it would be to do that using the existing mechanisms.

(In reply to Jason Merrill from comment #38)
> (In reply to Jonathan Wakely from comment #37)
> 
> If you add a
> 
> Y y{};
> 
> GCC warns about the Y constructor.
> 
> We don't warn about the implicit X constructor because we don't clobber the
> object at the beginning of an implicit constructor, because
> value-initialization zero-initializes the object before calling the implicit
> constructor, and we mustn't clobber that initialization (bug 68006).  The
> middle end relies on the clobber to know what's uninitialized, so we don't
> get the warning here.
> 
> It would be appropriate to give a maybe-uninitialized warning here, though. 
> I don't know how complicated it would be to do that using the existing
> mechanisms.

These case are difficult because they involve exported globals which GCC
thinks are always initialized.  For the testcase only the initializer
of x prevails, the constructor of y is discarded before we run the
warning machinery.  The initializer of x prevails in __static_initialization_and_destruction_0 like

  _1 = x.x2;
  x.x1 = _1;

which has the aforementioned issue.  So for a proper testcase we need
calls to the constructors (where we should warn in?) and the constructors
prevail.

Adding

Y y{};

makes Y::Y prevail and as you said we warn about it.  IL:

Y::Y (struct Y * const this)
{
  int _1;

  <bb 2> :
  MEM[(struct  &)this_3(D)] ={v} {CLOBBER};
  _1 = this_3(D)->y2;
  this_3(D)->y1 = _1;
  this_3(D)->y2 = 0;
  return;


so - how do I make X::X used and thus prevail?  It looks like it doesn't
really exist and the C++ FE even for

void foo() { X x{}; }

just outputs

;; Function void foo() (null)
;; enabled by -tree-original


{
  struct X x = {.x2=0};

  <<cleanup_point   struct X x = {.x2=0};>>;
  <<cleanup_point <<< Unknown tree: expr_stmt
  x.x1 = x.x2 >>>>>;
}

which ends up as

foo ()
{
  struct X x;

  try
    {
      x = {};
      _1 = x.x2;
      x.x1 = _1;
    }
  finally
    {
      x = {CLOBBER};
    }
}

so there is no uninitialized use.

OK, doing void foo() { X x; } shows

X::X (struct X * const this)
{
  _1 = this->x2;
  this->x1 = _1;
  this->x2 = 0;
}

foo ()
{
  struct X x;

  try
    {
      X::X (&x);
    }
  finally
    {
      x = {CLOBBER};
    }
}

warning would need inlining of the constructor which only happens after
the early warning pass, the late one isn't run at -O0 and with optimization
everything of course vanishes.

(In reply to Richard Biener from comment #39)
> so - how do I make X::X used and thus prevail?  It looks like it doesn't
> really exist

True, for C++14 and up, "X x{};" does aggregate initialization rather than calling the constructor.  We ought to warn about this at function scope, but we clear the object first, so it isn't actually uninitialized.

As you found, removing the {} makes it use the constructor.

> OK, doing void foo() { X x; } shows
> 
> X::X (struct X * const this)
> {
>   _1 = this->x2;
>   this->x1 = _1;
>   this->x2 = 0;
> }
> 
> foo ()
> {
>   struct X x;
> 
>   try
>     {
>       X::X (&x);
>     }
>   finally
>     {
>       x = {CLOBBER};
>     }
> }
> 
> warning would need inlining of the constructor which only happens after
> the early warning pass, the late one isn't run at -O0 and with optimization
> everything of course vanishes.

I was wondering about a maybe-uninitialized warning for the constructor without considering where it's called from; even if a particular object is zero-initialized when we enter the implicit constructor, the constructor shouldn't rely on that.  Basically, warn as if there were a clobber, without there actually being one.

All these cases can be handled perfectly by the FE and there's a patch
above.

Why complicate it by expecting the ME to understand C++ mem-initializer
semantics?

I posted the latest duplicate of this bug, and I don't know anything about how gcc works. I am very grateful for the hard work you guys put into this, but just looking at the data. This bug was first reported 13 years ago - and I think it's a pretty important one as one can easily make a mistake with initialization.

(In reply to Roger Weber from comment #42)
> I posted the latest duplicate of this bug, and I don't know anything about
> how gcc works. I am very grateful for the hard work you guys put into this,
> but just looking at the data. This bug was first reported 13 years ago - and
> I think it's a pretty important one as one can easily make a mistake with
> initialization.

GCC is very large and growing and the number of GCC developers is very very small and not growing. There are thousands of open bugs, thus developers need to prioritize. If something was truly pretty important, someone would have stepped up (or paid someone) to fix it. There is a 60 lines patch that fixes most of the testcases here and could serve as inspiration to solve the rest. The person who produced the patch left the work unfinished. Someone needs to finish it: https://gcc.gnu.org/wiki/GettingStarted#Basics:_Contributing_to_GCC_in_10_easy_steps

It is a simple as that.

(In reply to Jason Merrill from comment #40)
> (In reply to Richard Biener from comment #39)
> > so - how do I make X::X used and thus prevail?  It looks like it doesn't
> > really exist
> 
> True, for C++14 and up, "X x{};" does aggregate initialization rather than
> calling the constructor.  We ought to warn about this at function scope, but
> we clear the object first, so it isn't actually uninitialized.
> 
> As you found, removing the {} makes it use the constructor.
> 
> > OK, doing void foo() { X x; } shows
> > 
> > X::X (struct X * const this)
> > {
> >   _1 = this->x2;
> >   this->x1 = _1;
> >   this->x2 = 0;
> > }
> > 
> > foo ()
> > {
> >   struct X x;
> > 
> >   try
> >     {
> >       X::X (&x);
> >     }
> >   finally
> >     {
> >       x = {CLOBBER};
> >     }
> > }
> > 
> > warning would need inlining of the constructor which only happens after
> > the early warning pass, the late one isn't run at -O0 and with optimization
> > everything of course vanishes.
> 
> I was wondering about a maybe-uninitialized warning for the constructor
> without considering where it's called from; even if a particular object is
> zero-initialized when we enter the implicit constructor, the constructor
> shouldn't rely on that.  Basically, warn as if there were a clobber, without
> there actually being one.

Interesting suggestion but that's IMHO a bit too much information from
the outside to pack into the middle-end code.  That we're dealing with
accesses to *this and that we are inside a constructor.  You'd need to
add that here, tree-ssa-uninit.c:warn_uninitialized_vars

              /* Do not warn if it can be initialized outside this function.
                 If we did not reach function entry then we found killing
                 clobbers on all paths to entry.  */
              if (fentry_reached
                  /* ???  We'd like to use ref_may_alias_global_p but that
                     excludes global readonly memory and thus we get bougs
                     warnings from p = cond ? "a" : "b" for example.  */
                  && (!VAR_P (base)
                      || is_global_var (base)))
                continue;

given we have to constrain this to must-aliases of *this the easiest
check would be sth like

   && (!eligible-constructor (cfun)
       || TREE_CODE (base) != MEM_REF
       || TREE_OPERAND (base, 0) != default-def-of-this-parm))

and then elide the warning to maybe-uninit.  I guess we now have a flag
whether a function is a constructor and we also can get at the this
parm-decl so in theory "enhancing" the warning this way would be possible.

*** Bug 68301 has been marked as a duplicate of this bug. ***

*** Bug 89192 has been marked as a duplicate of this bug. ***

*** Bug 97525 has been marked as a duplicate of this bug. ***

I have a patch that for this:

// PR c++/19808
// { dg-do compile { target c++11 } }
// { dg-options "-Wuninitialized" }

struct S {
  int i, j, k, l;
  S() : i(j), // { dg-warning "field .S::j. is uninitialized when used here" }
	j(1),
	k(l + 1), // { dg-warning "field .S::l. is uninitialized when used here" }
	l(2) { }
};

struct A {
  int a, b, c;
  A() : a(b // { dg-warning "field .A::b. is uninitialized when used here" }
	  + c) { } // { dg-warning "field .A::c. is uninitialized when used here" }
};

struct B {
  int &r;
  int *p;
  int a;
  B() : r(a), p(&a), a(1) { }
};

struct C {
  const int &r1, &r2;
  C () : r1(r2), // { dg-warning "reference .C::r2. is not yet bound to a value when used here" }
	 r2(r1) { }
};

struct D {
  int a = 1;
  int b = 2;
  D() : a(b + 1), b(a + 1) { } // { dg-warning "field .D::b. is uninitialized when used here" }
};

struct E {
  int a = 1;
  E() : a(a + 1) { } // { dg-warning "field .E::a. is uninitialized when used here" }
};

struct F {
  int a = 1;
  int b;
  F() : b(a + 1) { }
};

struct bar {
  bar() {}
  bar(bar&) {}
};

class foo {
  bar first;
  bar second;
public:
  foo() : first(second) {} // { dg-warning "field .foo::second. is uninitialized when used here" }
};

does this:

$ ./cc1plus -quiet  -Wuninitialized  Wuninitialized-12.C
Wuninitialized-12.C: In constructor ‘S::S()’:
Wuninitialized-12.C:7:11: warning: field ‘S::j’ is uninitialized when used here [-Wuninitialized]
    7 |   S() : i(j), // { dg-warning "field .S::j. is uninitialized when used here" }
      |           ^
Wuninitialized-12.C:9:11: warning: field ‘S::l’ is uninitialized when used here [-Wuninitialized]
    9 |         k(l + 1), // { dg-warning "field .S::l. is uninitialized when used here" }
      |           ^
Wuninitialized-12.C: In constructor ‘A::A()’:
Wuninitialized-12.C:15:11: warning: field ‘A::b’ is uninitialized when used here [-Wuninitialized]
   15 |   A() : a(b // { dg-warning "field .A::b. is uninitialized when used here" }
      |           ^
Wuninitialized-12.C:16:13: warning: field ‘A::c’ is uninitialized when used here [-Wuninitialized]
   16 |           + c) { } // { dg-warning "field .A::c. is uninitialized when used here" }
      |             ^
Wuninitialized-12.C: In constructor ‘C::C()’:
Wuninitialized-12.C:28:13: warning: reference ‘C::r2’ is not yet bound to a value when used here [-Wuninitialized]
   28 |   C () : r1(r2), // { dg-warning "reference .C::r2. is not yet bound to a value when used here" }
      |             ^~
Wuninitialized-12.C: In constructor ‘D::D()’:
Wuninitialized-12.C:35:11: warning: field ‘D::b’ is uninitialized when used here [-Wuninitialized]
   35 |   D() : a(b + 1), b(a + 1) { } // { dg-warning "field .D::b. is uninitialized when used here" }
      |           ^
Wuninitialized-12.C: In constructor ‘E::E()’:
Wuninitialized-12.C:40:11: warning: field ‘E::a’ is uninitialized when used here [-Wuninitialized]
   40 |   E() : a(a + 1) { } // { dg-warning "field .E::a. is uninitialized when used here" }
      |           ^
Wuninitialized-12.C: In constructor ‘foo::foo()’:
Wuninitialized-12.C:58:17: warning: field ‘foo::second’ is uninitialized when used here [-Wuninitialized]
   58 |   foo() : first(second) {} // { dg-warning "field .foo::second. is uninitialized when used here" }
      |                 ^~~~~~

Patch resurrected for GCC 12:
https://gcc.gnu.org/pipermail/gcc-patches/2021-November/583544.html

The trunk branch has been updated by Marek Polacek <mpolacek@gcc.gnu.org>:

https://gcc.gnu.org/g:0790c8aacdfb4fd096aa580dae0fe49172c43ab2

commit r12-5391-g0790c8aacdfb4fd096aa580dae0fe49172c43ab2
Author: Marek Polacek <polacek@redhat.com>
Date:   Tue Nov 10 20:07:24 2020 -0500

    c++: Implement -Wuninitialized for mem-initializers (redux) [PR19808]
    
    2021 update: Last year I posted a version of this patch:
    <https://gcc.gnu.org/pipermail/gcc-patches/2020-November/559162.html>
    but it didn't make it in.  The main objection seemed to be that the
    patch tried to do too much, and overlapped with the ME uninitialized
    warnings.  Since the patch used walk_tree without any data flow info,
    it issued false positives for things like a(0 ? b : 42) and similar.
    
    I'll admit I've been dreading resurrecting this because of the lack
    of clarity about where we should warn about what.  On the other hand,
    I think we really should do something about this.  So I've simplified
    the original patch as much as it seemed reasonable.  For instance, it
    doesn't even attempt to handle cases like "a((b = 42)), c(b)" -- for
    these I simply give up for the whole mem-initializer (but who writes
    code like that, anyway?).  I also give up when a member is initialized
    with a function call, because we don't know what the call could do.
    See Wuninitialized-17.C, for which clang emits a false positive but
    we don't.  I remember having a hard time dealing with initializer lists
    in my previous patch, so now I only handle simple a{b} cases, but no
    more.  It turned out that this abridged version still warns about 90%
    cases where users would expect a warning.
    
    More complicated cases are left for the ME, which, for unused inline
    functions, will only warn with -fkeep-inline-functions, but so be it.
    (This is bug 21678.)
    
    This patch implements the long-desired -Wuninitialized warning for
    member initializer lists, so that the front end can detect bugs like
    
      struct A {
        int a;
        int b;
        A() : b(1), a(b) { }
      };
    
    where the field 'b' is used uninitialized because the order of member
    initializers in the member initializer list is irrelevant; what matters
    is the order of declarations in the class definition.
    
    I've implemented this by keeping a hash set holding fields that are not
    initialized yet, so at first it will be {a, b}, and after initializing
    'a' it will be {b} and so on.  Then I use walk_tree to walk the
    initializer and if we see that an uninitialized object is used, we warn.
    Of course, when we use the address of the object, we may not warn:
    
      struct B {
        int &r;
        int *p;
        int a;
        B() : r(a), p(&a), a(1) { } // ok
      };
    
    Likewise, don't warn in unevaluated contexts such as sizeof.  Classes
    without an explicit initializer may still be initialized by their
    default constructors; whether or not something is considered initialized
    is handled in perform_member_init, see member_initialized_p.
    
            PR c++/19808
            PR c++/96121
    
    gcc/cp/ChangeLog:
    
            * init.c (perform_member_init): Remove a forward declaration.
            Walk the initializer using find_uninit_fields_r.  New parameter
            to track uninitialized fields.  If a member is initialized,
            remove it from the hash set.
            (perform_target_ctor): Return the initializer.
            (struct find_uninit_data): New class.
            (find_uninit_fields_r): New function.
            (find_uninit_fields): New function.
            (emit_mem_initializers): Keep and initialize a set holding fields
            that are not initialized.  When handling delegating constructors,
            walk the constructor tree using find_uninit_fields_r.  Also when
            initializing base clases.  Pass uninitialized down to
            perform_member_init.
    
    gcc/ChangeLog:
    
            * doc/invoke.texi: Update documentation for -Wuninitialized.
            * tree.c (stabilize_reference): Set location.
    
    gcc/testsuite/ChangeLog:
    
            * g++.dg/warn/Wuninitialized-14.C: New test.
            * g++.dg/warn/Wuninitialized-15.C: New test.
            * g++.dg/warn/Wuninitialized-16.C: New test.
            * g++.dg/warn/Wuninitialized-17.C: New test.
            * g++.dg/warn/Wuninitialized-18.C: New test.
            * g++.dg/warn/Wuninitialized-19.C: New test.
            * g++.dg/warn/Wuninitialized-20.C: New test.
            * g++.dg/warn/Wuninitialized-21.C: New test.
            * g++.dg/warn/Wuninitialized-22.C: New test.
            * g++.dg/warn/Wuninitialized-23.C: New test.
            * g++.dg/warn/Wuninitialized-24.C: New test.
            * g++.dg/warn/Wuninitialized-25.C: New test.
            * g++.dg/warn/Wuninitialized-26.C: New test.
            * g++.dg/warn/Wuninitialized-27.C: New test.
            * g++.dg/warn/Wuninitialized-28.C: New test.
            * g++.dg/warn/Wuninitialized-29.C: New test.
            * g++.dg/warn/Wuninitialized-30.C: New test.

At last, implemented.

(In reply to Marek Polacek from comment #51)
> At last, implemented.

Marvellous. 

I will test it by compiling Fedora rawhide and report back
with any errors.

Nearly 17 years is quite a wait for a fix.

I am not sure if this belongs here or in a separate bug report,
but given this code:

class AllocatorWithCleanup {
public:
  int *allocate(int, void *);
};
class SecBlock {
  SecBlock() : m_ptr(m_alloc.allocate(0, nullptr)) {}
  AllocatorWithCleanup m_alloc;
  int *m_ptr;
};

Recent clang-14 thinks it is ok and new gcc trunk thinks it isn't.

$ /home/dcb/llvm/results/bin/clang++ -c -O2 -Wall bug774.cc
$ /home/dcb/llvm/results/bin/clang++ -v
clang version 14.0.0 (https://github.com/llvm/llvm-project.git f95bd18b5faa6a5af4b5786312c373c5b2dce687)
$ /home/dcb/gcc/results/bin/gcc -c -O2 -Wall bug774.cc
bug774.cc: In constructor ‘SecBlock::SecBlock()’:
bug774.cc:6:22: warning: member ‘SecBlock::m_alloc’ is used uninitialized [-Wuninitialized]
    6 |   SecBlock() : m_ptr(m_alloc.allocate(0, nullptr)) {}
      |                      ^~~~~~~
$ /home/dcb/gcc/results/bin/gcc -v
gcc version 12.0.0 20211119 (experimental) (0e510ab53414430e) 
$

Looks like it's missing a check for m_alloc having vacuous initialization, i.e. not actually needing any initialization before it's usable.

Aah, I should check is_empty_class before issuing the warning I guess.

The trunk branch has been updated by Marek Polacek <mpolacek@gcc.gnu.org>:

https://gcc.gnu.org/g:4b1d3d8d732bea86c7b2aba46c2a437461020824

commit r12-5479-g4b1d3d8d732bea86c7b2aba46c2a437461020824
Author: Marek Polacek <polacek@redhat.com>
Date:   Fri Nov 19 14:22:10 2021 -0500

    c++: -Wuninitialized for mem-inits and empty classes [PR19808]
    
    This fixes a bogus -Wuninitialized warning: there's nothing to initialize
    in empty classes, so don't add them into our uninitialized set.
    
            PR c++/19808
    
    gcc/cp/ChangeLog:
    
            * init.c (emit_mem_initializers): Don't add is_really_empty_class
            members into uninitialized.
    
    gcc/testsuite/ChangeLog:
    
            * g++.dg/warn/Wuninitialized-28.C: Make a class nonempty.
            * g++.dg/warn/Wuninitialized-29.C: Likewise.
            * g++.dg/warn/Wuninitialized-31.C: New test.