Warn about UB at member function calls from base class ctor initializers.

[teemperor]

Summary

According to [C++14/17 12.6.2 p16] [C++11 12.6.2 p13] calling member functions of the current
class before all the base classes are initialized is undefined behavior.
Some compilers (such as GCC 5.3 + ubsan) can produce a segfaulting code 
in the program when such code is encountered (see GCC bug #70035),
so clang should generate a warning about the UB.

[teemperor]

• Fixed indentation

[filcab]

You might want to mention that it's 12.6.2p16 in C++14/17 but p13 in C++11.
I wonder if we should have the example in the standard, verbatim. (Plus the added tests you made)

[filcab]

I meant changing the diff, but if you prefer to have a smaller comment, I'm ok with the different "paths" in the standards being mentioned only in the commit message.

lib/Sema/SemaDeclCXX.cpp

3981

For someone reading the source code, it's probably best to mention the different "paths" in the standards here too.
For my usual source browsing, as long as it's in the comment *or* the commit message, I'll eventually see it. But it might throw some people off when they look at C++11, and there's no p16 in that place :-)

[teemperor]

Oh, totally forgot that I also had a reference in the code, thanks! Will fix it ASAP.

Adding the example from the standard to the test cases sounds good. It actually tests a few things that aren't covered by the current test.

[teemperor]

• Standard reference is now specifying what standard (C++11)
• Added test based on the standard example
• Also check base classes now for member calls (to pass the test from the standard example).

[filcab]

The patch needs to get through clang-format.
Should we deal with typeid and dynamic_cast, since they're mentioned in the same paragraph in the standard?

lib/Sema/SemaDeclCXX.cpp

3964

What's the rationale for warning if getRecordDecl returns nullptr?

3966

Please run clang-format on the patch.

3980

clang-format

3981

"before the base class is initialized"?

[rtrieu]

Have you looked at http://reviews.llvm.org/D6561 which was a previous attempt at this warning?

[teemperor]

No, didn't saw the thread so far. I assume the comments in there are still valid, so I'll update this patch.
Thanks for the feedback so far!

[teemperor]

• Moved checks to the UninitializedFieldVisitor
• Also check for dynamic_cast on this during construction

[teemperor]

• Added checks and tests for typeid
• Moved warnings into same warning group
• Check that only possibly evaluated expressions are checked

[teemperor]

Merged all tests into one file.

[NoQ]

Sorry, couldn't help it. This patch looks so useful and so lonely :)

include/clang/Basic/DiagnosticSemaKinds.td

6782

I don't see these things referred to as "ctor-initializers" in other diagnostics. I'd rather say "initializer".

Also "has undefined behavior" is shorter and nicer.

lib/Sema/SemaDeclCXX.cpp

2295

I'd be scared not to initialize this variable to null.

Also, this visitor scans only one constructor throughout its lifetime, right?

2588–2589

Ugh.

This is worth a test case!

include/clang/Basic/DiagnosticGroups.td

 def : DiagGroup<"strict-overflow=3">;
 def : DiagGroup<"strict-overflow=4">;
 def : DiagGroup<"strict-overflow=5">;
 def : DiagGroup<"strict-overflow">;
 
 def InvalidOffsetof : DiagGroup<"invalid-offsetof">;
 def : DiagGroup<"strict-prototypes">;
 def StrictSelector : DiagGroup<"strict-selector-match">;
+def UndefinedCallFromCtor : DiagGroup<"undefined-call-from-ctor">;
 def MethodDuplicate : DiagGroup<"duplicate-method-match">;
 def ObjCCStringFormat : DiagGroup<"cstring-format-directive">;
 def CoveredSwitchDefault : DiagGroup<"covered-switch-default">;
 def SwitchBool     : DiagGroup<"switch-bool">;
 def SwitchEnum     : DiagGroup<"switch-enum">;
 def Switch         : DiagGroup<"switch">;
 def ImplicitFallthroughPerFunction :
   DiagGroup<"implicit-fallthrough-per-function">;

include/clang/Basic/DiagnosticSemaKinds.td

def err_invalid_conversion_between_vector_and_integer : Error<
   "invalid conversion between vector type %0 and integer type %1 "
   "of different size">;
 
 def err_opencl_function_pointer_variable : Error<
   "pointers to functions are not allowed">;
 
 def err_opencl_taking_function_address : Error<
   "taking address of function is not allowed">;
+def warn_member_call_in_ctor_init : Warning<
+  "member function call %0 in ctor-initializer for base class %1 results in "
+  "undefined behavior">,
+  InGroup<UndefinedCallFromCtor>, DefaultIgnore;
+def warn_dynamic_cast_in_ctor_init : Warning<
+  "dynamic_cast with this as operand in ctor-initializer for base class %0 "
+  "results in undefined behavior">,
+  InGroup<UndefinedCallFromCtor>, DefaultIgnore;
+def warn_typeid_in_ctor_init : Warning<
+  "typeid with this as operand in ctor-initializer for base class %0 "
+  "results in undefined behavior">,
+  InGroup<UndefinedCallFromCtor>, DefaultIgnore;
+
 
 def err_invalid_conversion_between_vector_and_scalar : Error<
   "invalid conversion between vector type %0 and scalar type %1">;
 
 // C++ member initializers.
 def err_only_constructors_take_base_inits : Error<
   "only constructors take base initializers">;
 

lib/Sema/SemaDeclCXX.cpp

class UninitializedFieldVisitor
     // If non-null, add a note to the warning pointing back to the constructor.
     const CXXConstructorDecl *Constructor;
     // Variables to hold state when processing an initializer list.  When
     // InitList is true, special case initialization of FieldDecls matching
     // InitListFieldDecl.
     bool InitList;
     FieldDecl *InitListFieldDecl;
     llvm::SmallVector<unsigned, 4> InitFieldIndex;
+    // The base class that is initialized by the current CXXCtorInitializer
+    // or 0 if the current CXXCtorInitializer isn't initializing a base class.
+    const Type *CurrentBaseClass;
 
   public:
     typedef EvaluatedExprVisitor<UninitializedFieldVisitor> Inherited;
     UninitializedFieldVisitor(Sema &S,
                               llvm::SmallPtrSetImpl<ValueDecl*> &Decls,
                               llvm::SmallPtrSetImpl<QualType> &BaseClasses)
       : Inherited(S.Context), S(S), Decls(Decls), BaseClasses(BaseClasses),
         Constructor(nullptr), InitList(false), InitListFieldDecl(nullptr) {}
void CheckInitializer(Expr *E, const CXXConstructorDecl *FieldConstructor,
       // Remove Decls that may have been initialized in the previous
       // initializer.
       for (ValueDecl* VD : DeclsToRemove)
         Decls.erase(VD);
       DeclsToRemove.clear();
 
       Constructor = FieldConstructor;
       InitListExpr *ILE = dyn_cast<InitListExpr>(E);
+      CurrentBaseClass = BaseClass;
 
       if (ILE && Field) {
         InitList = true;
         InitListFieldDecl = Field;
         InitFieldIndex.clear();
         CheckInitListExpr(ILE);
       } else {
         InitList = false;
void VisitCXXConstructExpr(CXXConstructExpr *E) {
           if (ICE->getCastKind() == CK_NoOp)
             ArgExpr = ICE->getSubExpr();
         HandleValue(ArgExpr, false /*AddressOf*/);
         return;
       }
       Inherited::VisitCXXConstructExpr(E);
     }
 
+    void VisitCXXDynamicCastExpr(CXXDynamicCastExpr *E) {
+      // Calling dynamic_cast on the current object before
+      // all base classes are initialized is undefined behavior.
+      // [C++14/17 12.6.2 p16] [C++11 12.6.2 p13]
+
+      // Check that we are currently initializing a base class (which means
+      // that not all base classes are initialized at the moment).
+      if (CurrentBaseClass) {
+        // Simple check if the operand of the dynamic_cast is a this expression
+        // and therefore results in undefined behavior.
+        if (isa<CXXThisExpr>(E->getSubExpr())) {
+          S.Diag(E->getExprLoc(), diag::warn_dynamic_cast_in_ctor_init)
+              << CurrentBaseClass->getAsCXXRecordDecl();
+        }
+      }
+      Inherited::VisitCXXDynamicCastExpr(E);
+    }
+
+    void VisitCXXTypeidExpr(CXXTypeidExpr *E) {
+      // Calling typeid on the current object before
+      // all base classes are initialized is undefined behavior.
+      // [C++14/17 12.6.2 p16] [C++11 12.6.2 p13]
+
+      // Check that we are currently initializing a base class (which means
+      // that not all base classes are initialized at the moment).
+      if (CurrentBaseClass) {
+        // Simple check if the operand of the typeid is a this expression
+        // and therefore results in undefined behavior.
+        if (!E->isTypeOperand() && isa<CXXThisExpr>(E->getExprOperand())) {
+          S.Diag(E->getExprLoc(), diag::warn_typeid_in_ctor_init)
+              << CurrentBaseClass->getAsCXXRecordDecl();
+        }
+      }
+      Inherited::VisitCXXTypeidExpr(E);
+    }
+
     void VisitCXXMemberCallExpr(CXXMemberCallExpr *E) {
+      // Calling a member function of the current object before
+      // all base classes are initialized is undefined behavior.
+      // [C++14/17 12.6.2 p16] [C++11 12.6.2 p13]
+
+      // Check that we are currently initializing a base class (which means
+      // that not all base classes are initialized at the moment).
+      if (CurrentBaseClass) {
+
+        // Calling a member function of the current object in this context
+        // is undefined behavior.
+        // As getImplicitObjectArgument() returns 0 for member pointer
+        // calls we use dyn_cast_or_null instead of isa.
+        if (dyn_cast_or_null<CXXThisExpr>(E->getImplicitObjectArgument())) {
+          S.Diag(E->getExprLoc(), diag::warn_member_call_in_ctor_init)
+              << E << CurrentBaseClass->getAsCXXRecordDecl();
+        }
+
+        // Same as above but taking the implicit cast when calling
+        // a member function of a parent class into account.
+        auto ImplicitCast =
+            dyn_cast_or_null<ImplicitCastExpr>(E->getImplicitObjectArgument());
+        if (ImplicitCast) {
+          if (isa<CXXThisExpr>(ImplicitCast->getSubExpr())) {
+            S.Diag(E->getExprLoc(), diag::warn_member_call_in_ctor_init)
+                << E << CurrentBaseClass->getAsCXXRecordDecl();
+          }
+        }
+      }
+
       Expr *Callee = E->getCallee();
       if (isa<MemberExpr>(Callee)) {
         HandleValue(Callee, false /*AddressOf*/);
         for (auto Arg : E->arguments())
           Visit(Arg);
         return;
       }
 
if (!Initializers.empty()) {
       Constructor->setCtorInitializers(baseOrMemberInitializers);
     }
 
     // Let template instantiation know whether we had errors.
     if (AnyErrors)
       Constructor->setInvalidDecl();
 
     return false;
   }
 
   BaseAndFieldInfo Info(*this, Constructor, AnyErrors);
 
   // We need to build the initializer AST according to order of construction
   // and not what user specified in the Initializers list.
   CXXRecordDecl *ClassDecl = Constructor->getParent()->getDefinition();
   if (!ClassDecl)
     return true;
   
   bool HadError = false;
 
   for (unsigned i = 0; i < Initializers.size(); i++) {
     CXXCtorInitializer *Member = Initializers[i];
 
     if (Member->isBaseInitializer())
       Info.AllBaseFields[Member->getBaseClass()->getAs<RecordType>()] = Member;
     else {
       Info.AllBaseFields[Member->getAnyMember()->getCanonicalDecl()] = Member;
 
       if (IndirectFieldDecl *F = Member->getIndirectMember()) {
         for (auto *C : F->chain()) {
           FieldDecl *FD = dyn_cast<FieldDecl>(C);
           if (FD && FD->getParent()->isUnion())
             Info.ActiveUnionMember.insert(std::make_pair(
                 FD->getParent()->getCanonicalDecl(), FD->getCanonicalDecl()));

test/SemaCXX/warn-undefined-in-ctor.cpp

+// RUN: %clang_cc1 -fsyntax-only -std=c++11 -verify %s -Wundefined-call-from-ctor
+
+// Warn about member function calls
+namespace MemberFunctionCallsTests {
+
+  // Helper class for the following test cases.
+  class A {
+  public:
+    A(int i) {}
+  };
+
+  // Calling member functions before bass class initialized is undefined behavior.
+  class B : public A {
+    int member_;
+
+  public:
+    B()
+        : A(1 + get_i()) { // expected-warning {{member function call this->get_i() in ctor-initializer for base class 'A' results in undefined behavior}}
+    }
+    B(float var)
+        : A(sizeof(get_i())) { // no-warning
+    }
+    B(int var) : A(0), member_(get_i()) {} // no-warning
+
+    int get_i() { return 2; }
+  };
+
+  // Same as previous test but with explicit this.
+  class C : public A {
+  public:
+    C()
+        : A(this->get_i() + 1) { // expected-warning {{member function call this->get_i() in ctor-initializer for base class 'A' results in undefined behavior}}
+    }
+
+    int get_i() { return 2; }
+  };
+
+  // Check that the whole ctor-initializer is checked for member calls.
+  class OtherA {
+  public:
+    OtherA(int i) {}
+  };
+
+  class D : public OtherA, public A {
+  public:
+    D()
+        : OtherA(this->get_i() + 1), A(this->get_i() + 1) { // expected-warning {{member function call this->get_i() in ctor-initializer for base class 'OtherA' results in undefined behavior}} \
+                                                            // expected-warning {{member function call this->get_i() in ctor-initializer for base class 'A' results in undefined behavior}}
+    }
+
+    int get_i() { return 2; }
+  };
+
+  // Calling static functions of this class is not undefined behavior.
+  class E : public A {
+  public:
+    E() : A(this->get_i() + 1) { // no-warning
+    }
+
+    static int get_i() { return 2; }
+  };
+
+  // Calling other functions of this class is not undefined behavior.
+  int other_foo() { return 2; }
+  class F : public A {
+  public:
+    F() : A(other_foo()) {} // no-warning
+  };
+
+  // Calling member functions of other classes is not undefined behavior.
+  class G : public A {
+  public:
+    G(B &b) : A(b.get_i()) {} // no-warning
+  };
+}
+
+
+
+// Warn about member function calls - example code from [C++11 12.6.2 p13]
+namespace MemberFunctionCallsStdTests {
+
+  class A {
+  public:
+    A(int);
+  };
+
+  class B : public A {
+    int j;
+
+  public:
+    int f();
+    B()
+        : A(f()),   // expected-warning {{member function call this->f() in ctor-initializer for base class 'A' results in undefined behavior}}
+          j(f()) {} // no-warning
+  };
+
+  class C {
+  public:
+    C(int);
+  };
+  class D : public B, C {
+    int i;
+
+  public:
+    D()
+        : C(f()),   // expected-warning {{member function call this->f() in ctor-initializer for base class 'C' results in undefined behavior}}
+          i(f()) {} // no-warning
+  };
+
+}
+
+
+
+
+// Warn about dynamic_cast on this
+namespace DynamicCastTests {
+
+  // Helper class for the following test case.
+  class A {
+  public:
+    A(A *a) {}
+    A(unsigned long a) {}
+  };
+
+  // Calling dynamic cast on this before base class is initialized is undefined
+  // behavior.
+  class B : public A {
+
+    A *j;
+
+  public:
+    B()
+        : A(dynamic_cast<A *>(this) + 1), j(nullptr) { // expected-warning {{dynamic_cast with this as operand in ctor-initializer for base class 'A' results in undefined behavior}}
+    }
+    B(float var)
+        : A(sizeof(dynamic_cast<A *>(this) + 1)), j(nullptr) { // no-warning
+    }
+    B(int var) : A(nullptr), j(dynamic_cast<A *>(this)) {} // no-warning
+  };
+}
+
+
+
+// Warn about typeid on this
+namespace std {
+  class type_info {
+  };
+}
+
+namespace TypeidTests {
+
+  class A {
+  public:
+    A(const std::type_info& a) {}
+    A(unsigned a) {}
+  };
+
+  // Calling dynamic cast on this before base class is initialized is undefined
+  // behavior.
+  class B : public A {
+
+    const std::type_info& j;
+
+  public:
+    B()
+        : A(typeid(this)), j(typeid(nullptr)) { // expected-warning {{typeid with this as operand in ctor-initializer for base class 'A' results in undefined behavior}}
+    }
+    B(float var)
+        : A(sizeof(typeid(this))), j(typeid(nullptr)) { // no-warning
+    }
+    B(int var) : A(typeid(nullptr)), j(typeid(this)) {} // no-warning
+  };
+
+}