hubert.reinterpretcast added inline comments.
================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:8901-8914
+ // [P2718R0] Lifetime extension in range-based for loops.
+ //
+ // 6.7.7 [class.temporary] p5:
+ // There are four contexts in which temporaries are destroyed at a different
+ // point than the end of the full-expression.
+ //
+ // 6.7.7 [class.temporary] p6:
----------------
yronglin wrote:
> hubert.reinterpretcast wrote:
> > yronglin wrote:
> > > hubert.reinterpretcast wrote:
> > > > yronglin wrote:
> > > > > yronglin wrote:
> > > > > > hubert.reinterpretcast wrote:
> > > > > > > yronglin wrote:
> > > > > > > > rsmith wrote:
> > > > > > > > > This isn't the right way to model the behavior here -- the
> > > > > > > > > presence or absence of an `ExprWithCleanups` is just a
> > > > > > > > > convenience to tell consumers of the AST whether they should
> > > > > > > > > expect to see cleanups later or not, and doesn't carry an
> > > > > > > > > implication of affecting the actual temporary lifetimes and
> > > > > > > > > storage durations.
> > > > > > > > >
> > > > > > > > > The outcome that we should be aiming to reach is that all
> > > > > > > > > `MaterializeTemporaryExpr`s created as part of processing the
> > > > > > > > > for-range-initializer are marked as being lifetime-extended
> > > > > > > > > by the for-range variable. Probably the simplest way to
> > > > > > > > > handle that would be to track the current enclosing
> > > > > > > > > for-range-initializer variable in the
> > > > > > > > > `ExpressionEvaluationContextRecord`, and whenever a
> > > > > > > > > `MaterializeTemporaryExpr` is created, if there is a current
> > > > > > > > > enclosing for-range-initializer, mark that
> > > > > > > > > `MaterializeTemporaryExpr` as being lifetime-extended by it.
> > > > > > > > Awesome! Thanks a lot for your advice, this is very helpful! I
> > > > > > > > want to take a longer look at it.
> > > > > > > As mentioned in D139586, `CXXDefaultArgExpr`s may need additional
> > > > > > > handling. Similarly for `CXXDefaultInitExpr`s.
> > > > > > Thanks for your tips! I have a question that what's the correct way
> > > > > > to extent the lifetime of `CXXBindTemporaryExpr`? Can I just
> > > > > > `materialize` the temporary? It may replaced by
> > > > > > `MaterializeTemporaryExpr`, and then I can mark it as being
> > > > > > lifetime-extended by the for-range variable.
> > > > > Eg.
> > > > > ```
> > > > > void f() {
> > > > > int v[] = {42, 17, 13};
> > > > > Mutex m;
> > > > > for (int x : static_cast<void>(LockGuard(m)), v) // lock released
> > > > > in C++ 2020
> > > > > {
> > > > > LockGuard guard(m); // OK in C++ 2020, now deadlocks
> > > > > }
> > > > > }
> > > > > ```
> > > > > ```
> > > > > BinaryOperator 0x135036220 'int[3]' lvalue ','
> > > > > |-CXXStaticCastExpr 0x1350361d0 'void' static_cast<void> <ToVoid>
> > > > > | `-CXXFunctionalCastExpr 0x135036198 'LockGuard':'struct LockGuard'
> > > > > functional cast to LockGuard <ConstructorConversion>
> > > > > | `-CXXBindTemporaryExpr 0x135036178 'LockGuard':'struct LockGuard'
> > > > > (CXXTemporary 0x135036178)
> > > > > | `-CXXConstructExpr 0x135036140 'LockGuard':'struct LockGuard'
> > > > > 'void (Mutex &)'
> > > > > | `-DeclRefExpr 0x135035e18 'Mutex':'class Mutex' lvalue Var
> > > > > 0x135035b40 'm' 'Mutex':'class Mutex'
> > > > > `-DeclRefExpr 0x135036200 'int[3]' lvalue Var 0x135035928 'v' 'int[3]'
> > > > > ```
> > > > If `MaterializeTemporaryExpr` represents a "temporary materialization
> > > > conversion", then the above should already have one just under the
> > > > `static_cast` to `void` (since the cast operand would be a
> > > > discarded-value expression).
> > > >
> > > > There may be unfortunate effects from materializing temporaries for
> > > > discarded-value expressions though: Technically, temporaries are also
> > > > created for objects having scalar type.
> > > >
> > > > Currently, `MaterializeTemporaryExpr` is documented as being tied to
> > > > reference binding, but that is not correct: for example,
> > > > `MaterializeTemporaryExpr` also appears when a member access is made on
> > > > a temporary of class type.
> > > http://eel.is/c++draft/class.temporary says:
> > > ```
> > > [Note 3: Temporary objects are materialized:
> > > .......
> > > (2.6)
> > > when a prvalue that has type other than cv void appears as a
> > > discarded-value expression ([expr.context]).
> > > — end note]
> > > ```
> > > Seems we should materialized the discard-value expression in this case,
> > > WDYT?
> > I think we should, but what is the codegen fallout? Would no-opt builds
> > start writing `42` into allocated memory for `static_cast<void>(42)`?
> Thanks for your confirm @hubert.reinterpretcast !
>
> I have tried locally, the generated IR of `void f()` is:
> ```
> define void @f()() {
> entry:
> %v = alloca [3 x i32], align 4
> %m = alloca %class.Mutex, align 8
> %__range1 = alloca ptr, align 8
> %ref.tmp = alloca %struct.LockGuard, align 8
> %__begin1 = alloca ptr, align 8
> %__end1 = alloca ptr, align 8
> %x = alloca i32, align 4
> %guard = alloca %struct.LockGuard, align 8
> call void @llvm.memcpy.p0.p0.i64(ptr align 4 %v, ptr align 4
> @__const.f().v, i64 12, i1 false)
> %call = call noundef ptr @Mutex::Mutex()(ptr noundef nonnull align 8
> dereferenceable(64) %m)
> %call1 = call noundef ptr @LockGuard::LockGuard(Mutex&)(ptr noundef nonnull
> align 8 dereferenceable(8) %ref.tmp, ptr noundef nonnull align 8
> dereferenceable(64) %m)
> store ptr %v, ptr %__range1, align 8
> %0 = load ptr, ptr %__range1, align 8
> %arraydecay = getelementptr inbounds [3 x i32], ptr %0, i64 0, i64 0
> store ptr %arraydecay, ptr %__begin1, align 8
> %1 = load ptr, ptr %__range1, align 8
> %arraydecay2 = getelementptr inbounds [3 x i32], ptr %1, i64 0, i64 0
> %add.ptr = getelementptr inbounds i32, ptr %arraydecay2, i64 3
> store ptr %add.ptr, ptr %__end1, align 8
> br label %for.cond
>
> for.cond: ; preds = %for.inc, %entry
> %2 = load ptr, ptr %__begin1, align 8
> %3 = load ptr, ptr %__end1, align 8
> %cmp = icmp ne ptr %2, %3
> br i1 %cmp, label %for.body, label %for.cond.cleanup
>
> for.cond.cleanup: ; preds = %for.cond
> %call5 = call noundef ptr @LockGuard::~LockGuard()(ptr noundef nonnull
> align 8 dereferenceable(8) %ref.tmp) #6
> br label %for.end
>
> for.body: ; preds = %for.cond
> %4 = load ptr, ptr %__begin1, align 8
> %5 = load i32, ptr %4, align 4
> store i32 %5, ptr %x, align 4
> %call3 = call noundef ptr @LockGuard::LockGuard(Mutex&)(ptr noundef nonnull
> align 8 dereferenceable(8) %guard, ptr noundef nonnull align 8
> dereferenceable(64) %m)
> %call4 = call noundef ptr @LockGuard::~LockGuard()(ptr noundef nonnull
> align 8 dereferenceable(8) %guard) #6
> br label %for.inc
>
> for.inc: ; preds = %for.body
> %6 = load ptr, ptr %__begin1, align 8
> %incdec.ptr = getelementptr inbounds i32, ptr %6, i32 1
> store ptr %incdec.ptr, ptr %__begin1, align 8
> br label %for.cond
>
> for.end: ; preds = %for.cond.cleanup
> %call6 = call noundef ptr @Mutex::~Mutex()(ptr noundef nonnull align 8
> dereferenceable(64) %m) #6
> ret void
> }
> ```
>
> The AST of `void f()` is:
> ```
> |-FunctionDecl 0x14311d408 <line:62:1, line:69:1> line:62:6 used f 'void ()'
> | `-CompoundStmt 0x143808898 <col:10, line:69:1>
> | |-DeclStmt 0x14311d710 <line:63:3, col:25>
> | | `-VarDecl 0x14311d528 <col:3, col:24> col:7 used v 'int[3]' cinit
> | | `-InitListExpr 0x14311d648 <col:13, col:24> 'int[3]'
> | | |-IntegerLiteral 0x14311d590 <col:14> 'int' 42
> | | |-IntegerLiteral 0x14311d5b0 <col:18> 'int' 17
> | | `-IntegerLiteral 0x14311d5d0 <col:22> 'int' 13
> | |-DeclStmt 0x14311d9f0 <line:64:3, col:10>
> | | `-VarDecl 0x14311d740 <col:3, col:9> col:9 used m 'Mutex':'Mutex'
> callinit destroyed
> | | `-CXXConstructExpr 0x14311d9b0 <col:9> 'Mutex':'Mutex' 'void ()'
> | `-CXXForRangeStmt 0x143808700 <line:65:3, line:68:3>
> | |-<<<NULL>>>
> | |-DeclStmt 0x14311e2c0 <line:65:16>
> | | `-VarDecl 0x14311dfa8 <col:16, col:49> col:16 implicit used __range1
> 'int (&)[3]' cinit
> | | `-ExprWithCleanups 0x14311e238 <col:16, col:49> 'int[3]' lvalue
> | | `-BinaryOperator 0x14311de38 <col:16, col:49> 'int[3]' lvalue ','
> | | |-CXXStaticCastExpr 0x14311dde8 <col:16, col:46> 'void'
> static_cast<void> <ToVoid>
> | | | `-MaterializeTemporaryExpr 0x14311ddd0 <col:34, col:45>
> 'LockGuard':'LockGuard' xvalue extended by Var 0x14311dfa8 '__range1' 'int
> (&)[3]'
> | | | `-CXXFunctionalCastExpr 0x14311dd98 <col:34, col:45>
> 'LockGuard':'LockGuard' functional cast to LockGuard <ConstructorConversion>
> | | | `-CXXBindTemporaryExpr 0x14311dd78 <col:34, col:45>
> 'LockGuard':'LockGuard' (CXXTemporary 0x14311dd78)
> | | | `-CXXConstructExpr 0x14311dd40 <col:34, col:45>
> 'LockGuard':'LockGuard' 'void (Mutex &)'
> | | | `-DeclRefExpr 0x14311da18 <col:44> 'Mutex':'Mutex'
> lvalue Var 0x14311d740 'm' 'Mutex':'Mutex'
> | | `-DeclRefExpr 0x14311de18 <col:49> 'int[3]' lvalue Var
> 0x14311d528 'v' 'int[3]'
> | |-DeclStmt 0x143808588 <col:14>
> | | `-VarDecl 0x14311e360 <col:14> col:14 implicit used __begin1 'int
> *':'int *' cinit
> | | `-ImplicitCastExpr 0x143808400 <col:14> 'int *'
> <ArrayToPointerDecay>
> | | `-DeclRefExpr 0x14311e2d8 <col:14> 'int[3]' lvalue Var
> 0x14311dfa8 '__range1' 'int (&)[3]'
> | |-DeclStmt 0x1438085a0 <col:14>
> | | `-VarDecl 0x143808248 <col:14, col:16> col:14 implicit used __end1
> 'int *':'int *' cinit
> | | `-BinaryOperator 0x143808468 <col:14, col:16> 'int *' '+'
> | | |-ImplicitCastExpr 0x143808450 <col:14> 'int *'
> <ArrayToPointerDecay>
> | | | `-DeclRefExpr 0x14311e2f8 <col:14> 'int[3]' lvalue Var
> 0x14311dfa8 '__range1' 'int (&)[3]'
> | | `-IntegerLiteral 0x143808430 <col:16> 'long' 3
> | |-BinaryOperator 0x143808628 <col:14> 'bool' '!='
> | | |-ImplicitCastExpr 0x1438085f8 <col:14> 'int *':'int *'
> <LValueToRValue>
> | | | `-DeclRefExpr 0x1438085b8 <col:14> 'int *':'int *' lvalue Var
> 0x14311e360 '__begin1' 'int *':'int *'
> | | `-ImplicitCastExpr 0x143808610 <col:14> 'int *':'int *'
> <LValueToRValue>
> | | `-DeclRefExpr 0x1438085d8 <col:14> 'int *':'int *' lvalue Var
> 0x143808248 '__end1' 'int *':'int *'
> | |-UnaryOperator 0x143808668 <col:14> 'int *':'int *' lvalue prefix '++'
> | | `-DeclRefExpr 0x143808648 <col:14> 'int *':'int *' lvalue Var
> 0x14311e360 '__begin1' 'int *':'int *'
> | |-DeclStmt 0x14311dee0 <col:8, col:50>
> | | `-VarDecl 0x14311de78 <col:8, col:14> col:12 x 'int' cinit
> | | `-ImplicitCastExpr 0x1438086d0 <col:14> 'int' <LValueToRValue>
> | | `-UnaryOperator 0x1438086b8 <col:14> 'int' lvalue prefix '*'
> cannot overflow
> | | `-ImplicitCastExpr 0x1438086a0 <col:14> 'int *':'int *'
> <LValueToRValue>
> | | `-DeclRefExpr 0x143808680 <col:14> 'int *':'int *' lvalue Var
> 0x14311e360 '__begin1' 'int *':'int *'
> | `-CompoundStmt 0x143808880 <line:66:3, line:68:3>
> | `-DeclStmt 0x143808868 <line:67:5, col:23>
> | `-VarDecl 0x143808778 <col:5, col:22> col:15 guard
> 'LockGuard':'LockGuard' callinit destroyed
> | `-CXXConstructExpr 0x143808820 <col:15, col:22>
> 'LockGuard':'LockGuard' 'void (Mutex &)'
> | `-DeclRefExpr 0x1438087e0 <col:21> 'Mutex':'Mutex' lvalue Var
> 0x14311d740 'm' 'Mutex':'Mutex'
> ```
>
> > Would no-opt builds start writing 42 into allocated memory for
> > static_cast<void>(42)?
> I have got this result with my locally build clang
>
> ```
> define noundef i32 @main() {
> entry:
> %retval = alloca i32, align 4
> %ref.tmp = alloca i32, align 4
> store i32 0, ptr %retval, align 4
> store i32 42, ptr %ref.tmp, align 4
> call void @f()()
> ret i32 0
> }
> ```
> > Would no-opt builds start writing 42 into allocated memory for
> > static_cast<void>(42)?
> I have got this result with my locally build clang
>
> ```
> define noundef i32 @main() {
> entry:
> %retval = alloca i32, align 4
> %ref.tmp = alloca i32, align 4
> store i32 0, ptr %retval, align 4
> store i32 42, ptr %ref.tmp, align 4
> call void @f()()
> ret i32 0
> }
> ```
@rsmith, in terms of the AST, do you have advice on the approach we should take
in generating `MaterializeTemporaryExpr`s for discarded-value expressions? Do
we go with a language-specification purest direction and generate these for all
prvalue object-typed discarded-value expressions? Do we limit generating them
to range-for initializers? Do we further specialize and limit to non-scalar
types or, even more specifically, to cases involving constructors or
destructors?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153701/new/
https://reviews.llvm.org/D153701
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
