Add new diagnostic to check for bad member function calls in mem-initializer-lists.

[em-bellows]

Summary

Hi,

This patch adds a new diagnostic that checks for member function calls before all base classes in a mem-initializer-list are initialized, which is undefined behavior. (C++03 12.6.2p8; C++11 12.6.2p13.)

The added code looks for member call expressions which use implicit or explicit 'this' inside of the arguments of a base class initializer to signal the warning.

[em-bellows]

Test suite passes now, didn't realize it wasn't before.

[rtrieu]

Besides the comments above, see http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface on how to get a full diff. Giving Phabricator a full diff makes reviewing easier.

lib/Sema/SemaDeclCXX.cpp

2643

There's multiple visitors in Clang. The RecursiveASTVisitor will visit every AST node, including unevaluated contexts such as sizeof expressions. It would be better to use an EvaluatedExprVisitor which ignores unevaluated expressions. Since there already is an EvaluatedExprVisitor for member initializers, it would be a better idea to try to add to that visitor instead of writing your own to minimize how often these expressions are traversed.

The best place to look is HandleMemberExpr inside UninitializedFieldVisitor. This is were all MemberExpr's that need to be checked are funneled into.

test/SemaCXX/member-call-before-bases-init.cpp

1

Merge this test case with uninitalized.cpp. The warnings here and in uninitalized.cpp come from the same warning group and should be kept together in test cases.

31

f() is from class B, which is fully initialized when class C is being constructed. There is no need for a warning here.

33

Add a test case using an unevaluated context to check that no warning is emitted. For example, sizeof(f()) in an initializer.

test/SemaCXX/uninitialized.cpp

1326

Don't trigger the warning here. The existing warning is good enough and the new warning doesn't add anything new here.

1342

Same as above.

[em-bellows]

Regarding my test case, it is taken almost directly from the C++11 standard I have, and it says the call to the member function 'f' on line 31 of my test case is undefined behavior. Though I'd imagine it would work fine on any reasonable compiler, as it is from the base class B which at that point is initialized as you mentioned.

12.6.2p13 However, if these operations [member function calls as far as this patch considers] are performed in a ctor-initializer (or in a function called directly or indirectly from a ctor-initializer) before all the mem-initializers for base classes have completed, the result of the operation is undefined.

class D : public B, C {
  int i;
  public:
    D() : C(f()), //undefined: calls member function
                  //but base C not yet initialized
    i(f()) { }    //well-defined: bases are all initialized
};

I was hoping to keep this warning as strict as possible, though I realize it is a little surprising. I apologize for not including that context in my summary. I'm new to all of this!

Thank you for the comments and review. I will get to work on everything you mentioned, and I'll make sure to include the full diff next time.

include/clang/Basic/DiagnosticSemaKinds.td

 def note_uninit_in_this_constructor : Note<
   "during field initialization in %select{this|the implicit default}0 "
   "constructor">;
+def warn_member_call_before_bases_init : Warning<
+  "member function %0 called before all base classes were initialized">,
+  InGroup<Uninitialized>;
 def warn_static_self_reference_in_init : Warning<
   "static variable %0 is suspiciously used within its own initialization">,
   InGroup<UninitializedStaticSelfInit>;

lib/Sema/SemaDeclCXX.cpp

   }
 } // namespace
 
+namespace {
+class DiagnoseMemberCallVisitor
+    : public RecursiveASTVisitor<DiagnoseMemberCallVisitor> {
+  Sema &S;
+
+public:
+  DiagnoseMemberCallVisitor(Sema &SemaRef) : S(SemaRef) {}
+
+  bool VisitCXXMemberCallExpr(CXXMemberCallExpr *E) {
+    const Expr *IA = E->getImplicitObjectArgument();
+    if (IA->isImplicitCXXThis() || isa<CXXThisExpr>(IA)) {
+      S.Diag(E->getExprLoc(), diag::warn_member_call_before_bases_init)
+          << E->getDirectCallee();
+    }
+    return true;
+  }
+};
+
+// Diagnoses method calls before all base classes are initialized in a
+// mem-initializer-list which is undefined behaviour.
+// Source: C++03 12.6.2p8; C++11 12.6.2p13.
+static void
+DiagnoseUndefinedMemberFunctionCalls(Sema &SemaRef,
+                                     const CXXConstructorDecl *Constructor) {
+  if (SemaRef.getDiagnostics().isIgnored(
+          diag::warn_member_call_before_bases_init, Constructor->getLocation()))
+    return;
+
+  DiagnoseMemberCallVisitor Visitor(SemaRef);
+  for (const auto *I : Constructor->inits()) {
+    if (!I->isBaseInitializer())
+      continue;
+    Expr *E = I->getInit();
+    Visitor.TraverseStmt(E);
+  }
+}
+} // namespace
+
 /// \brief Enter a new C++ default initializer scope. After calling this, the
 /// caller must call \ref ActOnFinishCXXInClassMemberInitializer, even if
 /// parsing or instantiating the initializer failed.

   SetCtorInitializers(Constructor, AnyErrors, MemInits);
 
   DiagnoseUninitializedFields(*this, Constructor);
+  DiagnoseUndefinedMemberFunctionCalls(*this, Constructor);
 }
 
 void

test/SemaCXX/member-call-before-bases-init.cpp

+// RUN: %clang_cc1 -fsyntax-only -Wuninitialized -verify %s
+
+class A {
+public:
+  A(int);
+};
+
+int g(int);
+
+class B : public A {
+  int j;
+
+public:
+  int f();
+  B() : A(g(f())), // expected-warning{{member function 'f' called before all base classes were initialized}}
+        j(f()) {}  // no-warning
+
+  B(int) : A(this->f()), // expected-warning{{member function 'f' called before all base classes were initialized}}
+           j(0) {}
+};
+
+class C {
+public:
+  C(int);
+};
+
+class D : public B, C {
+  int i;
+
+public:
+  D() : C(f()), // expected-warning{{member function 'f' called before all base classes were initialized}}
+        i(f()) {}  // no-warning
+};

test/SemaCXX/uninitialized.cpp

 
   B(int (*)[4]) : A(foo()) {}
   // expected-warning@-1 {{base_class_access::A' is uninitialized when used here to access 'base_class_access::A::foo'}}
+  // expected-warning@-2 {{member function 'foo' called before all base classes were initialized}}
 };
 
 struct C {

 
   D(int (*)[4]) : C(foo()) {}
   // expected-warning@-1 {{base_class_access::A' is uninitialized when used here to access 'base_class_access::A::foo'}}
+  // expected-warning@-2 {{member function 'foo' called before all base classes were initialized}}
 };
 
 }