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

[Richard Smith]

On Mon, Jul 1, 2013 at 7:34 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
> On Mon, Jul 1, 2013 at 7:05 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>>
>> 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,
>
>
> I'm not sure why this is an issue; as long as the LValue in question
> represents a reference temporary with the same mangled name and the same
> value, it doesn't really matter how many instances of it exist.

Sure, but not all LValues referring to the temporary will have the
same value during evaluation if we evaluate the initializer multiple
times and it modifies the temporary.

>> 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.
>
>
> I think that's just the nature of the C++1y model.

I'm not sure I understand what you mean. Under the C++1y model, the
value of the temporary can only change while you're evaluating the
initializer of the surrounding declaration. The value is a global
property, and doesn't depend on how you constructed the lvalue.