[PATCH] D153701: [WIP][Clang] Implement P2718R0 "Lifetime extension in range-based for loops"
Hubert Tong via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Jul 13 15:21:46 PDT 2023
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
More information about the cfe-commits
mailing list