* [C++ PATCH] CWG 1299, not extending temporary lifetime for ?: (PR c++/92831)
@ 2019-12-06 16:28 Jakub Jelinek
2019-12-06 19:32 ` Jason Merrill
0 siblings, 1 reply; 3+ messages in thread
From: Jakub Jelinek @ 2019-12-06 16:28 UTC (permalink / raw)
To: Jason Merrill; +Cc: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 10137 bytes --]
Hi!
This is a reason latest firefox is miscompiled by G++ 9, seems DR 1299
says in [class.temporary]/(6.7) that reference binding to a temporary
object from the second or third operand of a conditional expression
should have extended lifetime too, but extend_ref_init_temps_1 was
apparently handling only comma expressions, some casts, . (COMPONENT_REF),
[] (ARRAY_REF), but not COND_EXPR.
The following patch handles COND_EXPR in there too, in a similar way how
the gimplifier handles the cleanups of the expressions in the COND_EXPR
operand if there is no lifetime extension due to reference binding.
In particular there are bool temporaries added, initialized to false and set
to true if the particular (leaf) COND_EXPR argument has been invoked and the
corresponding cleanup guarded that way.
I admit I haven't tried to construct testcases for all the things CWG 1299
added, e.g. don't know if ( expression ) will work, if all the *_cast cases
that need to be extended work (some of them do, as the testcase shows), or
if e.g. .* works, so I'm not claiming the DR is completely implemented.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk
and after a while for release branches?
Note, just to double check g++ doesn't ICE on expr1 ? : expr2 GNU extension,
I also wrote the attached testcase, but unlike the testcase included in the
patch which behaves the same with patched g++ as does recent clang++, the
testcase with expr1 ? : expr2 (omitted second operand) actually never
extends the lifetime of any temporary (that is the baz function), unlike
clang++ which extends the lifetime of expr2 if expr1 is false, and doesn't
extend lifetime of anything if expr1 is true. This is outside of the scope
of the standard, so no idea what is correct, so I'm just asking how it
should behave. extend_ref_init_temps_1 is never called in that case when
the expression is COND_EXPR.
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.
--- gcc/cp/cp-tree.h.jj 2019-12-05 10:03:04.111181297 +0100
+++ gcc/cp/cp-tree.h 2019-12-06 11:00:43.230257916 +0100
@@ -6321,7 +6321,9 @@ extern tree convert_for_arg_passing (tr
extern bool is_properly_derived_from (tree, tree);
extern tree initialize_reference (tree, tree, int,
tsubst_flags_t);
-extern tree extend_ref_init_temps (tree, tree, vec<tree, va_gc>**);
+extern tree extend_ref_init_temps (tree, tree,
+ vec<tree, va_gc>**,
+ tree * = NULL);
extern tree make_temporary_var_for_ref_to_temp (tree, tree);
extern bool type_has_extended_temps (tree);
extern tree strip_top_quals (tree);
--- gcc/cp/call.c.jj 2019-12-06 00:40:29.439856520 +0100
+++ gcc/cp/call.c 2019-12-06 11:00:17.111660971 +0100
@@ -11965,7 +11965,7 @@ make_temporary_var_for_ref_to_temp (tree
static tree
set_up_extended_ref_temp (tree decl, tree expr, vec<tree, va_gc> **cleanups,
- tree *initp)
+ tree *initp, tree *cond_guard)
{
tree init;
tree type;
@@ -11996,7 +11996,8 @@ set_up_extended_ref_temp (tree decl, tre
/* Recursively extend temps in this initializer. */
TARGET_EXPR_INITIAL (expr)
- = extend_ref_init_temps (decl, TARGET_EXPR_INITIAL (expr), cleanups);
+ = extend_ref_init_temps (decl, TARGET_EXPR_INITIAL (expr), cleanups,
+ cond_guard);
/* Any reference temp has a non-trivial initializer. */
DECL_NONTRIVIALLY_INITIALIZED_P (var) = true;
@@ -12037,7 +12038,24 @@ set_up_extended_ref_temp (tree decl, tre
{
tree cleanup = cxx_maybe_build_cleanup (var, tf_warning_or_error);
if (cleanup)
- vec_safe_push (*cleanups, cleanup);
+ {
+ if (cond_guard && cleanup != error_mark_node)
+ {
+ if (*cond_guard == NULL_TREE)
+ {
+ *cond_guard = build_local_temp (boolean_type_node);
+ add_decl_expr (*cond_guard);
+ tree set = cp_build_modify_expr (UNKNOWN_LOCATION,
+ *cond_guard, NOP_EXPR,
+ boolean_false_node,
+ tf_warning_or_error);
+ finish_expr_stmt (set);
+ }
+ cleanup = build3 (COND_EXPR, void_type_node,
+ *cond_guard, cleanup, NULL_TREE);
+ }
+ vec_safe_push (*cleanups, cleanup);
+ }
}
/* We must be careful to destroy the temporary only
@@ -12142,7 +12160,8 @@ initialize_reference (tree type, tree ex
which is bound either to a reference or a std::initializer_list. */
static tree
-extend_ref_init_temps_1 (tree decl, tree init, vec<tree, va_gc> **cleanups)
+extend_ref_init_temps_1 (tree decl, tree init, vec<tree, va_gc> **cleanups,
+ tree *cond_guard)
{
tree sub = init;
tree *p;
@@ -12150,20 +12169,52 @@ extend_ref_init_temps_1 (tree decl, tree
if (TREE_CODE (sub) == COMPOUND_EXPR)
{
TREE_OPERAND (sub, 1)
- = extend_ref_init_temps_1 (decl, TREE_OPERAND (sub, 1), cleanups);
+ = extend_ref_init_temps_1 (decl, TREE_OPERAND (sub, 1), cleanups,
+ cond_guard);
+ return init;
+ }
+ if (TREE_CODE (sub) == COND_EXPR)
+ {
+ tree cur_cond_guard = NULL_TREE;
+ if (TREE_OPERAND (sub, 1))
+ TREE_OPERAND (sub, 1)
+ = extend_ref_init_temps_1 (decl, TREE_OPERAND (sub, 1), cleanups,
+ &cur_cond_guard);
+ if (cur_cond_guard)
+ {
+ tree set = cp_build_modify_expr (UNKNOWN_LOCATION, cur_cond_guard,
+ NOP_EXPR, boolean_true_node,
+ tf_warning_or_error);
+ TREE_OPERAND (sub, 1)
+ = cp_build_compound_expr (set, TREE_OPERAND (sub, 1),
+ tf_warning_or_error);
+ }
+ cur_cond_guard = NULL_TREE;
+ if (TREE_OPERAND (sub, 2))
+ TREE_OPERAND (sub, 2)
+ = extend_ref_init_temps_1 (decl, TREE_OPERAND (sub, 2), cleanups,
+ &cur_cond_guard);
+ if (cur_cond_guard)
+ {
+ tree set = cp_build_modify_expr (UNKNOWN_LOCATION, cur_cond_guard,
+ NOP_EXPR, boolean_true_node,
+ tf_warning_or_error);
+ TREE_OPERAND (sub, 2)
+ = cp_build_compound_expr (set, TREE_OPERAND (sub, 2),
+ tf_warning_or_error);
+ }
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)
{
tree subinit = NULL_TREE;
- *p = set_up_extended_ref_temp (decl, *p, cleanups, &subinit);
+ *p = set_up_extended_ref_temp (decl, *p, cleanups, &subinit, cond_guard);
recompute_tree_invariant_for_addr_expr (sub);
if (init != sub)
init = fold_convert (TREE_TYPE (init), sub);
@@ -12178,13 +12229,14 @@ extend_ref_init_temps_1 (tree decl, tree
lifetime to match that of DECL. */
tree
-extend_ref_init_temps (tree decl, tree init, vec<tree, va_gc> **cleanups)
+extend_ref_init_temps (tree decl, tree init, vec<tree, va_gc> **cleanups,
+ tree *cond_guard)
{
tree type = TREE_TYPE (init);
if (processing_template_decl)
return init;
if (TYPE_REF_P (type))
- init = extend_ref_init_temps_1 (decl, init, cleanups);
+ init = extend_ref_init_temps_1 (decl, init, cleanups, cond_guard);
else
{
tree ctor = init;
@@ -12203,7 +12255,8 @@ extend_ref_init_temps (tree decl, tree i
/* The temporary array underlying a std::initializer_list
is handled like a reference temporary. */
tree array = CONSTRUCTOR_ELT (ctor, 0)->value;
- array = extend_ref_init_temps_1 (decl, array, cleanups);
+ array = extend_ref_init_temps_1 (decl, array, cleanups,
+ cond_guard);
CONSTRUCTOR_ELT (ctor, 0)->value = array;
}
else
@@ -12212,7 +12265,8 @@ extend_ref_init_temps (tree decl, tree i
constructor_elt *p;
vec<constructor_elt, va_gc> *elts = CONSTRUCTOR_ELTS (ctor);
FOR_EACH_VEC_SAFE_ELT (elts, i, p)
- p->value = extend_ref_init_temps (decl, p->value, cleanups);
+ p->value = extend_ref_init_temps (decl, p->value, cleanups,
+ cond_guard);
}
recompute_constructor_flags (ctor);
if (decl_maybe_constant_var_p (decl) && TREE_CONSTANT (ctor))
--- gcc/testsuite/g++.dg/cpp0x/temp-extend2.C.jj 2019-12-06 11:08:09.694368207 +0100
+++ gcc/testsuite/g++.dg/cpp0x/temp-extend2.C 2019-12-06 11:10:58.040768156 +0100
@@ -0,0 +1,36 @@
+// PR c++/92831
+// { dg-do run { target c++11 } }
+
+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);
+}
Jakub
[-- Attachment #2: cond.C --]
[-- Type: text/plain, Size: 676 bytes --]
template<typename T> using id = T;
struct S { S () : s (false) { ++c; } S (bool x) : s (x) { ++c; } ~S () { --c; }; bool s; static int c; };
int S::c = 0;
extern "C" int printf (const char *, ...);
void
bar ()
{
printf ("%d\n", S::c);
}
void
foo (int i)
{
const bool&& a = id<S[3]>{false, true, false}[i].s ? id<S[2]>{true, false}[i].s : id<S[4]>{true, false, true, false}[i].s;
bar ();
}
void
baz (int i)
{
const bool&& a = id<S[3]>{false, true, false}[i].s ? : id<S[4]>{true, false, true, false}[i].s;
bar ();
}
int
main ()
{
bar ();
foo (0);
bar ();
foo (1);
bar ();
baz (0);
bar ();
baz (1);
bar ();
}
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [C++ PATCH] CWG 1299, not extending temporary lifetime for ?: (PR c++/92831)
2019-12-06 16:28 [C++ PATCH] CWG 1299, not extending temporary lifetime for ?: (PR c++/92831) Jakub Jelinek
@ 2019-12-06 19:32 ` Jason Merrill
2019-12-06 23:46 ` [committed] Fix up xvalue handling in ?: with omitted middle operand " Jakub Jelinek
0 siblings, 1 reply; 3+ messages in thread
From: Jason Merrill @ 2019-12-06 19:32 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
On 12/6/19 11:28 AM, Jakub Jelinek wrote:
> Hi!
>
> This is a reason latest firefox is miscompiled by G++ 9, seems DR 1299
> says in [class.temporary]/(6.7) that reference binding to a temporary
> object from the second or third operand of a conditional expression
> should have extended lifetime too, but extend_ref_init_temps_1 was
> apparently handling only comma expressions, some casts, . (COMPONENT_REF),
> [] (ARRAY_REF), but not COND_EXPR.
>
> The following patch handles COND_EXPR in there too, in a similar way how
> the gimplifier handles the cleanups of the expressions in the COND_EXPR
> operand if there is no lifetime extension due to reference binding.
> In particular there are bool temporaries added, initialized to false and set
> to true if the particular (leaf) COND_EXPR argument has been invoked and the
> corresponding cleanup guarded that way.
>
> I admit I haven't tried to construct testcases for all the things CWG 1299
> added, e.g. don't know if ( expression ) will work, if all the *_cast cases
> that need to be extended work (some of them do, as the testcase shows), or
> if e.g. .* works, so I'm not claiming the DR is completely implemented.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk
> and after a while for release branches?
OK.
> Note, just to double check g++ doesn't ICE on expr1 ? : expr2 GNU extension,
> I also wrote the attached testcase, but unlike the testcase included in the
> patch which behaves the same with patched g++ as does recent clang++, the
> testcase with expr1 ? : expr2 (omitted second operand) actually never
> extends the lifetime of any temporary (that is the baz function), unlike
> clang++ which extends the lifetime of expr2 if expr1 is false, and doesn't
> extend lifetime of anything if expr1 is true. This is outside of the scope
> of the standard, so no idea what is correct, so I'm just asking how it
> should behave.
I would expect one or the other to be extended. I imagine that clang
not extending expr1 is a bug due to however they avoid evaluating it twice.
> extend_ref_init_temps_1 is never called in that case when the expression is COND_EXPR.
I think the problem is in build_conditional_expr_1, which needs to treat
xvalues properly in the handling of this extension, i.e.
> - if (lvalue_p (arg1))
> + if (glvalue_p (arg1))
> arg2 = arg1 = cp_stabilize_reference (arg1);
Jason
^ permalink raw reply [flat|nested] 3+ messages in thread
* [committed] Fix up xvalue handling in ?: with omitted middle operand (PR c++/92831)
2019-12-06 19:32 ` Jason Merrill
@ 2019-12-06 23:46 ` Jakub Jelinek
0 siblings, 0 replies; 3+ messages in thread
From: Jakub Jelinek @ 2019-12-06 23:46 UTC (permalink / raw)
To: Jason Merrill; +Cc: gcc-patches
On Fri, Dec 06, 2019 at 02:31:55PM -0500, Jason Merrill wrote:
> > Note, just to double check g++ doesn't ICE on expr1 ? : expr2 GNU extension,
> > I also wrote the attached testcase, but unlike the testcase included in the
> > patch which behaves the same with patched g++ as does recent clang++, the
> > testcase with expr1 ? : expr2 (omitted second operand) actually never
> > extends the lifetime of any temporary (that is the baz function), unlike
> > clang++ which extends the lifetime of expr2 if expr1 is false, and doesn't
> > extend lifetime of anything if expr1 is true. This is outside of the scope
> > of the standard, so no idea what is correct, so I'm just asking how it
> > should behave.
>
> I would expect one or the other to be extended. I imagine that clang not
> extending expr1 is a bug due to however they avoid evaluating it twice.
>
> > extend_ref_init_temps_1 is never called in that case when the expression is COND_EXPR.
>
> I think the problem is in build_conditional_expr_1, which needs to treat
> xvalues properly in the handling of this extension, i.e.
>
> > - if (lvalue_p (arg1))
> > + if (glvalue_p (arg1))
> > arg2 = arg1 = cp_stabilize_reference (arg1);
Indeed, that fixes it and you've preapproved a patch doing that on IRC, which I've
committed to trunk now that it successfully bootstrapped/regtested on
x86_64-linux and i686-linux. Thanks.
2019-12-07 Jason Merrill <jason@redhat.com>
Jakub Jelinek <jakub@redhat.com>
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.
--- gcc/cp/call.c.jj 2019-12-06 21:15:48.842199643 +0100
+++ gcc/cp/call.c 2019-12-06 22:01:18.737636630 +0100
@@ -5077,7 +5077,7 @@ build_conditional_expr_1 (const op_locat
warn_for_omitted_condop (loc, arg1);
/* Make sure that lvalues remain lvalues. See g++.oliva/ext1.C. */
- if (lvalue_p (arg1))
+ if (glvalue_p (arg1))
arg2 = arg1 = cp_stabilize_reference (arg1);
else
arg2 = arg1 = cp_save_expr (arg1);
--- gcc/testsuite/g++.dg/ext/temp-extend1.C.jj 2019-12-06 22:10:45.489814074 +0100
+++ gcc/testsuite/g++.dg/ext/temp-extend1.C 2019-12-06 22:10:09.495374051 +0100
@@ -0,0 +1,43 @@
+// PR c++/92831
+// { dg-do run { target c++11 } }
+// { dg-options "" }
+
+template<typename T> using id = T;
+struct S { S () : s (false) { ++c; } S (bool x) : s (x) { ++c; } ~S () { --c; }; bool s; static int c; };
+int S::c = 0;
+
+void
+foo (int i)
+{
+ const bool&& a
+ = id<S[3]>{false, true, false}[i].s
+ ? id<S[2]>{true, false}[i].s : id<S[4]>{true, false, true, false}[i].s;
+ if (S::c != (i ? 2 : 4))
+ __builtin_abort ();
+}
+
+void
+baz (int i)
+{
+ const bool&& a = id<S[3]>{false, true, false}[i].s
+ ? : id<S[4]>{true, false, true, false}[i].s;
+ if (S::c != (i ? 3 : 4))
+ __builtin_abort ();
+}
+
+int
+main ()
+{
+ foo (0);
+ if (S::c != 0)
+ __builtin_abort ();
+ foo (1);
+ if (S::c != 0)
+ __builtin_abort ();
+ baz (0);
+ if (S::c != 0)
+ __builtin_abort ();
+ baz (1);
+ if (S::c != 0)
+ __builtin_abort ();
+}
Jakub
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-12-06 23:46 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-06 16:28 [C++ PATCH] CWG 1299, not extending temporary lifetime for ?: (PR c++/92831) Jakub Jelinek
2019-12-06 19:32 ` Jason Merrill
2019-12-06 23:46 ` [committed] Fix up xvalue handling in ?: with omitted middle operand " Jakub Jelinek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).