[clang] [Sema] Fix lifetime extension for temporaries in range-based for loops in C++23 (PR #145164)

[Marco Vitale via cfe-commits]

mrcvtl wrote:

> I have debug the issue, seems we cannot move the `__range` variable check to the start of `checkExprLifetimeImpl`, we may missing extending lifetime of temporaries. Eg.
> 
> ```c++
> template <typename T>
> struct ListWrapper {
>   ListWrapper() {}
>   ~ListWrapper() {}
>   const T *begin() const;
>   const T *end() const;
>   ListWrapper& r();
>   ListWrapper g();
> };
> 
> using A = ListWrapper<int>;
> 
> A g() { return A(); }
> const A &f1(const A &t) { return t; }
> 
> void member_call() {
>   // CHECK-CXX23: void @_ZN7P2718R011member_call11member_callEv()
>   // CHECK-CXX23-LABEL: for.cond.cleanup:
>   // CHECK-CXX23-NEXT: call void @_ZN7P2718R011member_call11ListWrapperIiED1Ev(
>   // CHECK-CXX23-NEXT: call void @_ZN7P2718R011member_call11ListWrapperIiED1Ev(
>   // CHECK-CXX23-NEXT: call void @_ZN7P2718R011member_call11ListWrapperIiED1Ev(
>   // CHECK-CXX23-NEXT: call void @_ZN7P2718R011member_call11ListWrapperIiED1Ev(
>   for (auto e : g().r().g().r().g().r().g()) {}
> }
> ```
> 
> I think we should revert to the previous approach, what do you think?
> 
> ```c++
>       if (IsGslPtrValueFromGslTempOwner && DiagLoc.isValid()) {
>         if (const auto *VD =
>                 dyn_cast_if_present<VarDecl>(ExtendingEntity->getDecl());
>             SemaRef.getLangOpts().CPlusPlus23 && VD &&
>             VD->isCXXForRangeImplicitVar())
>           return true;
>         SemaRef.Diag(DiagLoc, diag::warn_dangling_lifetime_pointer)
>             << DiagRange;
>         return false;
>       }
> ```

Agree, there are some side effect that I can't fully understand.

> I have debug the issue, seems we cannot move the `__range` variable check to the start of `checkExprLifetimeImpl`, we may missing extending lifetime of temporaries. Eg.
> 
> ```c++
> template <typename T>
> struct ListWrapper {
>   ListWrapper() {}
>   ~ListWrapper() {}
>   const T *begin() const;
>   const T *end() const;
>   ListWrapper& r();
>   ListWrapper g();
> };
> 
> using A = ListWrapper<int>;
> 
> A g() { return A(); }
> const A &f1(const A &t) { return t; }
> 
> void member_call() {
>   // CHECK-CXX23: void @_ZN7P2718R011member_call11member_callEv()
>   // CHECK-CXX23-LABEL: for.cond.cleanup:
>   // CHECK-CXX23-NEXT: call void @_ZN7P2718R011member_call11ListWrapperIiED1Ev(
>   // CHECK-CXX23-NEXT: call void @_ZN7P2718R011member_call11ListWrapperIiED1Ev(
>   // CHECK-CXX23-NEXT: call void @_ZN7P2718R011member_call11ListWrapperIiED1Ev(
>   // CHECK-CXX23-NEXT: call void @_ZN7P2718R011member_call11ListWrapperIiED1Ev(
>   for (auto e : g().r().g().r().g().r().g()) {}
> }
> ```
> 
> I think we should revert to the previous approach, what do you think?
> 
> ```c++
>       if (IsGslPtrValueFromGslTempOwner && DiagLoc.isValid()) {
>         if (const auto *VD =
>                 dyn_cast_if_present<VarDecl>(ExtendingEntity->getDecl());
>             SemaRef.getLangOpts().CPlusPlus23 && VD &&
>             VD->isCXXForRangeImplicitVar())
>           return true;
>         SemaRef.Diag(DiagLoc, diag::warn_dangling_lifetime_pointer)
>             << DiagRange;
>         return false;
>       }
> ```

I agree. There are so

> I have debug the issue, seems we cannot move the `__range` variable check to the start of `checkExprLifetimeImpl`, we may missing extending lifetime of temporaries. Eg.
> 
> ```c++
> template <typename T>
> struct ListWrapper {
>   ListWrapper() {}
>   ~ListWrapper() {}
>   const T *begin() const;
>   const T *end() const;
>   ListWrapper& r();
>   ListWrapper g();
> };
> 
> using A = ListWrapper<int>;
> 
> A g() { return A(); }
> const A &f1(const A &t) { return t; }
> 
> void member_call() {
>   // CHECK-CXX23: void @_ZN7P2718R011member_call11member_callEv()
>   // CHECK-CXX23-LABEL: for.cond.cleanup:
>   // CHECK-CXX23-NEXT: call void @_ZN7P2718R011member_call11ListWrapperIiED1Ev(
>   // CHECK-CXX23-NEXT: call void @_ZN7P2718R011member_call11ListWrapperIiED1Ev(
>   // CHECK-CXX23-NEXT: call void @_ZN7P2718R011member_call11ListWrapperIiED1Ev(
>   // CHECK-CXX23-NEXT: call void @_ZN7P2718R011member_call11ListWrapperIiED1Ev(
>   for (auto e : g().r().g().r().g().r().g()) {}
> }
> ```
> 
> I think we should revert to the previous approach, what do you think?
> 
> ```c++
>       if (IsGslPtrValueFromGslTempOwner && DiagLoc.isValid()) {
>         if (const auto *VD =
>                 dyn_cast_if_present<VarDecl>(ExtendingEntity->getDecl());
>             SemaRef.getLangOpts().CPlusPlus23 && VD &&
>             VD->isCXXForRangeImplicitVar())
>           return true;
>         SemaRef.Diag(DiagLoc, diag::warn_dangling_lifetime_pointer)
>             << DiagRange;
>         return false;
>       }
> ```

Thanks for debugging that! 
I agree btw, didn't think about this side effect before.

https://github.com/llvm/llvm-project/pull/145164