This is an archive of the discontinued LLVM Phabricator instance.

[Clang][C++23] Lifetime extension in range-based for loops
AbandonedPublic

Authored by cor3ntin on Dec 7 2022, 2:54 PM.

Details

Reviewers
erichkeane
hubert.reinterpretcast
rZhBoYao
Group Reviewers
Restricted Project
Summary

Implemented the C++23 paper P2718R0

Diff Detail

Event Timeline

rZhBoYao created this revision.Dec 7 2022, 2:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2022, 2:54 PM
rZhBoYao requested review of this revision.Dec 7 2022, 2:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2022, 2:54 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Consider:

struct T {
  const int *begin() const;
  const int *end()   const;
  T &r() [[clang::lifetimebound]];
  T t();
};

const T &f1(const T &t [[clang::lifetimebound]]) { return t; }
T g();

void foo() {
  for (auto e : f1(g())) {}
  for (auto e : g().t().t().r()) {}
}

The first __range1:

| | `-VarDecl 0x11e901358 <col:17, col:23> col:17 implicit used __range1 'const T &' cinit
| |   `-ExprWithCleanups 0x11e9015b8 <col:17, col:23> 'const T':'const T' lvalue
| |     `-CallExpr 0x11e9010b0 <col:17, col:23> 'const T':'const T' lvalue
| |       |-ImplicitCastExpr 0x11e901098 <col:17> 'const T &(*)(const T &)' <FunctionToPointerDecay>
| |       | `-DeclRefExpr 0x11e901018 <col:17> 'const T &(const T &)' lvalue Function 0x11e900a48 'f1' 'const T &(const T &)'
| |       `-MaterializeTemporaryExpr 0x11e9010f0 <col:20, col:22> 'const T':'const T' lvalue extended by Var 0x11e901358 '__range1' 'const T &'
| |         `-ImplicitCastExpr 0x11e9010d8 <col:20, col:22> 'const T':'const T' <NoOp>
| |           `-CallExpr 0x11e900ec0 <col:20, col:22> 'T':'T'
| |             `-ImplicitCastExpr 0x11e900ea8 <col:20> 'T (*)()' <FunctionToPointerDecay>
| |               `-DeclRefExpr 0x11e900e30 <col:20> 'T ()' lvalue Function 0x11e900bd0 'g' 'T ()'

g() is extended by __range1.

The second __range1:

| `-VarDecl 0x11e9040a0 <col:17, col:31> col:17 implicit used __range1 'T &' cinit
|   `-ExprWithCleanups 0x11e904338 <col:17, col:31> 'T':'T' lvalue
|     `-CXXMemberCallExpr 0x11e903f68 <col:17, col:31> 'T':'T' lvalue
|       `-MemberExpr 0x11e903f38 <col:17, col:29> '<bound member function type>' .r 0x11e900638
|         `-MaterializeTemporaryExpr 0x11e903f20 <col:17, col:27> 'T':'T' xvalue extended by Var 0x11e9040a0 '__range1' 'T &'
|           `-CXXMemberCallExpr 0x11e903f00 <col:17, col:27> 'T':'T'
|             `-MemberExpr 0x11e903ed0 <col:17, col:25> '<bound member function type>' .t 0x11e900780
|               `-MaterializeTemporaryExpr 0x11e903eb8 <col:17, col:23> 'T':'T' xvalue extended by Var 0x11e9040a0 '__range1' 'T &'
|                 `-CXXMemberCallExpr 0x11e903e68 <col:17, col:23> 'T':'T'
|                   `-MemberExpr 0x11e903e38 <col:17, col:21> '<bound member function type>' .t 0x11e900780
|                     `-MaterializeTemporaryExpr 0x11e903e20 <col:17, col:19> 'T':'T' xvalue extended by Var 0x11e9040a0 '__range1' 'T &'
|                       `-CallExpr 0x11e903e00 <col:17, col:19> 'T':'T'
|                         `-ImplicitCastExpr 0x11e903de8 <col:17> 'T (*)()' <FunctionToPointerDecay>
|                           `-DeclRefExpr 0x11e903dc8 <col:17> 'T ()' lvalue Function 0x11e900bd0 'g' 'T ()'

g() and 2 t()'s are extended by __range1.

Thanks a lot for working on this.

I will try to do a more detailed review later, but at least I think we might want more tests. Maybe codegen tests that do not rely on [[clang::lifetimebound]], and tests with more chaining (a().b().c()) .

Thanks for picking this up! :)

The (non-wording) paper makes a pretty convincing case to just apply this retroactively to any C++11 code (https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2644r1.pdf). I think we should apply this retroactively, maybe add a pedantic warning when we do a lifetime extension on code before C++23.

Also, +1 to more tests. Specifically constexpr and dependent contexts would be good to see.

The (non-wording) paper makes a pretty convincing case to just apply this retroactively to any C++11 code (https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2644r1.pdf). I think we should apply this retroactively, maybe add a pedantic warning when we do a lifetime extension on code before C++23.

Just noting that the committee did not vote this in as a Defect Report, but I mostly agree that people should code for the new behaviour and that the old behaviour is unlikely to be relied on. I suspect this should be highlighted as technically a breaking change.

I will try to do a more detailed review later, but at least I think we might want more tests. Maybe codegen tests that do not rely on [[clang::lifetimebound]], and tests with more chaining (a().b().c()) .

We need to cover AST structure more than chaining. For example the patch currently does not find the B() temporary:

struct A { ~A(); int x[3]; };
struct B : A {};
int (&f(const A *))[3];
const A *g(const A &);
void bar(int);
void foo() {
  for (auto e : f(g(B()))) {
    bar(e);
  }
}

At least on the surface, it looks like there will be a lot of trouble to deal with default arguments:

struct A { A(); ~A(); int x[3]; };
int (&f(const A & = A()))[3];
void bar(int);
void foo() {
  for (auto e : f()) { bar(e); }
}

We get a CXXDefaultArgExpr, and the expression on the parameter declaration is what has the MaterializeTemporaryExpr. It looks like we now have cases where different call sites require different semantics (and perhaps different diagnostics, somewhat like template instantiations).

At least on the surface, it looks like there will be a lot of trouble to deal with default arguments:

struct A { A(); ~A(); int x[3]; };
int (&f(const A & = A()))[3];
void bar(int);
void foo() {
  for (auto e : f()) { bar(e); }
}

We get a CXXDefaultArgExpr, and the expression on the parameter declaration is what has the MaterializeTemporaryExpr. It looks like we now have cases where different call sites require different semantics (and perhaps different diagnostics, somewhat like template instantiations).

This PR https://reviews.llvm.org/D136554 adds some machinery to rewrite CXXDefaultArgExpr when they are used, so we could, if needed, reuse that machinery to inject MaterializeTemporaryExpr (as part of D136554 the rewrite only happen in the presence of immediate function calls)

hubert.reinterpretcast requested changes to this revision.Feb 1 2023, 8:16 PM

Waiting on changes to fix the visitation to locate the appropriate temporaries.

This revision now requires changes to proceed.Feb 1 2023, 8:16 PM

@rZhBoYao are you still working on this? Thanks!

rZhBoYao abandoned this revision.May 15 2023, 9:48 AM

@rZhBoYao are you still working on this? Thanks!

No. Sorry that I don't have the capacity to work on this.

@rZhBoYao are you still working on this? Thanks!

No. Sorry that I don't have the capacity to work on this.

Thanks a lot for letting us know and getting this started. We will try to find someone to take over. Cheers!

cor3ntin commandeered this revision.May 23 2023, 12:49 AM
cor3ntin reclaimed this revision.
cor3ntin added a reviewer: rZhBoYao.
This revision now requires changes to proceed.May 23 2023, 12:49 AM

@hubert.reinterpretcast I'll try to look at that but unless I'm mistaken, the wording excludes function parameters

The fourth context is when a temporary object other than a function parameter object is created in the for-range-initializer of a range-based for statement. If such a temporary object would otherwise be destroyed at the end of the for-range-initializer full-expression, the object persists for the lifetime of the reference initialized by the for-range-initializer.

I think that invalidates some of your examples?

FYI, I'm not actively working on this,
I did mess around with it a bit https://github.com/cor3ntin/llvm-project/commit/478ff7f1aa7a4046fa8b293dfb86489b930a8888
but I'm extremely unfamiliar with that part of clang and i have no clue how to write tests.
Feel free to take over!

FYI, I'm not actively working on this,
I did mess around with it a bit https://github.com/cor3ntin/llvm-project/commit/478ff7f1aa7a4046fa8b293dfb86489b930a8888
but I'm extremely unfamiliar with that part of clang and i have no clue how to write tests.
Feel free to take over!

FYI, I'm not actively working on this,
I did mess around with it a bit https://github.com/cor3ntin/llvm-project/commit/478ff7f1aa7a4046fa8b293dfb86489b930a8888
but I'm extremely unfamiliar with that part of clang and i have no clue how to write tests.
Feel free to take over!

Thanks @cor3ntin ! If you are no bandwidth to working on D139586, I'd be happy to do take over.

cor3ntin abandoned this revision.Jul 6 2023, 12:45 PM

The right way to do that is outline here https://reviews.llvm.org/D153701#4478240
There is no point in trying to keep this attempt around.

@hubert.reinterpretcast I'll try to look at that but unless I'm mistaken, the wording excludes function parameters

The fourth context is when a temporary object other than a function parameter object is created in the for-range-initializer of a range-based for statement. If such a temporary object would otherwise be destroyed at the end of the for-range-initializer full-expression, the object persists for the lifetime of the reference initialized by the for-range-initializer.

I think that invalidates some of your examples?

@cor3ntin, the examples I gave were for temporary objects created for binding to a reference (that, in turn, happens to be a function parameter). Those temporary objects are not function parameters.