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 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.
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
