[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