warn when ctor-initializer leaves uninitialized data?

Clang Frontend
#1

Hello,

What would the interest be for a new warning that warns when POD member variables are left out of the constructor initializer list and end up uninitialized? That’s bitten me many times and is especially hard on beginners. Please correct me if I’m wrong but clang doesn’t currently warn about this. It looks like this warning is currently being implemented for gcc (bug 2972). I don’t have any experience with clang so I’m not sure if this is the right place at all, but I took a stab at it last night on a whim and it looks like adding the warning at the end of BuildImplicitMemberInitializer() in SemaDeclCXX.cpp does the trick. If there is interest, would this warning want to be added to the -Wuninitialized group or would it want to be separate because it affects a lot of existing code? What I have right now looks like this:

basic.cpp:1:22: warning: member ‘y’ is missing from constructor initializer list [-Wuninitialized-member]

struct S { int x, y; S() : x() {} };
^

  • Evan
#2

Hi Evan,

Thanks for looking into this. Did you mean to include a patch file of
your current changes?

We'd have to see how accurate this diagnostic was (in finding bugs,
not noise) - my hunch would be that it's probably not accurate enough
to turn on by default, at least. Though that doesn't mean it can't
live under -Wuninitialized (I haven't checked what else is there, how
accurate they are, etc).

A few questions (I think I mentioned these on IRC, perhaps - or just
crossed my mind):

1) how well does this handle unions? struct S { union { int x, y; };
S() : x() {} }; - as an addition to this. I was wondering if we could
roll this checking up into the same machinery we need/could use/have
some of for errors relating to competing initialization (eg: we
currently correctly produce an error when you try to initialize both x
and y in my example)
2) Is there any way to silence the warning without introducing
initializations that may not be required/desired? (one interesting
case I came across in Clang recently is, effectively, a discriminated
union - some members are used/unused based on which enum value is
currently held - there's no need to initialize the other members as
they are dead)

Thanks,
- David

#3

Hi Evan,

Thanks for looking into this. Did you mean to include a patch file of
your current changes?

Sure, glad there’s interest! I wanted to see if I was on track before submitting a patch. I’ve attached a patch now.

We’d have to see how accurate this diagnostic was (in finding bugs,
not noise) - my hunch would be that it’s probably not accurate enough
to turn on by default, at least. Though that doesn’t mean it can’t
live under -Wuninitialized (I haven’t checked what else is there, how
accurate they are, etc).

I agree, I wouldn’t turn it on by default as a lot of existing code sometimes doesn’t initialize members in constructor initializers for a variety of reasons (such as performance or discriminating unions like you mentioned). It’s been great for the few personal projects I’ve tested though and all the warnings it generated were legitimate. It looks like -Wuninitialized is currently more for local flow-based uninitialized variable analysis although it is logically similar to this warning.

A few questions (I think I mentioned these on IRC, perhaps - or just
crossed my mind):

  1. how well does this handle unions? struct S { union { int x, y; };
    S() : x() {} }; - as an addition to this. I was wondering if we could
    roll this checking up into the same machinery we need/could use/have
    some of for errors relating to competing initialization (eg: we
    currently correctly produce an error when you try to initialize both x
    and y in my example)

I just tested my change further and it doesn’t warn on union members, unnamed structs and bitfields, or double-warn about uninitialized members in superclasses:

// warning about a
struct A { int a; };

// no warnings
struct B { A b; unsigned : 8; };

// no warnings
struct C { int c; C() : c() {} };

// warning about d but not a
struct D : A { int d; D() {} };

// warning about e2
struct E { int e1, e2; E() : e1() {} };

// two warnings about f, one for each constructor
struct F { int f; F() {} F(int) : f() {} F(int, int) {} };

// no warnings for union members
struct G { union { int g1, g2; }; };

// no warnings for a separate constructor
struct H { int h; H(); };
H::H() : h() {}

// warning for anonymous struct members (only when used do to laziness)
struct I { struct { int i; }; };

// warning about j (Wuninitialized, not Wuninitialized-member)
struct J { int j; J() : j(j) {} };

I’ll make more test cases and add them to clang’s tests soon.

  1. Is there any way to silence the warning without introducing
    initializations that may not be required/desired? (one interesting
    case I came across in Clang recently is, effectively, a discriminated
    union - some members are used/unused based on which enum value is
    currently held - there’s no need to initialize the other members as
    they are dead)

The way to silence the warning is currently through clang’s diagnostic pragmas, which would be another reason to have the warning be separate from -Wuninitialized. I can look into adding something like attribute((uninitialized)) if you think that would be preferable. When I’m ready, should I submit my patch to cfe-commits?

  • Evan

Wuninitialized-member.diff (1.91 KB)

#4

I like this idea, though like David said it might not be great for all projects. I think it should be off for POD structs, though, like your first example:

// warning about a
struct A { int a; };

I think fewer people mistakenly expect POD structs to be initialized, and of course initializing 'a' would make the struct no-longer POD. The issues I've come across while working with Clang are always POD members in non-POD structures. What do you think?

Jordy

#5

That makes a lot of sense, especially for compiling C-like code. I like that much better. I’ve updated my patch with test cases and a check for POD types. I’m still not sure where to put this diagnostic. It’s tentatively in a new warning called -Wuninitialized-member in the -Wextra group. Is that the best place for it, or would it fit better somewhere else?

  • Evan

Wuninitialized-member.diff (4.12 KB)

#6

I like this idea, though like David said it might not be great for all
projects. I think it should be off for POD structs, though, like your first
example:

> // warning about a
> struct A { int a; };

I think fewer people mistakenly expect POD structs to be initialized, and
of course initializing 'a' would make the struct no-longer POD. The issues
I've come across while working with Clang are always POD members in non-POD
structures. What do you think?

That makes a lot of sense, especially for compiling C-like code. I like that
much better. I've updated my patch with test cases and a check for POD
types. I'm still not sure where to put this diagnostic. It's tentatively in
a new warning called -Wuninitialized-member in the -Wextra group. Is that
the best place for it, or would it fit better somewhere else?

Where's GCC going to put it?

I'd also consider the phrasing of the diagnostic a little, perhaps

"member %0 is not initialized in the constructor initializer list"

Though I'm not too fussed on this front.

Have you tried/got tests with non-trivial types that don't need to be
explicitly listed in the ctor to be safe?

struct foo { foo(); };
struct bar { foo f; bar() { } }; // don't tell me that 'f' needs to be
in the init list

You don't need to test the DiagnosticLevel - just emit the diagnostic
& the underlying machinery will take care of printing it out, or not.
The only time we check the DiagnosticLevel is if we're going to do
substantial/non-trivial work to diagnose the problem - if the warning
is off, we can save ourselves from doing all that work.

Thanks,
- David

#7

And likewise for those that do:

struct pod { int a, b; }
struct composite { pod p; composite() {} };

Jordy

#8

Hi Evan,

this sounds like a really cool warning. I tried building chromium with
it, here are a few examples where it warns but probably shouldn't:

// Warns about path_ not being initialized (a char[PATH_MAX])
FileID::FileID(const char *path) {
  strlcpy(path_, path, sizeof(path_));
}

  // Warns about members not getting set, but Clear() does that
  KeyValueEntry() {
    Clear();
  }

  // Same thing again
  Var(DontManage, PP_Var var) {
    var_ = var;
    needs_release_ = false;
  }

This warning gave me 9 false positives and 0 bugs in the first 70
files I tried. Initializing variables not in the initializer list but
in the constructor body is a fairly common pattern. Can you think of
ways to make these cases work?

Nico

#9

Another case where the warning might not be desired is if a class overloads operator new and initializes its allocation itself?