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

[Tyker via Phabricator via cfe-commits]

Tyker marked 10 inline comments as done.
Tyker added a comment.

update done tasks.



================
Comment at: clang/lib/AST/APValue.cpp:599
         Out << '[' << Path[I].getAsArrayIndex() << ']';
-        ElemTy = Ctx.getAsArrayType(ElemTy)->getElementType();
+        ElemTy = cast<ArrayType>(ElemTy)->getElementType();
       }
----------------
aaron.ballman wrote:
> Tyker wrote:
> > aaron.ballman wrote:
> > > Are you sure this doesn't change behavior? See the implementation of `ASTContext::getAsArrayType()`. Same question applies below.
> > i ran the test suite after the change it there wasn't any test failures. but the test on dumping APValue are probably not as thorough as we would like them to be.
> > from analysis of `ASTContext::getAsArrayType()` the only effect i see on the element type is de-sugaring and canonicalization which shouldn't affect correctness of the output.  de-sugaring requires the ASTContext but canonicalization doesn't.
> > 
> > i think the best way the have higher confidence is to ask rsmith what he thinks.
> Yeah, I doubt we have good test coverage for all the various behaviors here. I was wondering if the qualifiers bit was handled properly with a simple cast. @rsmith is a good person to weigh in.
the original question we had is whether it is correct to replace `Ctx.ASTContext::getAsArrayType(ElemTy)` by `cast<ArrayType>(ElemTy.getCanonicalType())` in this context and the other comment below.


================
Comment at: clang/lib/AST/Expr.cpp:319
   case RSK_None:
     return;
   case RSK_Int64:
----------------
rsmith wrote:
> Can you use `llvm_unreachable` here? (Are there cases where we use `RSK_None` and then later find we actually have a value to store into the `ConstantExpr`?)
we can put `llvm_unreachable` in the switch because of `if (!Value.hasValue())` above the switch but we can't remove `if (!Value.hasValue())`.
all cases i have seen where `if (!Value.hasValue())` is taken occur after a semantic error occured. 


================
Comment at: clang/lib/Serialization/ASTReader.cpp:9635
+        if (IsExpr) {
+          Base = APValue::LValueBase(ReadExpr(F), CallIndex, Version);
+          ElemTy = Base.get<const Expr *>()->getType();
----------------
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 ?


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

https://reviews.llvm.org/D63640