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
