r183283 - Model temporary lifetime-extension explicitly in the AST. Use this model to

[Richard Smith]

On Mon, Jul 1, 2013 at 3:53 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
> On Mon, Jul 1, 2013 at 3:42 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>>
>> On Sun, Jun 30, 2013 at 6:39 PM, Eli Friedman <eli.friedman at gmail.com>
>> wrote:
>> > On Tue, Jun 4, 2013 at 5:46 PM, Richard Smith
>> > <richard-llvm at metafoo.co.uk>
>> > wrote:
>> >>
>> >> Author: rsmith
>> >> Date: Tue Jun  4 19:46:14 2013
>> >> New Revision: 183283
>> >>
>> >> URL: http://llvm.org/viewvc/llvm-project?rev=183283&view=rev
>> >> Log:
>> >> Model temporary lifetime-extension explicitly in the AST. Use this
>> >> model
>> >> to
>> >> handle temporaries which have been lifetime-extended to static storage
>> >> duration
>> >> within constant expressions. This correctly handles nested lifetime
>> >> extension
>> >> (through reference members of aggregates in aggregate initializers) but
>> >> non-constant-expression emission hasn't yet been updated to do the
>> >> same.
>> >>
>> >> Modified:
>> >>     cfe/trunk/include/clang/AST/ASTContext.h
>> >>     cfe/trunk/include/clang/AST/Decl.h
>> >>     cfe/trunk/include/clang/AST/ExprCXX.h
>> >>     cfe/trunk/include/clang/AST/Stmt.h
>> >>     cfe/trunk/include/clang/Basic/DiagnosticASTKinds.td
>> >>     cfe/trunk/include/clang/Basic/Specifiers.h
>> >>     cfe/trunk/include/clang/Sema/Initialization.h
>> >>     cfe/trunk/lib/AST/ASTContext.cpp
>> >>     cfe/trunk/lib/AST/ASTDumper.cpp
>> >>     cfe/trunk/lib/AST/ExprConstant.cpp
>> >>     cfe/trunk/lib/CodeGen/CGExprConstant.cpp
>> >>     cfe/trunk/lib/CodeGen/CodeGenModule.cpp
>> >>     cfe/trunk/lib/CodeGen/CodeGenModule.h
>> >>     cfe/trunk/lib/Sema/SemaExpr.cpp
>> >>     cfe/trunk/lib/Sema/SemaInit.cpp
>> >>     cfe/trunk/lib/Serialization/ASTReaderStmt.cpp
>> >>     cfe/trunk/lib/Serialization/ASTWriterStmt.cpp
>> >>     cfe/trunk/test/CodeGenCXX/const-init-cxx11.cpp
>> >>     cfe/trunk/test/CodeGenCXX/const-init-cxx1y.cpp
>> >>     cfe/trunk/test/CodeGenCXX/cxx0x-initializer-stdinitializerlist.cpp
>> >>     cfe/trunk/test/SemaCXX/constant-expression-cxx11.cpp
>> >>     cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp
>> >>
>> >> Modified: cfe/trunk/include/clang/AST/ASTContext.h
>> >> URL:
>> >>
>> >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ASTContext.h?rev=183283&r1=183282&r2=183283&view=diff
>> >>
>> >>
>> >> ==============================================================================
>> >> --- cfe/trunk/include/clang/AST/ASTContext.h (original)
>> >> +++ cfe/trunk/include/clang/AST/ASTContext.h Tue Jun  4 19:46:14 2013
>> >> @@ -55,6 +55,7 @@ namespace clang {
>> >>    class ExternalASTSource;
>> >>    class ASTMutationListener;
>> >>    class IdentifierTable;
>> >> +  class MaterializeTemporaryExpr;
>> >>    class SelectorTable;
>> >>    class TargetInfo;
>> >>    class CXXABI;
>> >> @@ -163,6 +164,11 @@ class ASTContext : public RefCountedBase
>> >>    llvm::DenseMap<const FunctionDecl*, FunctionDecl*>
>> >>      ClassScopeSpecializationPattern;
>> >>
>> >> +  /// \brief Mapping from materialized temporaries with static storage
>> >> duration
>> >> +  /// that appear in constant initializers to their evaluated values.
>> >> +  llvm::DenseMap<const MaterializeTemporaryExpr*, APValue>
>> >> +    MaterializedTemporaryValues;
>> >> +
>> >>    /// \brief Representation of a "canonical" template template
>> >> parameter
>> >> that
>> >>    /// is used in canonical template names.
>> >>    class CanonicalTemplateTemplateParm : public llvm::FoldingSetNode {
>> >> @@ -2117,7 +2123,12 @@ public:
>> >>    /// \brief Used by ParmVarDecl to retrieve on the side the
>> >>    /// index of the parameter when it exceeds the size of the normal
>> >> bitfield.
>> >>    unsigned getParameterIndex(const ParmVarDecl *D) const;
>> >> -
>> >> +
>> >> +  /// \brief Get the storage for the constant value of a materialized
>> >> temporary
>> >> +  /// of static storage duration.
>> >> +  APValue *getMaterializedTemporaryValue(const
>> >> MaterializeTemporaryExpr
>> >> *E,
>> >> +                                         bool MayCreate);
>> >>
>> >
>> > Having a global table of temporary values seems really fragile given the
>> > ability to modify the values of the temporaries in C++1y.
>> > (const-init-cxx1y.cpp happens to work given the precise order we call
>> > into
>> > the constant evaluator, but could easily break if we change how we do
>> > CodeGen.)  Have you given any thought to this?
>>
>> Yes; I'm also not entirely happy with the current state of affairs.
>>
>> The best approach I've thought of to improve the situation is to
>> ensure the ASTContext storage is only used when evaluating the
>> initializer for the variable that lifetime-extends the temporary (that
>> is, refuse to evaluate the temporary otherwise, and clear out all the
>> storage for the variable's extended temporaries if the overall
>> evaluation fails). It's not possible for us to emit a reference to a
>> MaterializeTemporaryExpr from outside the variable's initializer if
>> we've not succeeded in evaluating its initializer, so we shouldn't
>> emit it with the "wrong" initializer.
>>
>> In addition, we should probably move the code that emits the constant
>> in the ASTContext storage from GetAddrOfGlobalTemporary into
>> CGExprConstant, so that we can more aggressively assert that we get
>> this right.
>>
>> This would still be a little untidy. Do you have a better suggestion?
>
>
> This might be a bit crazy, but have you considered extending APValue?
> Specifically, we could add a new kind of LValue whose base is a temporary
> with the given value.  That way, when CGExprConstant is doing evaluation,
> the value is right there, instead of messing with a global table.

We'd need all the LValues pointing at the same temporary to share the
same value; that risks misbehavior if we evaluate the initializer for
a variable multiple times for whatever reason, but I think it can be
made to work. The semantic model feels a bit strange; the initial
value for a global temporary really is a global thing, and shouldn't
depend on how you constructed the LValue referring to it.