[clang][Interp] Check that constructor calls initialize all record fields

[tbaeder]

Summary

We need to do this, even for global variables (it seems).

The diagnostic output is slightly different than what we get from the current interpreter, but I'm not sure if it's worse or not or if that needs to be changed.

[aaron.ballman]

It's worth noting that there are core issues in this area: https://wg21.link/cwg2558 and https://wg21.link/cwg2536 and they're not yet resolved.

clang/test/AST/Interp/records.cpp

297–298

Comment is now stale

[shafik]

clang/lib/AST/Interp/Interp.cpp

1

Probably a separate NFC commit

431

Should we also check isLive() or should that just turn into a diagnostic?

[tbaeder]

clang/lib/AST/Interp/Interp.cpp

431

Since cc79ddb52c310be50d2ed0e0307b695cc7c142ce, we use CheckInvoke() for the instance pointer, which already calls isLive().

[tbaeder]

Ping

[tbaeder]

Ping

[tbaeder]

Ping

[tbaeder]

Ping

[aaron.ballman]

clang/lib/AST/Interp/Interp.cpp

413

Do we have to worry about static data members, or are those excluded in fields()?

clang/lib/AST/Interp/Interp.h

1264–1265

Should we assert that ThisPtr is valid?

1285–1288

[tbaeder]

clang/lib/AST/Interp/Interp.cpp

413

They aren't in RecordDecl::fields(), so aren't in Record::fields() either.

clang/lib/AST/Interp/Interp.h

1264–1265

We could only assert !isZero() as far as I understand, but that's part of what CheckInvoke does.

[aaron.ballman]

LGTM!

clang/lib/AST/Interp/Interp.h

1264–1265

Ah, then no need.

[tbaeder]

Oh I think this patch is doing too much. The checking should only occur for global variables.

[shafik]

LGTM

clang/lib/AST/Interp/Interp.cpp

1

One more time :-)

clang/lib/AST/Interp/Descriptor.cpp

 
 static void ctorRecord(Block *B, char *Ptr, bool IsConst, bool IsMutable,
                        bool IsActive, Descriptor *D) {
   const bool IsUnion = D->ElemRecord->isUnion();
   auto CtorSub = [=](unsigned SubOff, Descriptor *F, bool IsBase) {
     auto *Desc = reinterpret_cast<InlineDescriptor *>(Ptr + SubOff) - 1;
     Desc->Offset = SubOff;
     Desc->Desc = F;
-    Desc->IsInitialized = (B->isStatic() || F->IsArray) && !IsBase;
+    Desc->IsInitialized = F->IsArray && !IsBase;
     Desc->IsBase = IsBase;
     Desc->IsActive = IsActive && !IsUnion;
     Desc->IsConst = IsConst || F->IsConst;
     Desc->IsMutable = IsMutable || F->IsMutable;
     if (auto Fn = F->CtorFn)
       Fn(B, Ptr + SubOff, Desc->IsConst, Desc->IsMutable, Desc->IsActive, F);
   };
   for (const auto &B : D->ElemRecord->bases())

clang/lib/AST/Interp/Interp.h

 bool CheckCallable(InterpState &S, CodePtr OpPC, const Function *F);
 
 /// Checks the 'this' pointer.
 bool CheckThis(InterpState &S, CodePtr OpPC, const Pointer &This);
 
 /// Checks if a method is pure virtual.
 bool CheckPure(InterpState &S, CodePtr OpPC, const CXXMethodDecl *MD);
 
+/// Checks that all fields are initialized after a constructor call.
+bool CheckCtorCall(InterpState &S, CodePtr OpPC, const Pointer &This);
+
 /// Checks if Div/Rem operation on LHS and RHS is valid.
 template <typename T>
 bool CheckDivRem(InterpState &S, CodePtr OpPC, const T &LHS, const T &RHS) {
   if (RHS.isZero()) {
     const SourceInfo &Loc = S.Current->getSource(OpPC);
     S.FFDiag(Loc, diag::note_expr_divide_by_zero);
     return false;
   }

 inline bool ExpandPtr(InterpState &S, CodePtr OpPC) {
   const Pointer &Ptr = S.Stk.pop<Pointer>();
   S.Stk.push<Pointer>(Ptr.expand());
   return true;
 }
 
 inline bool Call(InterpState &S, CodePtr &PC, const Function *Func) {
   auto NewFrame = std::make_unique<InterpFrame>(S, Func, PC);
+  Pointer ThisPtr;
   if (Func->hasThisPointer()) {
-    if (!CheckInvoke(S, PC, NewFrame->getThis())) {
+    ThisPtr = NewFrame->getThis();
+    if (!CheckInvoke(S, PC, ThisPtr)) {
       return false;
     }
   }
 
   if (!CheckCallable(S, PC, Func))
     return false;
 
   InterpFrame *FrameBefore = S.Current;
   S.Current = NewFrame.get();
 
   APValue CallResult;
   // Note that we cannot assert(CallResult.hasValue()) here since
   // Ret() above only sets the APValue if the curent frame doesn't
   // have a caller set.
   if (Interpret(S, CallResult)) {
     NewFrame.release(); // Frame was delete'd already.
     assert(S.Current == FrameBefore);
+
+    // For constructors, check that all fields have been initialized.
+    if (Func->isConstructor() && !CheckCtorCall(S, PC, ThisPtr))
+      return false;
+
     return true;
   }
 
   // Interpreting the function failed somehow. Reset to
   // previous state.
   S.Current = FrameBefore;
   return false;
 }
 

clang/lib/AST/Interp/Interp.cpp

 //===------- Interp.cpp - Interpreter for the constexpr VM ------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Interp.h"
bool CheckPure(InterpState &S, CodePtr OpPC, const CXXMethodDecl *MD) {
   if (!MD->isPure())
     return true;
   const SourceInfo &E = S.Current->getSource(OpPC);
   S.FFDiag(E, diag::note_constexpr_pure_virtual_call, 1) << MD;
   S.Note(MD->getLocation(), diag::note_declared_at);
   return false;
 }
 
+static bool CheckFieldsInitialized(InterpState &S, CodePtr OpPC,
+                                   const Pointer &BasePtr, const Record *R) {
+  assert(R);
+  bool Result = true;
+  // Check all fields of this record are initialized.
+  for (const Record::Field &F : R->fields()) {
+    Pointer FieldPtr = BasePtr.atField(F.Offset);
+    QualType FieldType = FieldPtr.getType();
+
+    if (FieldType->isRecordType()) {
+      Result &= CheckFieldsInitialized(S, OpPC, FieldPtr, FieldPtr.getRecord());
+    } else if (FieldType->isArrayType()) {
+      // FIXME: Arrays need to be handled here as well I think.
+    } else if (!FieldPtr.isInitialized()) {
+      const SourceInfo &SI = S.Current->getSource(OpPC);
+      S.FFDiag(SI, diag::note_constexpr_uninitialized)
+          << true << F.Decl->getType();
+      SourceLocation SubobjectLoc = F.Decl->getLocation();
+      if (SubobjectLoc.isValid())
+        S.Note(SubobjectLoc, diag::note_constexpr_subobject_declared_here);
+      Result = false;
+    }
+  }
+  return Result;
+}
+
+bool CheckCtorCall(InterpState &S, CodePtr OpPC, const Pointer &This) {
+  assert(!This.isZero());
+  const Record *R = This.getRecord();
+  return CheckFieldsInitialized(S, OpPC, This, R);
+}
+
 bool Interpret(InterpState &S, APValue &Result) {
   // The current stack frame when we started Interpret().
   // This is being used by the ops to determine wheter
   // to return from this function and thus terminate
   // interpretation.
   const InterpFrame *StartFrame = S.Current;
   assert(!S.Current->isRoot());
   CodePtr PC = S.Current->getPC();

clang/lib/AST/Interp/InterpFrame.cpp

if (Func->hasThisPointer()) {
     if (Func->hasRVO())
       This = stackRef<Pointer>(sizeof(Pointer));
     else
       This = stackRef<Pointer>(0);
   }
 }
 
 InterpFrame::~InterpFrame() {
-  if (Func && Func->isConstructor() && This.isBaseClass())
-    This.initialize();
   for (auto &Param : Params)
     S.deallocate(reinterpret_cast<Block *>(Param.second.get()));
 }
 
 void InterpFrame::destroy(unsigned Idx) {
   for (auto &Local : Func->getScope(Idx).locals()) {
     S.deallocate(reinterpret_cast<Block *>(localBlock(Local.Offset)));
   }

clang/test/AST/Interp/cxx20.cpp

return *a; // expected-note {{read of object outside its lifetime}} \
              // ref-note {{read of uninitialized object is not allowed in a constant expression}}
 }
 static_assert(initializedLocal2() == 20); // expected-error {{not an integral constant expression}} \
                                           // expected-note {{in call to}} \
                                           // ref-error {{not an integral constant expression}} \
                                           // ref-note {{in call to}}
 
 
-struct Int { int a; };
+struct Int { int a; }; // expected-note {{subobject declared here}}
 constexpr int initializedLocal3() {
-  Int i;
-  return i.a; // expected-note {{read of object outside its lifetime}} \
-              // ref-note {{read of uninitialized object is not allowed in a constant expression}}
+  Int i; // expected-note {{subobject of type 'int' is not initialized}}
+  return i.a; // ref-note {{read of uninitialized object is not allowed in a constant expression}}
 }
 static_assert(initializedLocal3() == 20); // expected-error {{not an integral constant expression}} \
                                           // expected-note {{in call to}} \
                                           // ref-error {{not an integral constant expression}} \
                                           // ref-note {{in call to}}
 
 
 

 
 constexpr auto b3 = name1() == name1();
 static_assert(b3);
 constexpr auto b4 = name1() == name2(); // ref-error {{must be initialized by a constant expression}} \
                                         // ref-note {{has unspecified value}} \
                                         // ref-note {{declared here}}
 static_assert(!b4); // ref-error {{not an integral constant expression}} \
                     // ref-note {{not a constant expression}}
+
+namespace UninitializedFields {
+  class A {
+  public:
+    int a; // expected-note {{subobject declared here}} \
+           // ref-note {{subobject declared here}}
+    constexpr A() {}
+  };
+  constexpr A a; // expected-error {{must be initialized by a constant expression}} \
+                 // expected-note {{subobject of type 'int' is not initialized}} \
+                 // ref-error {{must be initialized by a constant expression}} \
+                 // ref-note {{subobject of type 'int' is not initialized}}
+
+
+  class Base {
+  public:
+    bool b;
+    int a; // expected-note {{subobject declared here}} \
+           // ref-note {{subobject declared here}}
+    constexpr Base() : b(true) {}
+  };
+
+  class Derived : public Base {
+  public:
+    constexpr Derived() : Base() {} // expected-note {{subobject of type 'int' is not initialized}}
+  };
+
+constexpr Derived D; // expected-error {{must be initialized by a constant expression}} \\
+                     // expected-note {{in call to 'Derived()'}} \
+                     // ref-error {{must be initialized by a constant expression}} \
+                     // ref-note {{subobject of type 'int' is not initialized}}
+};

clang/test/AST/Interp/records.cpp

namespace DeriveFailures {
   };
 
   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}}
+                                 // ref-note {{initializer of 'D' is not a constant expression}} \
+                                 // expected-error {{not an integral constant expression}} \
+                                 // expected-note {{read of object outside its lifetime}}
 
   struct AnotherBase {
     int Val;
     constexpr AnotherBase(int i) : Val(12 / i) {} //ref-note {{division by zero}} \
                                                   //expected-note {{division by zero}}
   };
 
   struct AnotherDerived : AnotherBase {