Bug 107637 - [C++23] P2718R0 - Final Fix of Broken Range‐based for Loop
Summary: [C++23] P2718R0 - Final Fix of Broken Range‐based for Loop
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 13.0
: P3 normal
Target Milestone: ---
Assignee: Jakub Jelinek
URL:
Keywords:
Depends on:
Blocks: c++23-core
  Show dependency treegraph
 
Reported: 2022-11-11 14:59 UTC by Jakub Jelinek
Modified: 2024-10-21 12:35 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2022-11-14 00:00:00


Attachments
gcc15-pr107637-wip.patch (1.42 KB, patch)
2024-08-08 18:06 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jakub Jelinek 2022-11-11 14:59:42 UTC
Testcase:
// P2644R1 - Final Fix of Broken Range‐based for Loop
// { dg-do run { target c++11 } }

extern "C" void abort ();
struct S
{
  S () { ++s; }
  S (const S &) { ++s; }
  ~S () { --s; }
  static int s;
};
int S::s;
struct T
{
  T (const S &, const S &) { ++t; }
  T (const T &) { ++t; }
  ~T () { --t; }
  static int t;
};
int T::t;
int a[4];

int *
begin (const S &)
{
  return &a[0];
}

int *
end (const S &)
{
  return &a[4];
}

int *
begin (const T &)
{
  return &a[0];
}

int *
end (const T &)
{
  return &a[4];
}

const S &
foo (const S &x)
{
  return x;
}

const T &
foo (const T &x)
{
  return x;
}

int
main ()
{
  if (S::s != 0)
    abort ();
  for (auto x : S ())
    {
      if (S::s != 1)
	abort ();
    }
  if (S::s != 0)
    abort ();
  for (auto x : foo (S ()))
    {
      if (S::s != (__cpp_range_based_for >= 202211L))
	abort ();
    }
  if (S::s != 0)
    abort ();
  if (T::t != 0)
    abort ();
  for (auto x : T (S (), S ()))
    {
      if (S::s != 2 * (__cpp_range_based_for >= 202211L) || T::t != 1)
	abort ();
    }
  if (S::s != 0 || T::t != 0)
    abort ();
  for (auto x : foo (T (S (), S ())))
    {
      if (S::s != 2 * (__cpp_range_based_for >= 202211L)
	  || T::t != (__cpp_range_based_for >= 202211L))
	abort ();
    }
  if (S::s != 0 || T::t != 0)
    abort ();
}

if I understand the paper well.
Tried to play with it, but:
--- gcc/cp/decl.cc.jj	2022-11-11 08:43:28.296462815 +0100
+++ gcc/cp/decl.cc	2022-11-11 13:53:19.071246170 +0100
@@ -7809,7 +7809,14 @@ initialize_local_var (tree decl, tree in
 
 	  gcc_assert (building_stmt_list_p ());
 	  saved_stmts_are_full_exprs_p = stmts_are_full_exprs_p ();
-	  current_stmt_tree ()->stmts_are_full_exprs_p = 1;
+	  // P2644R1 - for-range-initializer in C++23 should have temporaries
+	  // destructed only at the end of the whole range for loop.
+	  if (cxx_dialect >= cxx23
+	      && DECL_ARTIFICIAL (decl)
+	      && DECL_NAME (decl) == for_range__identifier)
+	    current_stmt_tree ()->stmts_are_full_exprs_p = 0;
+	  else
+	    current_stmt_tree ()->stmts_are_full_exprs_p = 1;
 	  finish_expr_stmt (init);
 	  current_stmt_tree ()->stmts_are_full_exprs_p =
 	    saved_stmts_are_full_exprs_p;
--- gcc/cp/semantics.cc.jj	2022-11-09 11:22:42.612628127 +0100
+++ gcc/cp/semantics.cc	2022-11-11 15:49:30.569832414 +0100
@@ -1408,7 +1408,10 @@ finish_for_stmt (tree for_stmt)
 	}
     }
 
-  add_stmt (do_poplevel (scope));
+  tree bind = do_poplevel (scope);
+  if (range_for_decl[0] && cxx_dialect >= cxx23)
+    bind = maybe_cleanup_point_expr_void (bind);
+  add_stmt (bind);
 
   /* If we're being called from build_vec_init, don't mess with the names of
      the variables for an enclosing range-for.  */

ICEs, not sure why the outer CLEANUP_POINT_EXPR doesn't catch those TARGET_EXPR cleanups.
But, I think it could interact badly with the cleanups for extended lifetime references.
So shall something walk init in cp_finish_decl of for_range__identifier decls,
look similarly to wrap_cleanups init and look for cleanups on TARGET_EXPRs not nested inside of CLEANUP_POINT_EXPRs and somehow extend their lifetime (perhaps move them out of the TARGET_EXPRs just into normal cleanups)?
Giving up on this...
Comment 1 Jakub Jelinek 2022-11-14 17:46:51 UTC
I think the actual wording is in P2718R0 now, so will show up in
https://wg21.link/p2718r0 at some point.
Comment 2 Jakub Jelinek 2024-08-08 15:14:30 UTC
Adjusted testcase so that it also verifies what is destructed first, S vs. T in the various cases and standards:

// P2718R0 - Wording for P2644R1 Fix for Range-based for Loop
// { dg-do run { target c++11 } }

extern "C" void abort ();
void check (bool);

struct S
{
  S () { ++s; }
  S (const S &) { ++s; }
  ~S () { check (true); --s; }
  static int s;
};
int S::s;
struct T
{
  T (const S &, const S &) { ++t; }
  T (const T &) { ++t; }
  ~T () { check (false); --t; }
  static int t;
};
int T::t;
int a[4];
int c;

void
check (bool is_s)
{
  if (c)
    {
      if (is_s)
	{
	  if (T::t != (c == 1))
	    abort ();
	}
      else
	{
	  if (S::s != (c == 1 ? 0 : 2))
	    abort ();
	}
    }
}

int *
begin (const S &)
{
  return &a[0];
}

int *
end (const S &)
{
  return &a[4];
}

int *
begin (const T &)
{
  return &a[0];
}

int *
end (const T &)
{
  return &a[4];
}

const S &
foo (const S &x)
{
  return x;
}

const T &
foo (const T &x)
{
  return x;
}

int
main ()
{
  if (S::s != 0)
    abort ();
  for (auto x : S ())
    {
      if (S::s != 1)
	abort ();
    }
  if (S::s != 0)
    abort ();
  for (auto x : foo (S ()))
    {
      if (S::s != (__cpp_range_based_for >= 202211L))
	abort ();
    }
  if (S::s != 0)
    abort ();
  if (T::t != 0)
    abort ();
  c = 1 + (__cpp_range_based_for >= 202211L);
  for (auto x : T (S (), S ()))
    {
      if (S::s != 2 * (__cpp_range_based_for >= 202211L) || T::t != 1)
	abort ();
    }
  if (S::s != 0 || T::t != 0)
    abort ();
  c = 2;
  for (auto x : foo (T (S (), S ())))
    {
      if (S::s != 2 * (__cpp_range_based_for >= 202211L)
	  || T::t != (__cpp_range_based_for >= 202211L))
	abort ();
    }
  if (S::s != 0 || T::t != 0)
    abort ();
}
Comment 3 Jakub Jelinek 2024-08-08 18:06:24 UTC
Created attachment 58878 [details]
gcc15-pr107637-wip.patch

Completely untested WIP patch, with which the above testcase now passes also in -std=c++23 mode (well, it passed before too, but only because it had the older FTM value).
Will need to figure out what to do for OpenMP range for loops tomorrow and add more testsuite coverage.
Comment 4 GCC Commits 2024-09-24 18:25:05 UTC
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:650e91566561870f3d1c8d5b92e6613296ee1a8d

commit r15-3840-g650e91566561870f3d1c8d5b92e6613296ee1a8d
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Tue Sep 24 20:19:50 2024 +0200

    c++: Implement C++23 P2718R0 - Wording for P2644R1 Fix for Range-based for Loop [PR107637]
    
    The following patch implements the C++23 P2718R0 paper
    - Wording for P2644R1 Fix for Range-based for Loop.
    The patch introduces a new option, -f{,no-}range-for-ext-temps so that
    user can control the behavior even in older C++ versions.
    The option is on by default in C++23 and later (-fno-range-for-ext-temps
    is an error in that case) and in the -std=gnu++11 ... -std=gnu++20 modes
    (one can use -fno-range-for-ext-temps to request previous behavior in that
    case), and is not enabled by default in -std=c++11 ... -std=c++20 modes
    but one can explicitly enable it with -frange-for-ext-temps.
    As all the temporaries from __for_range initialization should have life
    extended until the end of __for_range scope, this patch disables (for
    -frange-for-ext-temps and if !processing_template_decl) CLEANUP_POINT_EXPR wrapping
    of the __for_range declaration, also disables -Wdangling-reference warning
    as well as the rest of extend_ref_init_temps (we know the __for_range temporary
    is not TREE_STATIC and as all the temporaries from the initializer will be life
    extended, we shouldn't try to handle temporaries referenced by references any
    differently) and adds an extra push_stmt_list/pop_stmt_list before
    cp_finish_decl of __for_range and after end of the for body and wraps all
    that into CLEANUP_POINT_EXPR.
    I had to repeat that also for OpenMP range loops because those are handled
    differently.
    
    2024-09-24  Jakub Jelinek  <jakub@redhat.com>
    
            PR c++/107637
    gcc/
            * omp-general.cc (find_combined_omp_for, find_nested_loop_xform):
            Handle CLEANUP_POINT_EXPR like TRY_FINALLY_EXPR.
            * doc/invoke.texi (frange-for-ext-temps): Document.  Add
            -fconcepts to the C++ option list.
    gcc/c-family/
            * c.opt (frange-for-ext-temps): New option.
            * c-opts.cc (c_common_post_options): Set flag_range_for_ext_temps
            for C++23 or later or for C++11 or later in !flag_iso mode if
            the option wasn't set by user.
            * c-cppbuiltin.cc (c_cpp_builtins): Change __cpp_range_based_for
            value for flag_range_for_ext_temps from 201603L to 202212L in C++17
            or later.
            * c-omp.cc (c_find_nested_loop_xform_r): Handle CLEANUP_POINT_EXPR
            like TRY_FINALLY_EXPR.
    gcc/cp/
            * cp-tree.h: Implement C++23 P2718R0 - Wording for P2644R1 Fix for
            Range-based for Loop.
            (cp_convert_omp_range_for): Add bool tmpl_p argument.
            (find_range_for_decls): Declare.
            * parser.cc (cp_convert_range_for): For flag_range_for_ext_temps call
            push_stmt_list () before cp_finish_decl for range_temp and save it
            temporarily to FOR_INIT_STMT.
            (cp_convert_omp_range_for): Add tmpl_p argument.  If set, remember
            DECL_NAME of range_temp and for cp_finish_decl call restore it before
            clearing it again, if unset, don't adjust DECL_NAME of range_temp at
            all.
            (cp_parser_omp_loop_nest): For flag_range_for_ext_temps range for add
            CLEANUP_POINT_EXPR around sl.  Call find_range_for_decls and adjust
            DECL_NAMEs for range fors if not processing_template_decl.  Adjust
            cp_convert_omp_range_for caller.  Remove superfluous backslash at the
            end of line.
            * decl.cc (initialize_local_var): For flag_range_for_ext_temps
            temporarily clear stmts_are_full_exprs_p rather than set for
            for_range__identifier decls.
            * call.cc (extend_ref_init_temps): For flag_range_for_ext_temps return
            init early for for_range__identifier decls.
            * semantics.cc (find_range_for_decls): New function.
            (finish_for_stmt): Use it.  For flag_range_for_ext_temps if
            cp_convert_range_for set FOR_INIT_STMT, pop_stmt_list it and wrap
            into CLEANUP_POINT_EXPR.
            * pt.cc (tsubst_omp_for_iterator): Adjust tsubst_omp_for_iterator
            caller.
            (tsubst_stmt) <case OMP_FOR>: For flag_range_for_ext_temps if there
            are any range fors in the loop nest, add push_stmt_list starting
            before the initializations, pop_stmt_list it after the body and wrap
            into CLEANUP_POINT_EXPR.  Change DECL_NAME of range for temps from
            NULL to for_range_identifier.
    gcc/testsuite/
            * g++.dg/cpp23/range-for1.C: New test.
            * g++.dg/cpp23/range-for2.C: New test.
            * g++.dg/cpp23/range-for3.C: New test.
            * g++.dg/cpp23/range-for4.C: New test.
            * g++.dg/cpp23/range-for5.C: New test.
            * g++.dg/cpp23/range-for6.C: New test.
            * g++.dg/cpp23/range-for7.C: New test.
            * g++.dg/cpp23/range-for8.C: New test.
            * g++.dg/cpp23/feat-cxx2b.C (__cpp_range_based_for): Check for
            202212L rather than 201603L.
            * g++.dg/cpp26/feat-cxx26.C (__cpp_range_based_for): Likewise.
            * g++.dg/warn/Wdangling-reference4.C: Don't expect warning for C++23
            or newer.  Use dg-additional-options rather than dg-options.
    libgomp/
            * testsuite/libgomp.c++/range-for-1.C: New test.
            * testsuite/libgomp.c++/range-for-2.C: New test.
            * testsuite/libgomp.c++/range-for-3.C: New test.
            * testsuite/libgomp.c++/range-for-4.C: New test.
            * testsuite/libgomp.c++/range-for-5.C: New test.
Comment 5 Jakub Jelinek 2024-09-24 18:26:01 UTC
Implemented for GCC 15.