[PATCH] D63640: [clang] Improve Serialization/Imporing/Dumping of APValues

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Are we at a point where we can test this now? Perhaps by adding an assert in codegen that we always have an evaluated value for any `constexpr` variable that we emit?



================
Comment at: clang/lib/Sema/SemaDecl.cpp:11883-11885
   ExprResult Result =
       ActOnFinishFullExpr(Init, VDecl->getLocation(),
                           /*DiscardedValue*/ false, VDecl->isConstexpr());
----------------
This may create additional AST nodes outside the `ConstantExpr`, which `VarDecl::evaluateValue` is not expecting to see (in particular, if we have cleanups for the initializer). Should the `ConstantExpr` go outside those nodes rather than inside?


================
Comment at: clang/lib/Serialization/ASTReader.cpp:9635
+        if (IsExpr) {
+          Base = APValue::LValueBase(ReadExpr(F), CallIndex, Version);
+          ElemTy = Base.get<const Expr *>()->getType();
----------------
Tyker wrote:
> Tyker wrote:
> > rsmith wrote:
> > > This is problematic.
> > > 
> > > `ReadExpr` will read a new copy of the expression, creating a distinct object. But in the case where we reach this when deserializing (for a `MaterializeTemporaryExpr`), we need to refer to the existing `MaterializeTemporaryExpr` in the initializer of its lifetime-extending declaration. We will also need to serialize the `ASTContext`'s `MaterializedTemporaryValues` collection so that the temporaries lifetime-extended in a constant initializer get properly handled.
> > > 
> > > That all sounds very messy, so I think we should reconsider the model that we use for lifetime-extended materialized temporaries. As a half-baked idea:
> > > 
> > >  * When we lifetime-extend a temporary, create a `MaterializedTemporaryDecl` to hold its value, and modify `MaterializeTemporaryExpr` to refer to the `MaterializedTemporaryDecl` rather than to just hold the subexpression for the temporary.
> > >  * Change the `LValueBase` representation to denote the declaration rather than the expression.
> > >  * Store the constant evaluated value for a materialized temporary on the `MaterializedTemporaryDecl` rather than on a side-table in the `ASTContext`.
> > > 
> > > With that done, we should verify that all remaining `Expr*`s used as `LValueBase`s are either only transiently used during evaluation or don't have these kinds of identity problems.
> > Would it be possible to adapt serialization/deserialization so that they make sure that `MaterializeTemporaryExpr` are unique.
> > by:
> > 
> >   - When serializing `MaterializeTemporaryExpr` serialize a key obtained from the pointer to the expression as it is unique.
> >   - When deserializing `MaterializeTemporaryExpr` deserializing the key, and than have a cache for previously deserialized expression that need to be unique.
> > 
> > This would make easier adding new `Expr` that require uniqueness and seem less complicated.
> > What do you think ?
> i added a review that does the refactoring https://reviews.llvm.org/D69360.
What are the cases for which we still encounter expressions as lvalue bases during serialization? I think all the other ones should be OK, but maybe there's another interesting one we've overlooked.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63640/new/

https://reviews.llvm.org/D63640