[PATCH] c++: Implement -Wuninitialized for mem-initializers (redux) [PR19808]

[Marek Polacek]

On Sun, Nov 07, 2021 at 04:48:46PM -0700, Martin Sebor wrote:
> On 11/5/21 5:22 PM, Marek Polacek via Gcc-patches wrote:
> > 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.
> 
> I (still) believe this is a great improvement :)

Yea, would be nice to finally nail it down this time around.

> Thinking about your last sentence above and experimenting with
> the patch just a little, we do know that function calls cannot
> "initialize" (by assigning a value) const or reference members,
> so those can be assumed to be uninitiazed when used before
> their initialized has been evaluated.  As in:
> 
> int f (int);
> 
> struct A
> {
>   int x;
>   const int y;
>   A ():
>     x (f (y)),   << y used before initialized
>     y (1) { }
> };
> 
> It should also be reasonable for the prurposes of warnings to
> assume that a call to a const member function (or any call where
> an object is passed by a const reference) doesn't change any
> non-mutable members.  The middle end issues -Wmaybe-uninitialized
> when it sees an uninitualized object passed to a const-qualified
> pointer or reference.

Maybe, I don't know.  It gets tricky and I don't think I want to
handle it in my patch, at least not in this initial version.  It
may be best to leave this to the ME.  Dunno.

> At the same time, a call to a non-const member function can assign
> a value to a not-yet-initialized member, so I wonder if its uses
> subsequent to it should be accepted without warning:
> 
> struct A
> {
>   int x, y, z;
>   int f () { y = 1; return 2; }
>   A ():
>     x (f ()),
>     z (y)   << avoid warning here?
>   { }
> };

This is actually a bug in my code, now fixed (made changes in the
DECL_NONSTATIC_MEMBER_FUNCTION_P block).
 
> > +      /* We're initializing a reference member with itself.  */
> > +      if (TYPE_REF_P (type) && cp_tree_equal (d->member, init))
> > +	warning_at (EXPR_LOCATION (init), OPT_Winit_self,
> > +		    "%qD is initialized with itself", field);
> > +      else if (cp_tree_equal (TREE_OPERAND (init, 0), current_class_ref)
> > +	       && uninitialized->contains (field))
> > +	{
> > +	  if (TYPE_REF_P (TREE_TYPE (field)))
> > +	    warning_at (EXPR_LOCATION (init), OPT_Wuninitialized,
> > +			"reference %qD is not yet bound to a value when used "
> > +			"here", field);
> > +	  else if (!INDIRECT_TYPE_P (type) || is_this_parameter (d->member))
> > +	    warning_at (EXPR_LOCATION (init), OPT_Wuninitialized,
> > +			"field %qD is used uninitialized", field);
> 
> Might I suggest the word "member" instead?
> 
> This is my OCD rearing its head but I always cringe when I read
> the word field in reference to a subobject.  Field is a term
> common in database programming but in C and C++, either member
> or subobject are more wide-spread.   Except for bit-fields, C
> formally uses the term field is to refer to the are into which
> printf directives format their output.  With one exception, C++
> 98 used it the same way.  I see that in recent versions another
> reference has crept in with the memory model, but I think it
> would be fair to call these two anomalies.

Hahah!  No worries, changed to "member".  "Field" is probably just
a GCCism in my head (due to FIELD_DECL).

> > +++ b/gcc/testsuite/g++.dg/warn/Wuninitialized-14.C
> > @@ -0,0 +1,31 @@
> > +// PR c++/19808
> > +// { dg-do compile { target c++11 } }
> > +// { dg-options "-Wuninitialized" }
> > +
> > +struct A {
> > +  int m;
> > +  int get() const { return m; }
> > +
> > +  A() { }
> > +  A(int) { }
> > +  A(const A &) { }
> > +  A(A *) { }
> > +};
> > +
> > +struct S {
> > +  A a, b;
> > +
> > +  S(int (*)[1]) : a() {}
> > +  S(int (*)[2]) : b(a.get()) {}
> 
> Here, because S::a.m isn't initialized when a.get() returns it,
> we ideally want a warning, we just can't get it, right?
> 
> To make this clear to people who read tests and also easier to
> add a warning if GCC gets smart enough (without causing what
> might look like false positives), I'd suggest to write the tests
> so that they will not trigger these "ideal" warnings.  (In
> the above, it should be doable simply by initializing A::m in
> A's ctors.)

Right, we don't warn though it wouldn't be wrong to warn.  I've
added ": m{}" to make 'm' initialized.

Regtest with the updated patch running...

Marek