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

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9fb7e98db5aa: [AST] Fix a crash on accessing a class without 
definition in constexpr function… (authored by hokein).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80981/new/

https://reviews.llvm.org/D80981

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/constexpr-default-init-value-crash.cpp

Index: clang/test/SemaCXX/constexpr-default-init-value-crash.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/constexpr-default-init-value-crash.cpp
@@ -0,0 +1,14 @@
+// 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;
+}
+}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -4475,37 +4475,48 @@
 }
 
 /// Get the value to use for a default-initialized object of type T.
-static APValue getDefaultInitValue(QualType T) {
+/// Return false if it encounters something invalid.
+static bool getDefaultInitValue(QualType T, APValue ) {
+  bool Success = true;
   if (auto *RD = T->getAsCXXRecordDecl()) {
-if (RD->isUnion())
-  return APValue((const FieldDecl*)nullptr);
-
-APValue Struct(APValue::UninitStruct(), RD->getNumBases(),
-   std::distance(RD->field_begin(), RD->field_end()));
+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(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 {
@@ -4535,10 +4546,8 @@
   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;
@@ -5535,11 +5544,11 @@
   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 , 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
@@ -5563,8 +5572,9 @@
   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 , QualType SubobjType) {
@@ -5880,8 +5890,9 @@
 for (; !declaresSameEntity(*FieldIt, FD); ++FieldIt) {
   assert(FieldIt != RD->field_end() && "missing 

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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for the review!




Comment at: clang/lib/AST/ExprConstant.cpp:4320
+if (!RD->hasDefinition())
+  return APValue();
 APValue Struct(APValue::UninitStruct(), RD->getNumBases(),

rsmith wrote:
> hokein wrote:
> > rsmith wrote:
> > > hokein wrote:
> > > > rsmith wrote:
> > > > > hokein wrote:
> > > > > > sammccall wrote:
> > > > > > > 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 :-)
> > > > > > 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?
> > > > > `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.)
> > > > 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. 
> > > 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...
> > > 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 

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

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 273281.
hokein marked 5 inline comments as done.
hokein added a comment.

address comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80981/new/

https://reviews.llvm.org/D80981

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/constexpr-default-init-value-crash.cpp

Index: clang/test/SemaCXX/constexpr-default-init-value-crash.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/constexpr-default-init-value-crash.cpp
@@ -0,0 +1,14 @@
+// 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;
+}
+}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -4475,37 +4475,48 @@
 }
 
 /// Get the value to use for a default-initialized object of type T.
-static APValue getDefaultInitValue(QualType T) {
+/// Return false if it encounters something invalid.
+static bool getDefaultInitValue(QualType T, APValue ) {
+  bool Success = true;
   if (auto *RD = T->getAsCXXRecordDecl()) {
-if (RD->isUnion())
-  return APValue((const FieldDecl*)nullptr);
-
-APValue Struct(APValue::UninitStruct(), RD->getNumBases(),
-   std::distance(RD->field_begin(), RD->field_end()));
+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(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 {
@@ -4535,10 +4546,8 @@
   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;
@@ -5535,11 +5544,11 @@
   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 , 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
@@ -5563,8 +5572,9 @@
   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 , QualType SubobjType) {
@@ -5880,8 +5890,9 @@
 for (; !declaresSameEntity(*FieldIt, FD); ++FieldIt) {
   assert(FieldIt != RD->field_end() && "missing field?");
   if (!FieldIt->isUnnamedBitfield())
-

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith marked an inline comment as done.
rsmith added a comment.
This revision is now accepted and ready to land.

Minor suggestions but this LGTM.




Comment at: clang/lib/AST/ExprConstant.cpp:4320
+if (!RD->hasDefinition())
+  return APValue();
 APValue Struct(APValue::UninitStruct(), RD->getNumBases(),

hokein wrote:
> rsmith wrote:
> > hokein wrote:
> > > rsmith wrote:
> > > > hokein wrote:
> > > > > sammccall wrote:
> > > > > > 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 :-)
> > > > > 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?
> > > > `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.)
> > > 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. 
> > 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...
> > 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 

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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:4320
+if (!RD->hasDefinition())
+  return APValue();
 APValue Struct(APValue::UninitStruct(), RD->getNumBases(),

rsmith wrote:
> hokein wrote:
> > rsmith wrote:
> > > hokein wrote:
> > > > sammccall wrote:
> > > > > 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 :-)
> > > > 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?
> > > `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.)
> > 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. 
> 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...
> 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 

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

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 272018.
hokein marked an inline comment as done.
hokein added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80981/new/

https://reviews.llvm.org/D80981

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/constexpr-default-init-value-crash.cpp

Index: clang/test/SemaCXX/constexpr-default-init-value-crash.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/constexpr-default-init-value-crash.cpp
@@ -0,0 +1,14 @@
+// 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;
+}
+}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -4312,37 +4312,48 @@
 }
 
 /// 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 ) {
+  bool Success = true;
   if (auto *RD = T->getAsCXXRecordDecl()) {
-if (RD->isUnion())
-  return APValue((const FieldDecl*)nullptr);
-
-APValue Struct(APValue::UninitStruct(), RD->getNumBases(),
-   std::distance(RD->field_begin(), RD->field_end()));
+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(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 {
@@ -4372,10 +4383,8 @@
   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;
@@ -5372,11 +5381,11 @@
   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 , 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
@@ -5400,8 +5409,9 @@
   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 , QualType SubobjType) {
@@ -5717,8 +5727,9 @@
 for (; !declaresSameEntity(*FieldIt, FD); ++FieldIt) {
   assert(FieldIt != RD->field_end() && 

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

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 272019.
hokein added a comment.

cleanup the debug code.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80981/new/

https://reviews.llvm.org/D80981

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/constexpr-default-init-value-crash.cpp

Index: clang/test/SemaCXX/constexpr-default-init-value-crash.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/constexpr-default-init-value-crash.cpp
@@ -0,0 +1,14 @@
+// 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;
+}
+}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -4312,37 +4312,48 @@
 }
 
 /// 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 ) {
+  bool Success = true;
   if (auto *RD = T->getAsCXXRecordDecl()) {
-if (RD->isUnion())
-  return APValue((const FieldDecl*)nullptr);
-
-APValue Struct(APValue::UninitStruct(), RD->getNumBases(),
-   std::distance(RD->field_begin(), RD->field_end()));
+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(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 {
@@ -4372,10 +4383,8 @@
   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;
@@ -5372,11 +5381,11 @@
   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 , 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
@@ -5400,8 +5409,9 @@
   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 , QualType SubobjType) {
@@ -5717,8 +5727,9 @@
 for (; !declaresSameEntity(*FieldIt, FD); ++FieldIt) {
   assert(FieldIt != RD->field_end() && "missing field?");
   if (!FieldIt->isUnnamedBitfield())
-Result.getStructField(FieldIt->getFieldIndex()) =
-

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:4320
+if (!RD->hasDefinition())
+  return APValue();
 APValue Struct(APValue::UninitStruct(), RD->getNumBases(),

hokein wrote:
> rsmith wrote:
> > hokein wrote:
> > > sammccall wrote:
> > > > 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 :-)
> > > 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?
> > `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.)
> 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. 
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...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80981/new/

https://reviews.llvm.org/D80981



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Haojian Wu via Phabricator via cfe-commits]

hokein marked an inline comment as done.
hokein added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:4320
+if (!RD->hasDefinition())
+  return APValue();
 APValue Struct(APValue::UninitStruct(), RD->getNumBases(),

rsmith wrote:
> hokein wrote:
> > sammccall wrote:
> > > 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 :-)
> > 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?
> `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.)
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. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80981/new/

https://reviews.llvm.org/D80981



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:4320
+if (!RD->hasDefinition())
+  return APValue();
 APValue Struct(APValue::UninitStruct(), RD->getNumBases(),

hokein wrote:
> sammccall wrote:
> > 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 :-)
> 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?
`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.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80981/new/

https://reviews.llvm.org/D80981



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Haojian Wu via Phabricator via cfe-commits]

hokein marked an inline comment as done.
hokein added a subscriber: rsmith.
hokein added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:4320
+if (!RD->hasDefinition())
+  return APValue();
 APValue Struct(APValue::UninitStruct(), RD->getNumBases(),

sammccall wrote:
> 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 :-)
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80981/new/

https://reviews.llvm.org/D80981



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:4320
+if (!RD->hasDefinition())
+  return APValue();
 APValue Struct(APValue::UninitStruct(), RD->getNumBases(),

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 :-)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80981/new/

https://reviews.llvm.org/D80981



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: sammccall.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D80981

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/constant-expression-cxx2a.cpp


Index: clang/test/SemaCXX/constant-expression-cxx2a.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx2a.cpp
+++ clang/test/SemaCXX/constant-expression-cxx2a.cpp
@@ -1401,3 +1401,15 @@
   //   decreasing address
   static_assert(f(6) == 543210);
 }
+
+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;
+}
+}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -4316,7 +4316,8 @@
   if (auto *RD = T->getAsCXXRecordDecl()) {
 if (RD->isUnion())
   return APValue((const FieldDecl*)nullptr);
-
+if (!RD->hasDefinition())
+  return APValue();
 APValue Struct(APValue::UninitStruct(), RD->getNumBases(),
std::distance(RD->field_begin(), RD->field_end()));
 


Index: clang/test/SemaCXX/constant-expression-cxx2a.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx2a.cpp
+++ clang/test/SemaCXX/constant-expression-cxx2a.cpp
@@ -1401,3 +1401,15 @@
   //   decreasing address
   static_assert(f(6) == 543210);
 }
+
+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;
+}
+}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -4316,7 +4316,8 @@
   if (auto *RD = T->getAsCXXRecordDecl()) {
 if (RD->isUnion())
   return APValue((const FieldDecl*)nullptr);
-
+if (!RD->hasDefinition())
+  return APValue();
 APValue Struct(APValue::UninitStruct(), RD->getNumBases(),
std::distance(RD->field_begin(), RD->field_end()));
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits