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
