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 `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.
>
> Would you like me to help here? happy to help though I don't fully understand
> clang yet.
If you're motivated, this might be a good way to learn more about Clang :)
You'll certainly discover things that no-one knows about Clang (and a fair few
bugs) if you add something like the LLVM IR verifier for the Clang AST.
================
Comment at: clang/lib/AST/ExprConstant.cpp:4315
/// 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 &Result) {
----------------
It would be useful to mention here that this only happens if we encounter
something invalid (so that callers know they don't need to produce a nice
diagnostic).
================
Comment at: clang/lib/AST/ExprConstant.cpp:5783-5787
+ else if (!getDefaultInitValue(Info.Ctx.getRecordType(CD), *Value))
+ // FIXME: This immediately starts the lifetime of all members of
+ // an anonymous struct. It would be preferable to strictly start
+ // member lifetime in initialization order.
+ Success = false;
----------------
Nit: you're inconsistently using `if (!get) Success = false;` here and `Success
&= get;` above and below. I'd prefer it if you expressed this the same way in
all three cases in this function.
================
Comment at: clang/lib/AST/ExprConstant.cpp:8923
+ } else if (!getDefaultInitValue(AllocType, *Val)) {
+ return Error(E);
}
----------------
This is the only place where we produce a diagnostic after
`getDefaultInitValue` fails. It'd be more consistent to just `return false`
here.
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
