On 11/16/20 2:24 PM, Marek Polacek wrote:
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.

I agree it's OK, but may be redundant if the middle-end will provide the same warnings.

Jason

Reply via email to