[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

275–276

Comment is now stale

[shafik]

clang/lib/AST/Interp/Interp.cpp

1

Probably a separate NFC commit

465

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

[tbaeder]

clang/lib/AST/Interp/Interp.cpp

465

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

447

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

clang/lib/AST/Interp/Interp.h

1346–1347

Should we assert that ThisPtr is valid?

1365–1368

[tbaeder]

clang/lib/AST/Interp/Interp.cpp

447

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

clang/lib/AST/Interp/Interp.h

1346–1347

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

1346–1347

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

 /// Checks if a method is pure virtual.
 bool CheckPure(InterpState &S, CodePtr OpPC, const CXXMethodDecl *MD);
 
 /// Checks if the result is a floating-point operation is valid
 /// in the current context.
 bool CheckFloatResult(InterpState &S, CodePtr OpPC, APFloat::opStatus Status,
                       const Floating &Result);
 
+/// 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()) {
+    ThisPtr = NewFrame->getThis();
     if (!CheckInvoke(S, PC, NewFrame->getThis())) {
       return false;
     }
     // TODO: CheckCallable
   }
 
   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()) {
+      if (!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

 //===--- InterpState.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"
if ((Status & APFloat::opStatus::opInvalidOp) &&
     // There is no usefully definable result.
     S.FFDiag(E);
     return false;
   }
 
   return true;
 }
 
+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 CastFP(InterpState &S, CodePtr OpPC, const llvm::fltSemantics *Sem,
             llvm::RoundingMode RM) {
   Floating F = S.Stk.pop<Floating>();
   Floating Result = F.toSemantics(Sem, RM);
   S.Stk.push<Floating>(Result);
   return true;
 }
 

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

 //   as a function parameter.
 constexpr int inc(int a) { return a + 1; }
 constexpr int f() {
     int i;
     return inc(i);
 }
 static_assert(f());
 #endif
+
+
+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 {