[clang][Interp] Support base class constructors

[tbaeder]

Summary

I think this is pretty simple since we get the proper constructor as out InitExpr anyway.

[shafik]

I realize it is not directly related but I don't see it in the test suite (maybe I missed it) but also delegating constructors, default member initializaers Vs mem-init list (mem-init list has priority) and mem-init list items out of order of initialization.

clang/test/AST/Interp/records.cpp

190

How about also testing private and virtual as well as multiple bases.

[shafik]

This looks good to me but I would like one other set of eyes on it

[erichkeane]

clang/lib/AST/Interp/ByteCodeExprGen.h

251

We are de-pointering here, why not de-referencing?

If we are OK with that, we can do this as:

if (const auto *RD = T->getPointeeCXXRecordDecl()) 
     return RD;
   return T->getAsCXXRecordDecl();

and most of the work is done for you.

clang/test/AST/Interp/records.cpp

190

I like the idea of testing virtual bases as well.

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.h

251

No reason, as usual I just didn't know this was a thing. Thanks!

clang/test/AST/Interp/records.cpp

190

How would that work in a constexpr context? I get:

array.cpp:48:15: error: constexpr constructor not allowed in class with virtual base class
    constexpr D() : A(17) {}
              ^
array.cpp:45:13: note: virtual base class declared here
  class B : public virtual A {};
            ^~~~~~~~~~~~~~~~
1 error generated.

[erichkeane]

clang/test/AST/Interp/records.cpp

190

Ah! TIL, thanks!

[tbaeder]

clang/test/AST/Interp/records.cpp

190

Ah, this works: https://godbolt.org/z/ern3Yje9q

[tbaeder]

clang/test/AST/Interp/records.cpp

190

Checking this example, I should've tested a lot more non-constexpr stuff I guess.

[tbaeder]

Anything still missing here?

[aaron.ballman]

clang/lib/AST/Interp/ByteCodeExprGen.h

249

clang/test/AST/Interp/records.cpp

234

I'd appreciate some more failure test cases, if they're not already covered elsewhere:

struct Base {
  int Val;
};

struct Derived : Base {
  int OtherVal;

  constexpr Derived(int i) : OtherVal(i) {}
};

// Something here should be diagnosed; either because the Derived ctor is not a
// valid constexpr function or because we're accessing an uninitialized member.
constexpr Derived D(12);
static_assert(D.Val == 0);


// Another test is when a constexpr ctor calls a non-constexpr base class ctor.
struct AnotherBase {
  int Val;
  constexpr AnotherBase(int i) : Val(12 / i) {}
};

struct AnotherDerived : AnotherBase {
  constexpr AnotherDerived(int i) : AnotherBase(i) {}
};
constexpr AnotherDerived Derp(0);

// Skipping the derived class constructor is also
// interesting to consider:
struct YetAnotherBase {
  int Val;
  constexpr YetAnotherBase(int i) : Val(i) {}
};

struct YetAnotherDerived : YetAnotherBase {
  using YetAnotherBase::YetAnotherBase;
  
  int OtherVal;

  constexpr bool doit() const { return Val == OtherVal; }
};

constexpr YetAnotherDerived Oops(0);

[tbaeder]

clang/test/AST/Interp/records.cpp

234

Thanks for the tests. I added them but it doesn't look very good.

[aaron.ballman]

Do you think we should address some of the FIXMEs with base class ctor functionality as part of this patch, given that the failures all seem to be related to that functionality? Or are these failures due to other missing functionality that happened to be exposed here?

[tbaeder]

In D135025#3862031, @aaron.ballman wrote:

Do you think we should address some of the FIXMEs with base class ctor functionality as part of this patch, given that the failures all seem to be related to that functionality? Or are these failures due to other missing functionality that happened to be exposed here?

IMO it's fine to merge this. From a quick look the problems are:

1. We don't reject non-constexpr functions in a constexpr context. (this is a more general problem and can be tested with simple normal functions, so that's probably a better test)
2. In ByteCodeExprGen::getFunction(), we ignore any errors returned, so we can't print a reason for the rejection of the last test.
3. The AnotherBase test is rejected properly, but we don't compute the value that was passed to the constructor correctly. This might just be an uninitialized APValue because we abort execution, but I'd have to investigate.

[aaron.ballman]

Okay, this is incremental progress, so LGTM!

clang/lib/AST/Interp/ByteCodeExprGen.h

private:
   llvm::Optional<unsigned> getGlobalIdx(const VarDecl *VD);
 
   /// Emits the initialized pointer.
   bool emitInitFn() {
     assert(InitFn && "missing initializer");
     return (*InitFn)();
   }
 
+  /// Returns the CXXRecordDecl for the type of the given expression,
+  /// or nullptr if no such decl exists.
+  const CXXRecordDecl *getRecordDecl(const Expr *E) const {
+    QualType T = E->getType();
+    if (const auto *RD = T->getPointeeCXXRecordDecl())
+      return RD;
+    return T->getAsCXXRecordDecl();
+  }
+
 protected:
   /// Variable to storage mapping.
   llvm::DenseMap<const ValueDecl *, Scope::Local> Locals;
 
   /// OpaqueValueExpr to location mapping.
   llvm::DenseMap<const OpaqueValueExpr *, unsigned> OpaqueExprs;
 
   /// Current scope.

clang/lib/AST/Interp/ByteCodeExprGen.cpp

return dereference(
         [this, CE](PrimType T) {
           // Pointer on stack - dereference it.
           if (!this->emitLoadPop(T, CE))
             return false;
           return DiscardResult ? this->emitPop(T, CE) : true;
         });
   }
 
+  case CK_UncheckedDerivedToBase: {
+    if (!this->visit(SubExpr))
+      return false;
+
+    const CXXRecordDecl *FromDecl = getRecordDecl(SubExpr);
+    assert(FromDecl);
+    const CXXRecordDecl *ToDecl = getRecordDecl(CE);
+    assert(ToDecl);
+    const Record *R = getRecord(FromDecl);
+    const Record::Base *ToBase = R->getBase(ToDecl);
+    assert(ToBase);
+
+    return this->emitGetPtrBase(ToBase->Offset, CE);
+  }
+
   case CK_ArrayToPointerDecay:
   case CK_AtomicToNonAtomic:
   case CK_ConstructorConversion:
   case CK_FunctionToPointerDecay:
   case CK_NonAtomicToAtomic:
   case CK_NoOp:
   case CK_UserDefinedConversion:
   case CK_NullToPointer:

clang/lib/AST/Interp/ByteCodeStmtGen.cpp

bool ByteCodeStmtGen<Emitter>::visitFunc(const FunctionDecl *F) {
   ReturnType = this->classify(F->getReturnType());
 
   // Constructor. Set up field initializers.
   if (const auto Ctor = dyn_cast<CXXConstructorDecl>(F)) {
     const RecordDecl *RD = Ctor->getParent();
     const Record *R = this->getRecord(RD);
 
     for (const auto *Init : Ctor->inits()) {
-      const FieldDecl *Member = Init->getMember();
       const Expr *InitExpr = Init->getInit();
+      if (const FieldDecl *Member = Init->getMember()) {
         const Record::Field *F = R->getField(Member);
 
         if (Optional<PrimType> T = this->classify(InitExpr->getType())) {
           if (!this->emitThis(InitExpr))
             return false;
 
           if (!this->visit(InitExpr))
             return false;
 
           if (!this->emitInitField(*T, F->Offset, InitExpr))
             return false;
         } else {
           // Non-primitive case. Get a pointer to the field-to-initialize
           // on the stack and call visitInitialzer() for it.
           if (!this->emitThis(InitExpr))
             return false;
 
           if (!this->emitGetPtrField(F->Offset, InitExpr))
             return false;
 
           if (!this->visitInitializer(InitExpr))
             return false;
 
           if (!this->emitPopPtr(InitExpr))
             return false;
         }
+      } else if (const Type *Base = Init->getBaseClass()) {
+        // Base class initializer.
+        // Get This Base and call initializer on it.
+        auto *BaseDecl = Base->getAsCXXRecordDecl();
+        assert(BaseDecl);
+        const Record::Base *B = R->getBase(BaseDecl);
+        assert(B);
+        if (!this->emitGetPtrThisBase(B->Offset, InitExpr))
+          return false;
+        if (!this->visitInitializer(InitExpr))
+          return false;
+        if (!this->emitPopPtr(InitExpr))
+          return false;
+      }
     }
   }
 
   if (const auto *Body = F->getBody())
     if (!visitStmt(Body))
       return false;
 
   // Emit a guard return to protect against a code path missing one.

clang/test/AST/Interp/records.cpp

 // RUN: %clang_cc1 -fexperimental-new-constant-interpreter -verify %s
 // RUN: %clang_cc1 -fexperimental-new-constant-interpreter -std=c++14 -verify %s
 // RUN: %clang_cc1 -fexperimental-new-constant-interpreter -triple i686 -verify %s
 // RUN: %clang_cc1 -verify=ref %s
 // RUN: %clang_cc1 -verify=ref -std=c++14 %s
 // RUN: %clang_cc1 -verify=ref -triple i686 %s
 
-// expected-no-diagnostics
-
 struct BoolPair {
   bool first;
   bool second;
 };
 
 struct Ints {
   int a = 20;
   int b = 30;

 // Init
 constexpr FourBoolPairs LT;
 // Copy ctor
 constexpr FourBoolPairs LT2 = LT;
 static_assert(LT2.v[0].first == false, "");
 static_assert(LT2.v[0].second == false, "");
 static_assert(LT2.v[2].first == true, "");
 static_assert(LT2.v[2].second == false, "");
+
+class Base {
+public:
+  int i;
+  constexpr Base() : i(10) {}
+  constexpr Base(int i) : i(i) {}
+};
+
+class A : public Base {
+public:
+  constexpr A() : Base(100) {}
+  constexpr A(int a) : Base(a) {}
+};
+constexpr A a{};
+static_assert(a.i == 100, "");
+constexpr A a2{12};
+static_assert(a2.i == 12, "");
+static_assert(a2.i == 200, ""); // ref-error {{static assertion failed}} \
+                                // ref-note {{evaluates to '12 == 200'}} \
+                                // expected-error {{static assertion failed}} \
+                                // expected-note {{evaluates to '12 == 200'}}
+
+namespace MI {
+  class A {
+  public:
+    int a;
+    constexpr A(int a) : a(a) {}
+  };
+
+  class B {
+  public:
+    int b;
+    constexpr B(int b) : b(b) {}
+  };
+
+  class C : public A, public B {
+  public:
+    constexpr C() : A(10), B(20) {}
+  };
+  constexpr C c = {};
+  static_assert(c.a == 10, "");
+  static_assert(c.b == 20, "");
+
+
+  class D : private A, private B {
+    public:
+    constexpr D() : A(20), B(30) {}
+    constexpr int getA() const { return a; }
+    constexpr int getB() const { return b; }
+  };
+  constexpr D d = {};
+  static_assert(d.getA() == 20, "");
+  static_assert(d.getB() == 30, "");
+};
+
+namespace DeriveFailures {
+  struct Base { // ref-note 2{{declared here}}
+    int Val;
+  };
+
+  struct Derived : Base {
+    int OtherVal;
+
+    constexpr Derived(int i) : OtherVal(i) {} // ref-error {{never produces a constant expression}} \
+                                              // ref-note 2{{non-constexpr constructor 'Base' cannot be used in a constant expression}}
+  };
+
+  // FIXME: This is currently not being diagnosed with the new constant interpreter.
+  constexpr Derived D(12); // ref-error {{must be initialized by a constant expression}} \
+                           // ref-note {{in call to 'Derived(12)'}} \
+                           // ref-note {{declared here}} \
+                           // expected-error {{must be initialized by a constant expression}}
+  static_assert(D.Val == 0, ""); // ref-error {{not an integral constant expression}} \
+                                 // ref-note {{initializer of 'D' is not a constant expression}}
+
+#if 0
+  // FIXME: This test is currently disabled because the failing constructor call
+  //   causes us to run into an assertion later on in the new interpreter.
+  //   Once that is fixed, we fail successfully but the diagnostic uses the
+  //   wrong value.
+  struct AnotherBase {
+    int Val;
+    constexpr AnotherBase(int i) : Val(12 / i) {} //ref-note {{division by zero}} \
+                                                  //expected-note {{division by zero}}
+  };
+
+  struct AnotherDerived : AnotherBase {
+    constexpr AnotherDerived(int i) : AnotherBase(i) {}
+  };
+  constexpr AnotherBase Derp(0); // ref-error {{must be initialized by a constant expression}} \
+                                 // ref-note {{in call to 'AnotherBase(0)'}} \
+                                 // expected-error {{must be initialized by a constant expression}} \
+                                 // expected-note {{in call to 'AnotherBase(}}
+                                 // FIXME Previous note uses the wrong value
+#endif
+
+  struct YetAnotherBase {
+    int Val;
+    constexpr YetAnotherBase(int i) : Val(i) {}
+  };
+
+  struct YetAnotherDerived : YetAnotherBase {
+    using YetAnotherBase::YetAnotherBase; //ref-note {{declared here}}
+    int OtherVal;
+
+    constexpr bool doit() const { return Val == OtherVal; }
+  };
+
+  constexpr YetAnotherDerived Oops(0); // ref-error {{must be initialized by a constant expression}} \
+                                       // ref-note {{constructor inherited from base class 'YetAnotherBase' cannot be used in a constant expression}} \
+                                       // expected-error {{must be initialized by a constant expression}}
+                                       // FIXME: Missing reason for rejection.
+};