[clang][Interp] Start implementing record types

[tbaeder]

Summary

This is just a humble beginning of course.

Posting this now for your weekend reading. And also to keep the individual patches small.

[erichkeane]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

316

I THINK Member is a ValueDecl because it could be a member function, right? So forcing it to be a FieldDecl here is likely not valid. Perhaps in the 'non-FieldDecl' case we could jsut return false for now?

ALSO, don't do a dyn_cast followed by an assert, cast will do the assert for you.

659

Eh?

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

316

Right, I was just trying to limit the code to the subset I've implemented for now. I can try to make it more defensive.

659

As far as I understand, actually visiting the body of the constructor will require a bit more setup work of this pointers and such. So for now, not implemented and a FIXME.

[shafik]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

316

Also I believe this can also be an IndirectFieldDecl (anonymous union members) or a ValueDecl for static data members.

Maybe outline what is left to fill in a comment?

672

This section of code looks duplicated w/ the above, can it be factored out or will they diverge as you fill in more details?

clang/test/AST/Interp/records.cpp

8

How about also have a field that is a struct and initializing that.

Also using initializer lists in in class member initializers and also designated initializers as well.

I am not sure if unions works yet but anon union members as well.

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

672

The version above will go away in my next patch to record implementation.

clang/test/AST/Interp/records.cpp

8

Struct initializers here aren't supported yet (that's what the "Handle initializer for non-primitive values" assert in VisitRecordInitializer is about). I added a FIXME comment and a test for designated initializers.

[aaron.ballman]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

316

There are quite a few kinds of declarations, but many of them don't matter or only matter in special circumstances. Like StaticAssertDecl shouldn't be something we can visit here, but technically TypedefDecl can because of VLA evaluations. MSPropertyDecl is one that could possibly matter as well.

640

Do we have to do anything special if the ctor is an inheriting ctor?

685

CXXMemberCallExpr as well?

687

What if this comes back as nullptr (does getFunction() handle that gracefully)?

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

640

I have a feeling we do, but I'll tackle inheritance and virtual functions later.

685

Yup, that's unhandled here right now it seems, I can add it to this patch or add it in a later one.

687

It does not :) I was relying on a crash somewhere when I have a test case for it.

[aaron.ballman]

LGTM aside from a minor nit, we can handle the other cases in follow-ups.

clang/lib/AST/Interp/ByteCodeExprGen.cpp

640

Okay, that seems reasonable to me.

685

I'm fine doing it in a follow-up.

687

I'd rather use an explicit assert for that instead of relying on a crash; it's easier to spot the assert in that case.

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

687

I forgot I hadn't posted this to phab yet, but https://reviews.llvm.org/D134699 introduces a getFunction that asserts that the given FunctionDecl is not null.

[aaron.ballman]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

687

Ah, okay, that works fine for me then.

clang/lib/AST/Interp/ByteCodeExprGen.h

public:
 
   // Expression visitors - result returned on interp stack.
   bool VisitCastExpr(const CastExpr *E);
   bool VisitIntegerLiteral(const IntegerLiteral *E);
   bool VisitParenExpr(const ParenExpr *E);
   bool VisitBinaryOperator(const BinaryOperator *E);
   bool VisitCXXDefaultArgExpr(const CXXDefaultArgExpr *E);
   bool VisitCallExpr(const CallExpr *E);
+  bool VisitCXXDefaultInitExpr(const CXXDefaultInitExpr *E);
   bool VisitCXXBoolLiteralExpr(const CXXBoolLiteralExpr *E);
   bool VisitCXXNullPtrLiteralExpr(const CXXNullPtrLiteralExpr *E);
   bool VisitUnaryOperator(const UnaryOperator *E);
   bool VisitDeclRefExpr(const DeclRefExpr *E);
   bool VisitImplicitValueInitExpr(const ImplicitValueInitExpr *E);
   bool VisitSubstNonTypeTemplateParmExpr(const SubstNonTypeTemplateParmExpr *E);
   bool VisitArraySubscriptExpr(const ArraySubscriptExpr *E);
   bool VisitInitListExpr(const InitListExpr *E);
   bool VisitConstantExpr(const ConstantExpr *E);
   bool VisitUnaryExprOrTypeTraitExpr(const UnaryExprOrTypeTraitExpr *E);
+  bool VisitMemberExpr(const MemberExpr *E);
 
 protected:
   bool visitExpr(const Expr *E) override;
   bool visitDecl(const VarDecl *VD) override;
 
 protected:
   /// Emits scope cleanup instructions.
   void emitCleanup();
protected:
   /// Evaluates an expression for side effects and discards the result.
   bool discard(const Expr *E);
   /// Evaluates an expression and places result on stack.
   bool visit(const Expr *E);
   /// Compiles an initializer.
   bool visitInitializer(const Expr *E);
   /// Compiles an array initializer.
   bool visitArrayInitializer(const Expr *Initializer);
+  /// Compiles a record initializer.
+  bool visitRecordInitializer(const Expr *Initializer);
 
   /// Visits an expression and converts it to a boolean.
   bool visitBool(const Expr *E);
 
   /// Visits an initializer for a local.
   bool visitLocalInitializer(const Expr *Init, unsigned I) {
     if (!this->emitGetPtrLocal(I, Init))
       return false;

clang/lib/AST/Interp/ByteCodeExprGen.cpp

if (E->getKind() == UETT_SizeOf) {
     }
 
     return this->emitConst(E, Size.getQuantity());
   }
 
   return false;
 }
 
+template <class Emitter>
+bool ByteCodeExprGen<Emitter>::VisitMemberExpr(const MemberExpr *E) {
+  // 'Base.Member'
+  const Expr *Base = E->getBase();
+  const ValueDecl *Member = E->getMemberDecl();
+
+  if (!this->visit(Base))
+    return false;
+
+  // Base above gives us a pointer on the stack.
+  const auto *FD = dyn_cast<FieldDecl>(Member);
+  assert(FD);
+  const RecordDecl *RD = FD->getParent();
+  assert(RD);
+  const Record *R = P.getOrCreateRecord(RD);
+  const Record::Field *F = R->getField(FD);
+  // Leave a pointer to the field on the stack.
+  return this->emitGetPtrField(F->Offset, E);
+}
+
 template <class Emitter> bool ByteCodeExprGen<Emitter>::discard(const Expr *E) {
   OptionScope<Emitter> Scope(this, /*NewDiscardResult=*/true);
   return this->Visit(E);
 }
 
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::visit(const Expr *E) {
   OptionScope<Emitter> Scope(this, /*NewDiscardResult=*/false);
bool ByteCodeExprGen<Emitter>::visitArrayInitializer(const Expr *Initializer) {
   } else {
     assert(false && "Unknown expression for array initialization");
   }
 
   return true;
 }
 
 template <class Emitter>
+bool ByteCodeExprGen<Emitter>::visitRecordInitializer(const Expr *Initializer) {
+  assert(Initializer->getType()->isRecordType());
+
+  if (const auto CtorExpr = dyn_cast<CXXConstructExpr>(Initializer)) {
+    const CXXConstructorDecl *Ctor = CtorExpr->getConstructor();
+    const RecordDecl *RD = Ctor->getParent();
+    const Record *R = getRecord(RD);
+
+    for (const auto *Init : Ctor->inits()) {
+      const FieldDecl *Member = Init->getMember();
+      const Expr *InitExpr = Init->getInit();
+
+      if (Optional<PrimType> T = classify(InitExpr->getType())) {
+        const Record::Field *F = R->getField(Member);
+
+        if (!this->emitDupPtr(Initializer))
+          return false;
+
+        if (!this->visit(InitExpr))
+          return false;
+
+        if (!this->emitInitField(*T, F->Offset, Initializer))
+          return false;
+      } else {
+        assert(false && "Handle initializer for non-primitive values");
+      }
+    }
+
+    // FIXME: Actually visit() the constructor Body
+    const Stmt *Body = Ctor->getBody();
+    (void)Body;
+    return true;
+  } else if (const auto *InitList = dyn_cast<InitListExpr>(Initializer)) {
+    const Record *R = getRecord(InitList->getType());
+
+    unsigned InitIndex = 0;
+    for (const Expr *Init : InitList->inits()) {
+      const Record::Field *FieldToInit = R->getField(InitIndex);
+
+      if (Optional<PrimType> T = classify(Init->getType())) {
+        if (!this->emitDupPtr(Initializer))
+          return false;
+
+        if (!this->visit(Init))
+          return false;
+
+        if (!this->emitInitField(*T, FieldToInit->Offset, Initializer))
+          return false;
+      }
+      ++InitIndex;
+    }
+
+    return true;
+  }
+
+  return false;
+}
+
+template <class Emitter>
 bool ByteCodeExprGen<Emitter>::visitInitializer(const Expr *Initializer) {
   QualType InitializerType = Initializer->getType();
 
   if (InitializerType->isArrayType())
     return visitArrayInitializer(Initializer);
 
+  if (InitializerType->isRecordType())
+    return visitRecordInitializer(Initializer);
+
   // Otherwise, visit the expression like normal.
   return this->Visit(Initializer);
 }
 
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::getPtrVarDecl(const VarDecl *VD, const Expr *E) {
   // Generate a pointer to the local, loading refs.
   if (Optional<unsigned> Idx = getGlobalIdx(VD)) {
bool ByteCodeExprGen<Emitter>::VisitCallExpr(const CallExpr *E) {
   } else {
     assert(false && "We don't support non-FunctionDecl callees right now.");
   }
 
   return false;
 }
 
 template <class Emitter>
+bool ByteCodeExprGen<Emitter>::VisitCXXDefaultInitExpr(
+    const CXXDefaultInitExpr *E) {
+  return this->visit(E->getExpr());
+}
+
+template <class Emitter>
 bool ByteCodeExprGen<Emitter>::VisitCXXDefaultArgExpr(
     const CXXDefaultArgExpr *E) {
   return this->visit(E->getExpr());
 }
 
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::VisitCXXBoolLiteralExpr(
     const CXXBoolLiteralExpr *E) {

clang/lib/AST/Interp/ByteCodeStmtGen.cpp

 } // namespace interp
 } // namespace clang
 
 template <class Emitter>
 bool ByteCodeStmtGen<Emitter>::visitFunc(const FunctionDecl *F) {
   // Classify the return type.
   ReturnType = this->classify(F->getReturnType());
 
-  // Set up fields and context if a constructor.
-  if (auto *MD = dyn_cast<CXXMethodDecl>(F))
-    return this->bail(MD);
-
   if (auto *Body = F->getBody())
     if (!visitStmt(Body))
       return false;
 
   // Emit a guard return to protect against a code path missing one.
   if (F->getReturnType()->isVoidType())
     return this->emitRetVoid(SourceInfo{});
   else

clang/lib/AST/Interp/Interp.h

if (!CheckThis(S, OpPC, This))
     return false;
   const Pointer &Field = This.atField(I);
   Field.deref<T>() = S.Stk.pop<T>();
   Field.activate();
   Field.initialize();
   return true;
 }
 
+/// 1) Pops the value from the stack
+/// 2) Pops a pointer from the stack
+/// 3) Writes the value to field I of the pointer
 template <PrimType Name, class T = typename PrimConv<Name>::T>
 bool InitField(InterpState &S, CodePtr OpPC, uint32_t I) {
   const T &Value = S.Stk.pop<T>();
   const Pointer &Field = S.Stk.pop<Pointer>().atField(I);
   Field.deref<T>() = Value;
   Field.activate();
   Field.initialize();
   return true;

clang/lib/AST/Interp/Record.h

public:
   const Base *getVirtualBase(const RecordDecl *RD) const;
 
   using const_field_iter = FieldList::const_iterator;
   llvm::iterator_range<const_field_iter> fields() const {
     return llvm::make_range(Fields.begin(), Fields.end());
   }
 
   unsigned getNumFields() const { return Fields.size(); }
+  const Field *getField(unsigned I) const { return &Fields[I]; }
   Field *getField(unsigned I) { return &Fields[I]; }
 
   using const_base_iter = BaseList::const_iterator;
   llvm::iterator_range<const_base_iter> bases() const {
     return llvm::make_range(Bases.begin(), Bases.end());
   }
 
   unsigned getNumBases() const { return Bases.size(); }

clang/test/AST/Interp/records.cpp

+// RUN: %clang_cc1 -fexperimental-new-constant-interpreter -verify %s
+// RUN: %clang_cc1 -verify=ref %s
+
+// expected-no-diagnostics
+// ref-no-diagnostics
+
+struct Ints {
+  int a = 20;
+  int b = 30;
+  bool c = true;
+
+  static const int five = 5;
+  static constexpr int getFive() {
+    return five;
+  }
+
+  constexpr int getTen() const {
+    return 10;
+  }
+};
+
+static_assert(Ints::getFive() == 5, "");
+
+constexpr Ints ints;
+static_assert(ints.a == 20, "");
+static_assert(ints.b == 30, "");
+static_assert(ints.c, "");
+static_assert(ints.getTen() == 10, "");
+
+constexpr Ints ints2{-20, -30, false};
+static_assert(ints2.a == -20, "");
+static_assert(ints2.b == -30, "");
+static_assert(!ints2.c, "");
+
+
+struct Ints2 {
+  int a = 10;
+  int b;
+};
+// FIXME: Broken in the new constant interpreter.
+//   Should be rejected, but without asan errors.
+//constexpr Ints2 ints2;
+

clang/test/AST/Interp/references.cpp

 struct S {
   int i, j;
 };
 
 constexpr int RefToMemberExpr() {
   S s{1, 2};
 
   int &j = s.i;
-  j += 10;
+  j = j + 10;
 
   return j;
 }
 // FIXME: Should be accepted.
-static_assert(RefToMemberExpr() == 11, ""); // expected-error {{not an integral constant expression}}
+static_assert(RefToMemberExpr() == 11, "");