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

[Richard Smith]

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?