Bug 92831 - CWG1299 extend_ref_init_temps_1 punts on COND_EXPRs
Summary: CWG1299 extend_ref_init_temps_1 punts on COND_EXPRs
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 10.0
: P3 normal
Target Milestone: ---
Assignee: Jakub Jelinek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-12-05 18:28 UTC by Jakub Jelinek
Modified: 2020-10-26 17:48 UTC (History)
6 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2019-12-05 00:00:00


Attachments
gcc10-pr92831.patch (2.47 KB, patch)
2019-12-06 11:16 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 2019-12-05 18:28:36 UTC
http://eel.is/c++draft/class.temporary#6.7
says that even ?: second and third operands need to be handled, but short testcase modified from the standard one:
template<typename T> using id = T;

int i = 1;
int&& a = id<int[3]>{1, 2, 3}[i];           // temporary array has same lifetime as a
const int& b = static_cast<const int&>(0);  // temporary int has same lifetime as b
int&& c = cond ? id<int[3]>{1, 2, 3}[i] : static_cast<int&&>(0);
                                            // exactly one of the two temporaries is lifetime-extended
like:
template<typename T> using id = T;
struct S { S () { s++; } ~S () { s--; } S (int) { s++; } static int s; };
int S::s = 0;

void
bar (bool cond)
{
  if (S::s != (cond ? 7 : 5))
    __builtin_abort ();
}

void
foo (bool cond)
{
  int i = 1;
  // temporary array has same lifetime as a
  S&& a = id<S[3]>{1, 2, 3}[i];
  // temporary S has same lifetime as b
  const S& b = static_cast<const S&>(0);
  // exactly one of the two temporaries is lifetime-extended
  S&& c = cond ? id<S[3]>{1, 2, 3}[i] : static_cast<S&&>(0);
  bar (cond);
}

int
main ()
{
  foo (true);
  foo (false);
}

fails with g++ and succeeds with clang++.
Comment 1 Jakub Jelinek 2019-12-05 18:48:56 UTC
Seems this is http://wg21.link/p0727 aka CWG 1299.
Comment 2 Jakub Jelinek 2019-12-05 20:13:03 UTC
Even better testcase that has nested COND_EXPRs:
template<typename T> using id = T;
struct S { S () { s++; } ~S () { s--; } S (int) { s++; } static int s; };
int S::s = 0;

void
bar (bool cond, bool cond2)
{
  if (S::s != (cond ? cond2 ? 7 : 5 : cond2 ? 8 : 9))
    __builtin_abort ();
}

void
foo (bool cond, bool cond2)
{
  int i = 1;
  // temporary array has same lifetime as a
  S&& a = id<S[3]>{1, 2, 3}[i];
  // temporary S has same lifetime as b
  const S& b = static_cast<const S&>(0);
  // exactly one of the four temporaries is lifetime-extended
  S&& c = cond ? cond2 ? id<S[3]>{1, 2, 3}[i] : static_cast<S&&>(0)
	       : cond2 ? id<S[4]>{1, 2, 3, 4}[i] : id<S[5]>{1, 2, 3, 4, 5}[i];
  bar (cond, cond2);
}

int
main ()
{
  foo (true, true);
  foo (true, false);
  foo (false, true);
  foo (false, false);
}

--- gcc/cp/call.c.jj	2019-12-05 10:03:04.110181312 +0100
+++ gcc/cp/call.c	2019-12-05 20:35:08.386092965 +0100
@@ -12153,12 +12153,19 @@ extend_ref_init_temps_1 (tree decl, tree
         = extend_ref_init_temps_1 (decl, TREE_OPERAND (sub, 1), cleanups);
       return init;
     }
+  if (TREE_CODE (sub) == COND_EXPR)
+    {
+      TREE_OPERAND (sub, 1)
+	= extend_ref_init_temps_1 (decl, TREE_OPERAND (sub, 1), cleanups);
+      TREE_OPERAND (sub, 2)
+	= extend_ref_init_temps_1 (decl, TREE_OPERAND (sub, 2), cleanups);
+      return init;
+    }
   if (TREE_CODE (sub) != ADDR_EXPR)
     return init;
   /* Deal with binding to a subobject.  */
   for (p = &TREE_OPERAND (sub, 0);
-       (TREE_CODE (*p) == COMPONENT_REF
-	|| TREE_CODE (*p) == ARRAY_REF); )
+       TREE_CODE (*p) == COMPONENT_REF || TREE_CODE (*p) == ARRAY_REF; )
     p = &TREE_OPERAND (*p, 0);
   if (TREE_CODE (*p) == TARGET_EXPR)
     {
isn't sufficient, because if there are any cleanups added by either of the recursive calls, we need to conditionalize those cleanups on whether the particular COND_EXPR's first operand evaluated to true or false.  I think cp_save_expr won't do it for the nested cases though, because the SAVE_EXPR might end up not being initialized.  So we need to arrange for a bool temporary that will be initialized to say false early, before the first COND_EXPR is encountered, set it to true at the start of corresponding init code and wrap cleanups with that guard.
Any better ideas?
Comment 3 Jakub Jelinek 2019-12-06 11:16:23 UTC
Created attachment 47434 [details]
gcc10-pr92831.patch

Untested fix.
Comment 4 Jakub Jelinek 2019-12-06 20:16:58 UTC
Author: jakub
Date: Fri Dec  6 20:16:27 2019
New Revision: 279064

URL: https://gcc.gnu.org/viewcvs?rev=279064&root=gcc&view=rev
Log:
	PR c++/92831 - CWG 1299, not extending temporary lifetime for ?:
	* cp-tree.h (extend_ref_init_temps): Add a new argument with NULL
	default arg.
	* call.c (set_up_extended_ref_temp): Add COND_GUARD argument, pass it
	down to extend_ref_init_temps.  Before pushing cleanup, if COND_GUARD
	is non-NULL, create a bool temporary if needed, initialize to false
	and guard the cleanup with the temporary being true.
	(extend_ref_init_temps_1): Add COND_GUARD argument, pass it down
	to recursive calls and set_up_extended_ref_temp.  Handle COND_EXPR.
	(extend_ref_init_temps): Add COND_GUARD argument, pass it down to
	recursive calls and to extend_ref_init_temps_1.

	* g++.dg/cpp0x/temp-extend2.C: New test.

Added:
    trunk/gcc/testsuite/g++.dg/cpp0x/temp-extend2.C
Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/call.c
    trunk/gcc/cp/cp-tree.h
    trunk/gcc/testsuite/ChangeLog
Comment 5 Jakub Jelinek 2019-12-06 23:44:17 UTC
Author: jakub
Date: Fri Dec  6 23:43:45 2019
New Revision: 279069

URL: https://gcc.gnu.org/viewcvs?rev=279069&root=gcc&view=rev
Log:
	PR c++/92831
	* call.c (build_conditional_expr_1): For ?: with omitted middle
	operand use cp_stabilize_reference if arg1 is glvalue_p rather than
	just if it is lvalue_p.

	* g++.dg/ext/temp-extend1.C: New test.

Added:
    trunk/gcc/testsuite/g++.dg/ext/temp-extend1.C
Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/call.c
    trunk/gcc/testsuite/ChangeLog
Comment 6 Jakub Jelinek 2019-12-07 10:04:27 UTC
Should be fixed on the trunk so far, will backport to release branches in two weeks or so.
Comment 7 Jan Hubicka 2019-12-07 14:35:36 UTC
Thank you!  I wonder if your fix can also have an optional warning which would let us to fix occurrences of this in Firefox since requiring unreleased compilers is not cool
Comment 8 Jakub Jelinek 2019-12-20 17:38:16 UTC
Author: jakub
Date: Fri Dec 20 17:37:45 2019
New Revision: 279669

URL: https://gcc.gnu.org/viewcvs?rev=279669&root=gcc&view=rev
Log:
	Backported from mainline
	2019-12-06  Jakub Jelinek  <jakub@redhat.com>

	PR c++/92831 - CWG 1299, not extending temporary lifetime for ?:
	* cp-tree.h (extend_ref_init_temps): Add a new argument with NULL
	default arg.
	* call.c (set_up_extended_ref_temp): Add COND_GUARD argument, pass it
	down to extend_ref_init_temps.  Before pushing cleanup, if COND_GUARD
	is non-NULL, create a bool temporary if needed, initialize to false
	and guard the cleanup with the temporary being true.
	(extend_ref_init_temps_1): Add COND_GUARD argument, pass it down
	to recursive calls and set_up_extended_ref_temp.  Handle COND_EXPR.
	(extend_ref_init_temps): Add COND_GUARD argument, pass it down to
	recursive calls and to extend_ref_init_temps_1.

	* g++.dg/cpp0x/temp-extend2.C: New test.

Added:
    branches/gcc-9-branch/gcc/testsuite/g++.dg/cpp0x/temp-extend2.C
Modified:
    branches/gcc-9-branch/gcc/cp/ChangeLog
    branches/gcc-9-branch/gcc/cp/call.c
    branches/gcc-9-branch/gcc/cp/cp-tree.h
    branches/gcc-9-branch/gcc/testsuite/ChangeLog
Comment 9 CVS Commits 2020-01-15 20:05:01 UTC
The master branch has been updated by Jason Merrill <jason@gcc.gnu.org>:

https://gcc.gnu.org/g:7192b1ec12484f5ca8b20930d8dc4d28ab4a533a

commit r10-5987-g7192b1ec12484f5ca8b20930d8dc4d28ab4a533a
Author: Jason Merrill <jason@redhat.com>
Date:   Wed Jan 15 09:31:11 2020 -0500

    PR c++/92871 - bad code with xvalue and GNU ?: extension.
    
    I steered Jakub wrong on the desired behavior for temp-extend1.C in the
    context of bug 92831; it doesn't make sense to try to extend the lifetime of
    a temporary that we've already materialized to evaluate the test.  So this
    patch munges the stabilized expression so that it won't be subject to
    lifetime extension.
    
    	* call.c (prevent_lifetime_extension): New.
    	(build_conditional_expr_1): Use it.
Comment 10 Marek Polacek 2020-10-26 17:33:42 UTC
Fixed?
Comment 11 Jakub Jelinek 2020-10-26 17:48:37 UTC
Not backported to 8, but I vaguely recall it was difficult to backport it and I gave up.
So let's close it as fixed in 9.3+ and the omitted second operand on ?: for 10.1+ only.