yronglin marked 2 inline comments as done.
yronglin 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:
----------------
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
}
```
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
