hokein added a comment.
Thanks for the review!
================
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:
> > > > 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.
oh, I thought it's just something low-hanging fruit like clarifying
comments/documentation.
Adding verifier to clang AST looks promising and ambitious (personally I like
it), but would probably require a lot of work.
================
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;
----------------
rsmith wrote:
> 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.
oops, made it consistent with `Success &=`
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
