Re: [PATCH] c++: Implement -Wuninitialized for mem-initializers [PR19808]

[Marek Polacek via Gcc-patches]

On Mon, Nov 16, 2020 at 02:02:00PM -0500, Jason Merrill via Gcc-patches wrote:
> On 11/16/20 12:42 PM, Martin Sebor wrote:
> > On 11/15/20 3:44 PM, Marek Polacek via Gcc-patches wrote:
> > > 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.  It is
> > > possible that we might want to tweak that if the current approach turns
> > > out to be flawed.
> > > 
> > > And this is where things stop being simple.  I had to implement certain
> > > gymnastics to handle cases like
> > > 
> > >    struct S {
> > >      int a, b, c;
> > >      S() : a((b = 42)), c(b) { }
> > >    };
> > > 
> > > where the initializer for 'a' surreptitiously initializes 'b'.  And since
> > > 'b' isn't read in the assignment, we can't warn that it's being used
> > > uninitialized.
> > > 
> > > Also, initializer lists.  Consider:
> > > 
> > >    struct A {
> > >      int x, y, z;
> > >    };
> > > 
> > >    struct S {
> > >      A a;
> > >      S() : a{ 1, 2, a.y } {} // #1
> > >      S(int) : a{ a.y, 2, 3 } {} // #2
> > >    };
> > > 
> > > In #1, a.y refers to an initialized element, so we oughtn't warn.  But
> > > in #2, a.y refers to an uninitialized element, so we should warn.  To
> > > that end I've added a vector that tracks which elements of an initializer
> > > list are already initialized, so that when we encounter them later in
> > > the list, we don't warn.  This is the find_uninit_data->list_inits
> > > thing.  It will contain paths to the initialized elements, so e.g.
> > > 
> > >    ((S *)this)->a.y
> > > 
> > > These are edge cases, but some developers live on the edge.
> > > 
> > > This patch also handles delegating constructors and initializing base
> > > classes.
> > > 
> > > A couple of TODOs:
> > > - the same approach could be used to warn for NSDMIs, but I think
> > > it's out
> > >    of scope for this patch;
> > > - the diagnostic for anonymous unions/structs should be improved
> > >    to say the name of the member instead of "<anonymous>";
> > > - certain uninitialized warnings in initializer-lists are issued
> > >    by the FE as well as the ME.  It's unclear to me what we want to do
> > >    here;
> > > - using uninitialized members of base classes is not implemented yet.
> > 
> > This is a great enhancement!  Detecting the misues even in inline
> > functions is especially useful.
> 
> Why doesn't the middle-end warning work for inline functions?
> 
> > I installed the patch to see how
> > it handles some of the cases the middle end was recently enhanced
> > to detect.  Below is a test case I tried and the outupt.  Clearly,
> > the middle end warning doesn't work as intended for C++ ctors, so
> > that's something for me to look into.
> 
> Please.  If we can handle these warnings in the middle-end (where we have
> data flow info), that should be more reliable than the front-end walking the
> trees.

For function bodies, definitely.  I don't we should attempt to use walk_tree
on them.  But for initializers, like in this patch, I think it's okay.
Though of course it will still issue false positives for cases like ?: or
a || b, or a && b, etc. in the initializer.

Marek