[AST] Fix a crash on accessing a class without definition in constexpr function context.

[sammccall]

clang/lib/AST/ExprConstant.cpp

4319

This doesn't look all that safe - you're using a None value to indicate failure, but no current code path does that and none of the callers seem to check for failure.
(e.g. evaluateVarDecl returns true instead of false).
Presumably we're going to get a diagnostic somewhere (though it's not completely obvious to me) but can we be sure we won't assume this value has the right type somewhere down the line?

I get the feeling this is correct and I don't have enough context to understand why... how about you :-)

[hokein]

clang/lib/AST/ExprConstant.cpp

4319

I don't have a promising explanation neither.

I didn't find a better way to model failures in getDefaultInitValue. This function is used in multiple places of this file (and I'm not sure whether we should change it).

@rsmith any thoughts?

[rsmith]

clang/lib/AST/ExprConstant.cpp

4319

APValue() is a valid representation for an object of class type, representing a class object that is outside its lifetime, so I think it's OK to use this representation, if we can be sure that this only happens along error paths. (It's not ideal, though.)

If we can't be sure this happens only along error paths, then we should produce a diagnostic here. Perhaps feed an EvalInfo& and a source location into every caller of this function and produce a diagnostic if we end up querying the default-initialized value of an incomplete-but-valid class type. Or perhaps we could check that the class is complete and valid from every caller of this function instead. (I think that we guarantee that, for a valid complete class type, all transitive subobjects are of valid complete types, so checking this only once at the top level before calling into getDefaultInitValue should be enough.)

[hokein]

clang/lib/AST/ExprConstant.cpp

4319

Thanks for the suggestions.

oh, yeah, I missed that the Foo Decl is invalid, so checking the class decl is valid at every caller of getDefaultInitValue should work -- it would also fix other potential issues, looks like here we guarantee that the VarDecl is valid, but don't verify the decl which the VarDecl's type refers to is valid in all callers.

Given the fact that the VarDecl e is valid and class Foo Decl is invalid, another option to fix the crash is to invalidate this VarDecl. Should we invalidate the VarDecl if the type of the VarDecl refers to an invalid decl? My gut feeling is that it is worth keeping the VarDecl valid, so that more related AST nodes will be built (for better recovery and diagnostics), though it seems unsafe.

[rsmith]

clang/lib/AST/ExprConstant.cpp

4319

I think keeping the VarDecl valid is probably the better choice, to allow us to build downstream uses of it. Also, because variables can be redeclared, we could have something like struct A; extern A v; struct A { invalid; }; -- and we can't reasonably retroactively mark v as invalid in this case, so we can't guarantee that the type of every valid variable is itself valid. (We *could* guarantee that the type of every valid variable *definition* is valid, but that will lead to inconsistencies where defining the variable causes later behavior of references to the variable to change.)

It's really unfortunate that we don't have a good definition of what "valid" means for a variable, or really any listing of what invariants we maintain in the AST in the presence of invalid nodes. :( This is one of the things I would work on if I had time...

[hokein]

Update per Richard's comment: do the error check in getDefaultInitValue, and
modify every caller of it to support the error handling.

[hokein]

cleanup the debug code.

[hokein]

clang/lib/AST/ExprConstant.cpp

4319

I think keeping the VarDecl valid is probably the better choice, to allow us to build downstream uses of it. Also, because variables can be redeclared, we could have something like struct A; extern A v; struct A { invalid; }; -- and we can't reasonably retroactively mark v as invalid in this case, so we can't guarantee that the type of every valid variable is itself valid. (We *could* guarantee that the type of every valid variable *definition* is valid, but that will lead to inconsistencies where defining the variable causes later behavior of references to the variable to change.

yeah, that makes sense, thanks for the explanation.

I have updated the patch -- now the getDefaultInitValue() does error check. If fails, return APValue() which will only happen on error paths. Since it changes non-trivial amount of code, would be nice if you can take a look.

It's really unfortunate that we don't have a good definition of what "valid" means for a variable, or really any listing of what invariants we maintain in the AST in the presence of invalid nodes. :( This is one of the things I would work on if I had time...

that would be nice to have, and given that we have containsErrors, the meanings of them are subtle (sometimes I got confused by these concepts). Would you like me to help here? happy to help though I don't fully understand clang yet.

[rsmith]

Minor suggestions but this LGTM.

clang/lib/AST/ExprConstant.cpp

4315–4317

It would be useful to mention here that this only happens if we encounter something invalid (so that callers know they don't need to produce a nice diagnostic).

4319

Would you like me to help here? happy to help though I don't fully understand clang yet.

If you're motivated, this might be a good way to learn more about Clang :) You'll certainly discover things that no-one knows about Clang (and a fair few bugs) if you add something like the LLVM IR verifier for the Clang AST.

5783–5788

Nit: you're inconsistently using if (!get) Success = false; here and Success &= get; above and below. I'd prefer it if you expressed this the same way in all three cases in this function.

8924

This is the only place where we produce a diagnostic after getDefaultInitValue fails. It'd be more consistent to just return false here.

[hokein]

address comments.

[hokein]

Thanks for the review!

clang/lib/AST/ExprConstant.cpp

4319

oh, I thought it's just something low-hanging fruit like clarifying comments/documentation.

Adding verifier to clang AST looks promising and ambitious (personally I like it), but would probably require a lot of work.

5783–5788

oops, made it consistent with Success &=

clang/lib/AST/ExprConstant.cpp

if (FinalType->getCanonicalDecl() != TargetType->getCanonicalDecl()) {
     return false;
   }
 
   // Truncate the lvalue to the appropriate derived class.
   return CastToDerivedClass(Info, E, Result, TargetType, NewEntriesSize);
 }
 
 /// Get the value to use for a default-initialized object of type T.
-static APValue getDefaultInitValue(QualType T) {
+/// Return false if fails.
+static bool getDefaultInitValue(QualType T, APValue &Result) {
+  bool Success = true;
   if (auto *RD = T->getAsCXXRecordDecl()) {
-    if (RD->isUnion())
-      return APValue((const FieldDecl*)nullptr);
-
-    APValue Struct(APValue::UninitStruct(), RD->getNumBases(),
+    if (RD->isInvalidDecl()) {
+      Result = APValue();
+      return false;
+    }
+    if (RD->isUnion()) {
+      Result = APValue((const FieldDecl *)nullptr);
+      return true;
+    }
+    Result = APValue(APValue::UninitStruct(), RD->getNumBases(),
                      std::distance(RD->field_begin(), RD->field_end()));
 
     unsigned Index = 0;
     for (CXXRecordDecl::base_class_const_iterator I = RD->bases_begin(),
-           End = RD->bases_end(); I != End; ++I, ++Index)
-      Struct.getStructBase(Index) = getDefaultInitValue(I->getType());
+                                                  End = RD->bases_end();
+         I != End; ++I, ++Index)
+      Success &= getDefaultInitValue(I->getType(), Result.getStructBase(Index));
 
     for (const auto *I : RD->fields()) {
       if (I->isUnnamedBitfield())
         continue;
-      Struct.getStructField(I->getFieldIndex()) =
-          getDefaultInitValue(I->getType());
+      Success &= getDefaultInitValue(I->getType(),
+                                     Result.getStructField(I->getFieldIndex()));
     }
-    return Struct;
+    return Success;
   }
 
   if (auto *AT =
           dyn_cast_or_null<ConstantArrayType>(T->getAsArrayTypeUnsafe())) {
-    APValue Array(APValue::UninitArray(), 0, AT->getSize().getZExtValue());
-    if (Array.hasArrayFiller())
-      Array.getArrayFiller() = getDefaultInitValue(AT->getElementType());
-    return Array;
+    Result = APValue(APValue::UninitArray(), 0, AT->getSize().getZExtValue());
+    if (Result.hasArrayFiller())
+      Success &=
+          getDefaultInitValue(AT->getElementType(), Result.getArrayFiller());
+
+    return Success;
   }
 
-  return APValue::IndeterminateValue();
+  Result = APValue::IndeterminateValue();
+  return true;
 }
 
 namespace {
 enum EvalStmtResult {
   /// Evaluation failed.
   ESR_Failed,
   /// Hit a 'return' statement.
   ESR_Returned,
static bool EvaluateVarDecl(EvalInfo &Info, const VarDecl *VD) {
   if (!VD->hasLocalStorage())
     return true;
 
   LValue Result;
   APValue &Val =
       Info.CurrentCall->createTemporary(VD, VD->getType(), true, Result);
 
   const Expr *InitE = VD->getInit();
-  if (!InitE) {
-    Val = getDefaultInitValue(VD->getType());
-    return true;
-  }
+  if (!InitE)
+    return getDefaultInitValue(VD->getType(), Val);
 
   if (InitE->isValueDependent())
     return false;
 
   if (!EvaluateInPlace(Val, Info, Result, InitE)) {
     // Wipe out any partially-computed value, to allow tracking that this
     // evaluation failed.
     Val = APValue();

 }
 
 namespace {
 struct StartLifetimeOfUnionMemberHandler {
   EvalInfo &Info;
   const Expr *LHSExpr;
   const FieldDecl *Field;
   bool DuringInit;
-
+  bool Failed = false;
   static const AccessKinds AccessKind = AK_Assign;
 
   typedef bool result_type;
-  bool failed() { return false; }
+  bool failed() { return Failed; }
   bool found(APValue &Subobj, QualType SubobjType) {
     // We are supposed to perform no initialization but begin the lifetime of
     // the object. We interpret that as meaning to do what default
     // initialization of the object would do if all constructors involved were
     // trivial:
     //  * All base, non-variant member, and array element subobjects' lifetimes
     //    begin
     //  * No variant members' lifetimes begin
     //  * All scalar subobjects whose lifetimes begin have indeterminate values
     assert(SubobjType->isUnionType());
     if (declaresSameEntity(Subobj.getUnionField(), Field)) {
       // This union member is already active. If it's also in-lifetime, there's
       // nothing to do.
       if (Subobj.getUnionValue().hasValue())
         return true;
     } else if (DuringInit) {
       // We're currently in the process of initializing a different union
       // member.  If we carried on, that initialization would attempt to
       // store to an inactive union member, resulting in undefined behavior.
       Info.FFDiag(LHSExpr,
                   diag::note_constexpr_union_member_change_during_init);
       return false;
     }
-
-    Subobj.setUnion(Field, getDefaultInitValue(Field->getType()));
+    APValue Result;
+    Failed = !getDefaultInitValue(Field->getType(), Result);
+    Subobj.setUnion(Field, Result);
     return true;
   }
   bool found(APSInt &Value, QualType SubobjType) {
     llvm_unreachable("wrong value kind for union object");
   }
   bool found(APFloat &Value, QualType SubobjType) {
     llvm_unreachable("wrong value kind for union object");
   }
if (FieldIt == RD->field_end() ||
       assert(Indirect && "fields out of order?");
       return;
     }
 
     // Default-initialize any fields with no explicit initializer.
     for (; !declaresSameEntity(*FieldIt, FD); ++FieldIt) {
       assert(FieldIt != RD->field_end() && "missing field?");
       if (!FieldIt->isUnnamedBitfield())
-        Result.getStructField(FieldIt->getFieldIndex()) =
-            getDefaultInitValue(FieldIt->getType());
+        Success &= getDefaultInitValue(
+            FieldIt->getType(),
+            Result.getStructField(FieldIt->getFieldIndex()));
     }
     ++FieldIt;
   };
   for (const auto *I : Definition->inits()) {
     LValue Subobject = This;
     LValue SubobjectParent = This;
     APValue *Value = &Result;
 
#endif
         // preceding zero-initialization, and we're now initializing a union
         // subobject other than the first.
         // FIXME: In this case, the values of the other subobjects are
         // specified, since zero-initialization sets all padding bits to zero.
         if (!Value->hasValue() ||
             (Value->isUnion() && Value->getUnionField() != FD)) {
           if (CD->isUnion())
             *Value = APValue(FD);
-          else
-            // FIXME: This immediately starts the lifetime of all members of an
-            // anonymous struct. It would be preferable to strictly start member
-            // lifetime in initialization order.
-            *Value = getDefaultInitValue(Info.Ctx.getRecordType(CD));
+          else if (!getDefaultInitValue(Info.Ctx.getRecordType(CD), *Value))
+            // FIXME: This immediately starts the lifetime of all members of
+            // an anonymous struct. It would be preferable to strictly start
+            // member lifetime in initialization order.
+            Success = false;
         }
         // Store Subobject as its parent before updating it for the last element
         // in the chain.
         if (C == IndirectFieldChain.back())
           SubobjectParent = Subobject;
         if (!HandleLValueMember(Info, I->getInit(), Subobject, FD))
           return false;
         if (CD->isUnion())
           Value = &Value->getUnionValue();
#endif
     if (I->isBaseInitializer() && BasesSeen == RD->getNumBases())
       EvalObj.finishedConstructingBases();
   }
 
   // Default-initialize any remaining fields.
   if (!RD->isUnion()) {
     for (; FieldIt != RD->field_end(); ++FieldIt) {
       if (!FieldIt->isUnnamedBitfield())
-        Result.getStructField(FieldIt->getFieldIndex()) =
-            getDefaultInitValue(FieldIt->getType());
+        Success &= getDefaultInitValue(
+            FieldIt->getType(),
+            Result.getStructField(FieldIt->getFieldIndex()));
     }
   }
 
   EvalObj.finishedConstructingFields();
 
   return Success &&
          EvaluateStmt(Ret, Info, Definition->getBody()) != ESR_Failed &&
          LifetimeExtendedScope.destroy();
bool PointerExprEvaluator::VisitCXXNewExpr(const CXXNewExpr *E) {
   } else if (ResizedArrayCCE) {
     if (!EvaluateArrayNewConstructExpr(Info, Result, *Val, ResizedArrayCCE,
                                        AllocType))
       return false;
   } else if (Init) {
     if (!EvaluateInPlace(*Val, Info, Result, Init))
       return false;
   } else {
-    *Val = getDefaultInitValue(AllocType);
+    // should we return false here?
+    llvm::errs() << "hit!";
+    if (!getDefaultInitValue(AllocType, *Val))
+      return Error(E);
   }
 
   // Array new returns a pointer to the first element, not a pointer to the
   // array.
   if (auto *AT = AllocType->getAsArrayTypeUnsafe())
     Result.addArray(Info, E, cast<ConstantArrayType>(AT));
 
   return true;
bool RecordExprEvaluator::VisitCXXConstructExpr(const CXXConstructExpr *E,
   if (CheckTrivialDefaultConstructor(Info, E->getExprLoc(), FD, ZeroInit)) {
     // If we've already performed zero-initialization, we're already done.
     if (Result.hasValue())
       return true;
 
     if (ZeroInit)
       return ZeroInitialization(E, T);
 
-    Result = getDefaultInitValue(T);
-    return true;
+    return getDefaultInitValue(T, Result);
   }
 
   const FunctionDecl *Definition = nullptr;
   auto Body = FD->getBody(Definition);
 
   if (!CheckConstexprFunction(Info, E->getExprLoc(), FD, Definition, Body))
     return false;
 

 bool VarDecl::evaluateDestruction(
     SmallVectorImpl<PartialDiagnosticAt> &Notes) const {
   Expr::EvalStatus EStatus;
   EStatus.Diag = &Notes;
 
   // Make a copy of the value for the destructor to mutate, if we know it.
   // Otherwise, treat the value as default-initialized; if the destructor works
   // anyway, then the destruction is constant (and must be essentially empty).
-  APValue DestroyedValue =
-      (getEvaluatedValue() && !getEvaluatedValue()->isAbsent())
-          ? *getEvaluatedValue()
-          : getDefaultInitValue(getType());
+  APValue DestroyedValue;
+  if (getEvaluatedValue() && !getEvaluatedValue()->isAbsent())
+    DestroyedValue = *getEvaluatedValue();
+  else if (!getDefaultInitValue(getType(), DestroyedValue))
+    return false;
 
   EvalInfo Info(getASTContext(), EStatus, EvalInfo::EM_ConstantExpression);
   Info.setEvaluatingDecl(this, DestroyedValue,
                          EvalInfo::EvaluatingDeclKind::Dtor);
   Info.InConstantContext = true;
 
   SourceLocation DeclLoc = getLocation();
   QualType DeclTy = getType();

clang/test/SemaCXX/constexpr-default-init-value-crash.cpp

+// RUN: %clang_cc1 %s -std=c++20 -fsyntax-only -verify
+// RUN: %clang_cc1 %s -std=c++20 -fsyntax-only -verify -fno-recovery-ast
+
+namespace NoCrash {
+struct ForwardDecl; // expected-note {{forward declaration of}}
+struct Foo {        // expected-note 2{{candidate constructor}}
+  ForwardDecl f;    // expected-error {{field has incomplete type}}
+};
+
+constexpr Foo getFoo() {
+  Foo e = 123; // expected-error {{no viable conversion from 'int' to 'NoCrash::Foo'}}
+  return e;
+}
+}