Re: C++ PATCH for c++/70744 (wrong-code with x ?: y extension)

[Jason Merrill]

On 04/25/2016 11:35 AM, Marek Polacek wrote:

On Fri, Apr 22, 2016 at 03:28:27PM -0400, Jason Merrill wrote:

On Fri, Apr 22, 2016 at 2:12 PM, Marek Polacek  wrote:

+cp_stabilize_reference (tree ref)
+{
+  if (TREE_CODE (ref) == PREINCREMENT_EXPR
+  || TREE_CODE (ref) == PREDECREMENT_EXPR)


I think we want to do this for anything stabilize_reference doesn't
handle specifically, not just pre..crement.


Which would mean something along the lines of the following, I hope.


Yes, except let's drop this case:


+case COMPOUND_EXPR:


because stabilize_expr will do the wrong thing for lvalue COMPOUND_EXPR.

OK with that change.

Jason

Re: C++ PATCH for c++/70744 (wrong-code with x ?: y extension)

[Marek Polacek]

On Fri, Apr 22, 2016 at 03:28:27PM -0400, Jason Merrill wrote:
> On Fri, Apr 22, 2016 at 2:12 PM, Marek Polacek  wrote:
> > +cp_stabilize_reference (tree ref)
> > +{
> > +  if (TREE_CODE (ref) == PREINCREMENT_EXPR
> > +  || TREE_CODE (ref) == PREDECREMENT_EXPR)
> 
> I think we want to do this for anything stabilize_reference doesn't
> handle specifically, not just pre..crement.

Which would mean something along the lines of the following, I hope.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2016-04-25  Marek Polacek  

PR c++/70744
* call.c (build_conditional_expr_1): Call cp_stabilize_reference
instead of stabilize_reference.
(build_over_call): Likewise.
* cp-tree.h (cp_stabilize_reference): Declare.
* tree.c (cp_stabilize_reference): New function.
* typeck.c (cp_build_unary_op): Call cp_stabilize_reference instead of
stabilize_reference.
(unary_complex_lvalue): Likewise.
(cp_build_modify_expr): Likewise.

* g++.dg/ext/cond2.C: New test.

diff --git gcc/cp/call.c gcc/cp/call.c
index 11f2d42..476e806 100644
--- gcc/cp/call.c
+++ gcc/cp/call.c
@@ -4634,7 +4634,7 @@ build_conditional_expr_1 (location_t loc, tree arg1, tree 
arg2, tree arg3,
 
   /* Make sure that lvalues remain lvalues.  See g++.oliva/ext1.C.  */
   if (real_lvalue_p (arg1))
-   arg2 = arg1 = stabilize_reference (arg1);
+   arg2 = arg1 = cp_stabilize_reference (arg1);
   else
arg2 = arg1 = save_expr (arg1);
 }
@@ -7644,8 +7644,9 @@ build_over_call (struct z_candidate *cand, int flags, 
tsubst_flags_t complain)
   || (TREE_CODE (arg) == TARGET_EXPR
   && !unsafe_copy_elision_p (fa, arg)))
{
- tree to = stabilize_reference (cp_build_indirect_ref (fa, RO_NULL,
-   complain));
+ tree to = cp_stabilize_reference (cp_build_indirect_ref (fa,
+  RO_NULL,
+  complain));
 
  val = build2 (INIT_EXPR, DECL_CONTEXT (fn), to, arg);
  return val;
@@ -7655,7 +7656,7 @@ build_over_call (struct z_candidate *cand, int flags, 
tsubst_flags_t complain)
   && trivial_fn_p (fn)
   && !DECL_DELETED_FN (fn))
 {
-  tree to = stabilize_reference
+  tree to = cp_stabilize_reference
(cp_build_indirect_ref (argarray[0], RO_NULL, complain));
   tree type = TREE_TYPE (to);
   tree as_base = CLASSTYPE_AS_BASE (type);
diff --git gcc/cp/cp-tree.h gcc/cp/cp-tree.h
index ec92718..0e46ae1 100644
--- gcc/cp/cp-tree.h
+++ gcc/cp/cp-tree.h
@@ -6494,6 +6494,7 @@ extern cp_lvalue_kind real_lvalue_p   
(const_tree);
 extern cp_lvalue_kind lvalue_kind  (const_tree);
 extern bool lvalue_or_rvalue_with_address_p(const_tree);
 extern bool xvalue_p   (const_tree);
+extern tree cp_stabilize_reference (tree);
 extern bool builtin_valid_in_constant_expr_p(const_tree);
 extern tree build_min  (enum tree_code, tree, ...);
 extern tree build_min_nt_loc   (location_t, enum tree_code,
diff --git gcc/cp/tree.c gcc/cp/tree.c
index 112c8c7..137186f 100644
--- gcc/cp/tree.c
+++ gcc/cp/tree.c
@@ -296,6 +296,46 @@ xvalue_p (const_tree ref)
   return (lvalue_kind (ref) == clk_rvalueref);
 }
 
+/* C++-specific version of stabilize_reference.  */
+
+tree
+cp_stabilize_reference (tree ref)
+{
+  switch (TREE_CODE (ref))
+{
+/* We need to treat specially anything stabilize_reference doesn't
+   handle specifically.  */
+case VAR_DECL:
+case PARM_DECL:
+case RESULT_DECL:
+CASE_CONVERT:
+case FLOAT_EXPR:
+case FIX_TRUNC_EXPR:
+case INDIRECT_REF:
+case COMPONENT_REF:
+case BIT_FIELD_REF:
+case ARRAY_REF:
+case ARRAY_RANGE_REF:
+case COMPOUND_EXPR:
+case ERROR_MARK:
+  break;
+default:
+  cp_lvalue_kind kind = lvalue_kind (ref);
+  if ((kind & ~clk_class) != clk_none)
+   {
+ tree type = unlowered_expr_type (ref);
+ bool rval = !!(kind & clk_rvalueref);
+ type = cp_build_reference_type (type, rval);
+ /* This inhibits warnings in, eg, cxx_mark_addressable
+(c++/60955).  */
+ warning_sentinel s (extra_warnings);
+ ref = build_static_cast (type, ref, tf_error);
+   }
+}
+
+  return stabilize_reference (ref);
+}
+
 /* Test whether DECL is a builtin that may appear in a
constant-expression. */
 
diff --git gcc/cp/typeck.c gcc/cp/typeck.c
index cef5604..7e12009 100644
--- gcc/cp/typeck.c
+++ gcc/cp/typeck.c
@@ -5912,7 +5912,7 @@ cp_build_unary_op (enum tree_code code, tree xarg, int 
noconvert,
{
  tree real, imag;
 
- arg = stabilize_reference (arg);
+ arg = 

Re: C++ PATCH for c++/70744 (wrong-code with x ?: y extension)

[Jason Merrill]

On Fri, Apr 22, 2016 at 2:12 PM, Marek Polacek  wrote:
> On Fri, Apr 22, 2016 at 09:43:31AM -0400, Jason Merrill wrote:
>> On 04/22/2016 07:50 AM, Marek Polacek wrote:
>> >This PR shows that we generate wrong code with the x ?: y extension in case 
>> >the
>> >first operand contains either predecrement or preincrement.  The problem is
>> >that we don't emit SAVE_EXPR, thus the operand is evaluated twice, which it
>> >should not be.
>> >
>> >While ++i or --i can be lvalues in C++, i++ or i-- can not.  The code in
>> >build_conditional_expr_1 has:
>> >  4635   /* Make sure that lvalues remain lvalues.  See 
>> > g++.oliva/ext1.C.  */
>> >  4636   if (real_lvalue_p (arg1))
>> >  4637 arg2 = arg1 = stabilize_reference (arg1);
>> >  4638   else
>> >  4639 arg2 = arg1 = save_expr (arg1);
>> >so for ++i/--i we call stabilize_reference, but that doesn't know anything
>> >about PREINCREMENT_EXPR or PREDECREMENT_EXPR and just returns the same
>> >expression, so SAVE_EXPR isn't created.
>> >
>> >I think one fix would be to teach stabilize_reference what to do with those,
>> >similarly to how we handle COMPOUND_EXPR there.
>>
>> Your change will turn the expression into an rvalue, so that isn't enough.
>
> Oops.
>
>> We need to turn the expression into some sort of _REF before passing it to
>> stabilize_reference, perhaps by factoring out the cast-to-reference code
>> from force_paren_expr.  This should probably be part of a
>> cp_stabilize_reference function that replaces all uses of
>> stabilize_reference in the front end.
>
> Thanks, this magic seems to work.  So something like the following?  I didn't
> put the cast-to-reference code into its separate function because it didn't
> seem convenient, but I can work on that, too, if you prefer.

> +cp_stabilize_reference (tree ref)
> +{
> +  if (TREE_CODE (ref) == PREINCREMENT_EXPR
> +  || TREE_CODE (ref) == PREDECREMENT_EXPR)

I think we want to do this for anything stabilize_reference doesn't
handle specifically, not just pre..crement.

Jason

Re: C++ PATCH for c++/70744 (wrong-code with x ?: y extension)

[Marek Polacek]

On Fri, Apr 22, 2016 at 09:43:31AM -0400, Jason Merrill wrote:
> On 04/22/2016 07:50 AM, Marek Polacek wrote:
> >This PR shows that we generate wrong code with the x ?: y extension in case 
> >the
> >first operand contains either predecrement or preincrement.  The problem is
> >that we don't emit SAVE_EXPR, thus the operand is evaluated twice, which it
> >should not be.
> >
> >While ++i or --i can be lvalues in C++, i++ or i-- can not.  The code in
> >build_conditional_expr_1 has:
> >  4635   /* Make sure that lvalues remain lvalues.  See 
> > g++.oliva/ext1.C.  */
> >  4636   if (real_lvalue_p (arg1))
> >  4637 arg2 = arg1 = stabilize_reference (arg1);
> >  4638   else
> >  4639 arg2 = arg1 = save_expr (arg1);
> >so for ++i/--i we call stabilize_reference, but that doesn't know anything
> >about PREINCREMENT_EXPR or PREDECREMENT_EXPR and just returns the same
> >expression, so SAVE_EXPR isn't created.
> >
> >I think one fix would be to teach stabilize_reference what to do with those,
> >similarly to how we handle COMPOUND_EXPR there.
> 
> Your change will turn the expression into an rvalue, so that isn't enough.

Oops.

> We need to turn the expression into some sort of _REF before passing it to
> stabilize_reference, perhaps by factoring out the cast-to-reference code
> from force_paren_expr.  This should probably be part of a
> cp_stabilize_reference function that replaces all uses of
> stabilize_reference in the front end.

Thanks, this magic seems to work.  So something like the following?  I didn't
put the cast-to-reference code into its separate function because it didn't
seem convenient, but I can work on that, too, if you prefer.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2016-04-22  Marek Polacek  

PR c++/70744
* call.c (build_conditional_expr_1): Call cp_stabilize_reference
instead of stabilize_reference.
(build_over_call): Likewise.
* cp-tree.h (cp_stabilize_reference): Declare.
* tree.c (cp_stabilize_reference): New function.
* typeck.c (cp_build_unary_op): Call cp_stabilize_reference instead of
stabilize_reference.
(unary_complex_lvalue): Likewise.
(cp_build_modify_expr): Likewise.

* g++.dg/ext/cond2.C: New test.

diff --git gcc/cp/call.c gcc/cp/call.c
index 11f2d42..476e806 100644
--- gcc/cp/call.c
+++ gcc/cp/call.c
@@ -4634,7 +4634,7 @@ build_conditional_expr_1 (location_t loc, tree arg1, tree 
arg2, tree arg3,
 
   /* Make sure that lvalues remain lvalues.  See g++.oliva/ext1.C.  */
   if (real_lvalue_p (arg1))
-   arg2 = arg1 = stabilize_reference (arg1);
+   arg2 = arg1 = cp_stabilize_reference (arg1);
   else
arg2 = arg1 = save_expr (arg1);
 }
@@ -7644,8 +7644,9 @@ build_over_call (struct z_candidate *cand, int flags, 
tsubst_flags_t complain)
   || (TREE_CODE (arg) == TARGET_EXPR
   && !unsafe_copy_elision_p (fa, arg)))
{
- tree to = stabilize_reference (cp_build_indirect_ref (fa, RO_NULL,
-   complain));
+ tree to = cp_stabilize_reference (cp_build_indirect_ref (fa,
+  RO_NULL,
+  complain));
 
  val = build2 (INIT_EXPR, DECL_CONTEXT (fn), to, arg);
  return val;
@@ -7655,7 +7656,7 @@ build_over_call (struct z_candidate *cand, int flags, 
tsubst_flags_t complain)
   && trivial_fn_p (fn)
   && !DECL_DELETED_FN (fn))
 {
-  tree to = stabilize_reference
+  tree to = cp_stabilize_reference
(cp_build_indirect_ref (argarray[0], RO_NULL, complain));
   tree type = TREE_TYPE (to);
   tree as_base = CLASSTYPE_AS_BASE (type);
diff --git gcc/cp/cp-tree.h gcc/cp/cp-tree.h
index ec92718..0e46ae1 100644
--- gcc/cp/cp-tree.h
+++ gcc/cp/cp-tree.h
@@ -6494,6 +6494,7 @@ extern cp_lvalue_kind real_lvalue_p   
(const_tree);
 extern cp_lvalue_kind lvalue_kind  (const_tree);
 extern bool lvalue_or_rvalue_with_address_p(const_tree);
 extern bool xvalue_p   (const_tree);
+extern tree cp_stabilize_reference (tree);
 extern bool builtin_valid_in_constant_expr_p(const_tree);
 extern tree build_min  (enum tree_code, tree, ...);
 extern tree build_min_nt_loc   (location_t, enum tree_code,
diff --git gcc/cp/tree.c gcc/cp/tree.c
index 112c8c7..44e3893 100644
--- gcc/cp/tree.c
+++ gcc/cp/tree.c
@@ -296,6 +296,31 @@ xvalue_p (const_tree ref)
   return (lvalue_kind (ref) == clk_rvalueref);
 }
 
+/* C++-specific version of stabilize_reference.  For preincrement and
+   predecrement expressions we need to turn the expression into some
+   sort of _REF before passing it to stabilize_reference.  */
+
+tree
+cp_stabilize_reference (tree ref)

Re: C++ PATCH for c++/70744 (wrong-code with x ?: y extension)

[Jason Merrill]

On 04/22/2016 07:50 AM, Marek Polacek wrote:

This PR shows that we generate wrong code with the x ?: y extension in case the
first operand contains either predecrement or preincrement.  The problem is
that we don't emit SAVE_EXPR, thus the operand is evaluated twice, which it
should not be.

While ++i or --i can be lvalues in C++, i++ or i-- can not.  The code in
build_conditional_expr_1 has:
  4635   /* Make sure that lvalues remain lvalues.  See g++.oliva/ext1.C.  
*/
  4636   if (real_lvalue_p (arg1))
  4637 arg2 = arg1 = stabilize_reference (arg1);
  4638   else
  4639 arg2 = arg1 = save_expr (arg1);
so for ++i/--i we call stabilize_reference, but that doesn't know anything
about PREINCREMENT_EXPR or PREDECREMENT_EXPR and just returns the same
expression, so SAVE_EXPR isn't created.

I think one fix would be to teach stabilize_reference what to do with those,
similarly to how we handle COMPOUND_EXPR there.


Your change will turn the expression into an rvalue, so that isn't 
enough.  We need to turn the expression into some sort of _REF before 
passing it to stabilize_reference, perhaps by factoring out the 
cast-to-reference code from force_paren_expr.  This should probably be 
part of a cp_stabilize_reference function that replaces all uses of 
stabilize_reference in the front end.


Jason

C++ PATCH for c++/70744 (wrong-code with x ?: y extension)

[Marek Polacek]

This PR shows that we generate wrong code with the x ?: y extension in case the
first operand contains either predecrement or preincrement.  The problem is
that we don't emit SAVE_EXPR, thus the operand is evaluated twice, which it
should not be.

While ++i or --i can be lvalues in C++, i++ or i-- can not.  The code in
build_conditional_expr_1 has:
 4635   /* Make sure that lvalues remain lvalues.  See g++.oliva/ext1.C.  */
 4636   if (real_lvalue_p (arg1))
 4637 arg2 = arg1 = stabilize_reference (arg1);
 4638   else
 4639 arg2 = arg1 = save_expr (arg1);
so for ++i/--i we call stabilize_reference, but that doesn't know anything
about PREINCREMENT_EXPR or PREDECREMENT_EXPR and just returns the same
expression, so SAVE_EXPR isn't created.

I think one fix would be to teach stabilize_reference what to do with those,
similarly to how we handle COMPOUND_EXPR there.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2016-04-22  Marek Polacek  

PR c++/70744
* tree.c (stabilize_reference): Handle PREINCREMENT_EXPR and
PREDECREMENT_EXPR.

* g++.dg/ext/cond2.C: New test.

diff --git gcc/testsuite/g++.dg/ext/cond2.C gcc/testsuite/g++.dg/ext/cond2.C
index e69de29..d9f1d59 100644
--- gcc/testsuite/g++.dg/ext/cond2.C
+++ gcc/testsuite/g++.dg/ext/cond2.C
@@ -0,0 +1,28 @@
+// PR c++/70744
+// { dg-do run }
+// { dg-options "" }
+
+static void
+fn1 (void)
+{
+  int x = 2;
+  ++x ? : 42;
+  if (x != 3)
+__builtin_abort ();
+  --x ? : 42;
+  if (x != 2)
+__builtin_abort ();
+  x++ ? : 42;
+  if (x != 3)
+__builtin_abort ();
+  x-- ? : 42;
+  if (x != 2)
+__builtin_abort ();
+}
+
+int
+main ()
+{
+  fn1 ();
+  return 0;
+}
diff --git gcc/tree.c gcc/tree.c
index 6de46a8..8fd4e81 100644
--- gcc/tree.c
+++ gcc/tree.c
@@ -4255,6 +4255,13 @@ stabilize_reference (tree ref)
 volatiles.  */
   return stabilize_reference_1 (ref);
 
+case PREINCREMENT_EXPR:
+case PREDECREMENT_EXPR:
+  /* While in C++ postincrement and postdecrement expressions are not
+considered lvalues, preincrement and predecrement expressions can
+be lvalues.  */
+  return stabilize_reference_1 (ref);
+
   /* If arg isn't a kind of lvalue we recognize, make no change.
 Caller should recognize the error for an invalid lvalue.  */
 default:

Marek