[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

165

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

260

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

165

I like the idea of testing virtual bases as well.

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.h

260

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

clang/test/AST/Interp/records.cpp

165

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

165

Ah! TIL, thanks!

[tbaeder]

clang/test/AST/Interp/records.cpp

165

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

[tbaeder]

clang/test/AST/Interp/records.cpp

165

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

258

clang/test/AST/Interp/records.cpp

209

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

209

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 -verify=ref %s
 
-// ref-no-diagnostics
-// 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);
+};