[C++ RFH] PR 56961

[C++ RFH] PR 56961

[Paolo Carlini]

Hi,

in this minor issue, after a permerror about "passing ‘volatile foo’ as 
‘this’ argument discards qualifiers" we crash with an infinite recursion 
in the gimplifier. The testcase:

struct foo { };

typedef struct
{
volatile foo fields;
} CSPHandleState;

CSPHandleState a;

void fn1 ()
{
CSPHandleState b;
b.fields = foo();
}

involves the empty struct foo, and I noticed that the crash doesn't 
happen otherwise. Therefore this comment in cp-gimplify.c seems relevant:

624 /* Remove any copies of empty classes. We check that the RHS
625 has a simple form so that TARGET_EXPRs and non-empty
626 CONSTRUCTORs get reduced properly, and we leave the return
627 slot optimization alone because it isn't a copy (FIXME so it
628 shouldn't be represented as one).
629
630 Also drop volatile variables on the RHS to avoid infinite
631 recursion from gimplify_expr trying to load the value. */
632 if (!TREE_SIDE_EFFECTS (op1)
633 || (DECL_P (op1) && TREE_THIS_VOLATILE (op1)))
634 *expr_p = op0;
635 else if (TREE_CODE (op1) == MEM_REF
636 && TREE_THIS_VOLATILE (op1))
637 {
638 /* Similarly for volatile MEM_REFs on the RHS. */

and in fact we get there, op1 is volatile, but we don't adjust things 
because op1 is a COMPONENT_REF, not a decl, not a MEM_REF. Then I'm 
wondering if we should handle in the same place the COMPONENT_REF 
case?!? Eg, brutally hacking the above to handle a COMPONENT_REF like a 
DECL_P avoids the infinite recursion. The below is *expr_p (its arg0 is 
op0 and its arg1 is op1). Hints?

Thanks!
Paolo.

////////////////////////

<modify_expr 0x7ffff682d668
type <record_type 0x7ffff6835150 foo sizes-gimplified type_5 type_6 QI
size <integer_cst 0x7ffff66d5918 constant 8>
unit size <integer_cst 0x7ffff66d5930 constant 1>
align 8 symtab 0 alias set -1 canonical type 0x7ffff6835150
fields <type_decl 0x7ffff682e958 foo type <record_type 0x7ffff68351f8 foo>
nonlocal decl_4 VOID file 56961.C line 1 col 12
align 1 context <record_type 0x7ffff6835150 foo> result <record_type 
0x7ffff6835150 foo>
 > context <translation_unit_decl 0x7ffff66e0170 D.1>
full-name "struct foo"
X() X(constX&) this=(X&) n_parents=0 use_template=0 interface-unknown
pointer_to_this <pointer_type 0x7ffff6835930> reference_to_this 
<reference_type 0x7ffff6835690> chain <type_decl 0x7ffff682e8a0 foo>>
side-effects
arg 0 <var_decl 0x7ffff683a7b8 vol.0 type <record_type 0x7ffff6835150 foo>
used ignored QI file 56961.C line 13 col 19 size <integer_cst 
0x7ffff66d5918 8> unit size <integer_cst 0x7ffff66d5930 1>
align 8 context <function_decl 0x7ffff6834b00 fn1>>
arg 1 <component_ref 0x7ffff66d8870
type <record_type 0x7ffff6835498 foo sizes-gimplified volatile type_5 
type_6 QI size <integer_cst 0x7ffff66d5918 8> unit size <integer_cst 
0x7ffff66d5930 1>
align 8 symtab 0 alias set -1 canonical type 0x7ffff6835498 fields 
<type_decl 0x7ffff682e958 foo> context <translation_unit_decl 
0x7ffff66e0170 D.1>
full-name "volatile struct foo"
X() X(constX&) this=(X&) n_parents=0 use_template=0 interface-unknown
pointer_to_this <pointer_type 0x7ffff6835b28>>
side-effects volatile
arg 0 <var_decl 0x7ffff683a558 b type <record_type 0x7ffff6835348 
CSPHandleState>
addressable used tree_1 decl_5 QI file 56961.C line 12 col 18 size 
<integer_cst 0x7ffff66d5918 8> unit size <integer_cst 0x7ffff66d5930 1>
align 8 context <function_decl 0x7ffff6834b00 fn1>>
arg 1 <field_decl 0x7ffff683a390 fields type <record_type 0x7ffff6835498 
foo>
side-effects volatile used nonlocal decl_3 QI file 56961.C line 5 col 16 
size <integer_cst 0x7ffff66d5918 8> unit size <integer_cst 0x7ffff66d5930 1>
align 8 offset_align 128
offset <integer_cst 0x7ffff66d5858 constant 0>
bit offset <integer_cst 0x7ffff66d58a0 constant 0> context <record_type 
0x7ffff6835348 CSPHandleState> chain <type_decl 0x7ffff682eac8 ._0>>>>

Re: [C++ RFH] PR 56961

[Richard Biener]

On Thu, Jun 5, 2014 at 2:59 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi,
>
> in this minor issue, after a permerror about "passing ‘volatile foo’ as
> ‘this’ argument discards qualifiers" we crash with an infinite recursion in
> the gimplifier. The testcase:
>
> struct foo { };
>
> typedef struct
> {
> volatile foo fields;
> } CSPHandleState;
>
> CSPHandleState a;
>
> void fn1 ()
> {
> CSPHandleState b;
> b.fields = foo();
> }
>
> involves the empty struct foo, and I noticed that the crash doesn't happen
> otherwise. Therefore this comment in cp-gimplify.c seems relevant:
>
> 624 /* Remove any copies of empty classes. We check that the RHS
> 625 has a simple form so that TARGET_EXPRs and non-empty
> 626 CONSTRUCTORs get reduced properly, and we leave the return
> 627 slot optimization alone because it isn't a copy (FIXME so it
> 628 shouldn't be represented as one).
> 629
> 630 Also drop volatile variables on the RHS to avoid infinite
> 631 recursion from gimplify_expr trying to load the value. */
> 632 if (!TREE_SIDE_EFFECTS (op1)
> 633 || (DECL_P (op1) && TREE_THIS_VOLATILE (op1)))
> 634 *expr_p = op0;
> 635 else if (TREE_CODE (op1) == MEM_REF
> 636 && TREE_THIS_VOLATILE (op1))
> 637 {
> 638 /* Similarly for volatile MEM_REFs on the RHS. */
>
> and in fact we get there, op1 is volatile, but we don't adjust things
> because op1 is a COMPONENT_REF, not a decl, not a MEM_REF. Then I'm
> wondering if we should handle in the same place the COMPONENT_REF case?!?
> Eg, brutally hacking the above to handle a COMPONENT_REF like a DECL_P
> avoids the infinite recursion. The below is *expr_p (its arg0 is op0 and its
> arg1 is op1). Hints?

See my comment in the PR.  You need to handle all references/decls
here but preserve side-effects in operands of references.  For example
by gimplifying as unused address (just an idea):

Index: gcc/cp/cp-gimplify.c
===================================================================
--- gcc/cp/cp-gimplify.c        (revision 211262)
+++ gcc/cp/cp-gimplify.c        (working copy)
@@ -629,18 +629,14 @@ cp_gimplify_expr (tree *expr_p, gimple_s

               Also drop volatile variables on the RHS to avoid infinite
               recursion from gimplify_expr trying to load the value.  */
-           if (!TREE_SIDE_EFFECTS (op1)
-               || (DECL_P (op1) && TREE_THIS_VOLATILE (op1)))
+           if (!TREE_SIDE_EFFECTS (op1))
              *expr_p = op0;
-           else if (TREE_CODE (op1) == MEM_REF
-                    && TREE_THIS_VOLATILE (op1))
+           else if (TREE_THIS_VOLATILE (op1)
+                    && (REFERENCE_CLASS_P (op1) || DECL_P (op1)))
              {
-               /* Similarly for volatile MEM_REFs on the RHS.  */
-               if (!TREE_SIDE_EFFECTS (TREE_OPERAND (op1, 0)))
-                 *expr_p = op0;
-               else
-                 *expr_p = build2 (COMPOUND_EXPR, TREE_TYPE (*expr_p),
-                                   TREE_OPERAND (op1, 0), op0);
+               *expr_p = build2 (COMPOUND_EXPR, TREE_TYPE (*expr_p),
+                                 op0, build_fold_addr_expr (op1));
+
              }
            else
              *expr_p = build2 (COMPOUND_EXPR, TREE_TYPE (*expr_p),

Richard.


> Thanks!
> Paolo.
>
> ////////////////////////
>
> <modify_expr 0x7ffff682d668
> type <record_type 0x7ffff6835150 foo sizes-gimplified type_5 type_6 QI
> size <integer_cst 0x7ffff66d5918 constant 8>
> unit size <integer_cst 0x7ffff66d5930 constant 1>
> align 8 symtab 0 alias set -1 canonical type 0x7ffff6835150
> fields <type_decl 0x7ffff682e958 foo type <record_type 0x7ffff68351f8 foo>
> nonlocal decl_4 VOID file 56961.C line 1 col 12
> align 1 context <record_type 0x7ffff6835150 foo> result <record_type
> 0x7ffff6835150 foo>
>> context <translation_unit_decl 0x7ffff66e0170 D.1>
> full-name "struct foo"
> X() X(constX&) this=(X&) n_parents=0 use_template=0 interface-unknown
> pointer_to_this <pointer_type 0x7ffff6835930> reference_to_this
> <reference_type 0x7ffff6835690> chain <type_decl 0x7ffff682e8a0 foo>>
> side-effects
> arg 0 <var_decl 0x7ffff683a7b8 vol.0 type <record_type 0x7ffff6835150 foo>
> used ignored QI file 56961.C line 13 col 19 size <integer_cst 0x7ffff66d5918
> 8> unit size <integer_cst 0x7ffff66d5930 1>
> align 8 context <function_decl 0x7ffff6834b00 fn1>>
> arg 1 <component_ref 0x7ffff66d8870
> type <record_type 0x7ffff6835498 foo sizes-gimplified volatile type_5 type_6
> QI size <integer_cst 0x7ffff66d5918 8> unit size <integer_cst 0x7ffff66d5930
> 1>
> align 8 symtab 0 alias set -1 canonical type 0x7ffff6835498 fields
> <type_decl 0x7ffff682e958 foo> context <translation_unit_decl 0x7ffff66e0170
> D.1>
> full-name "volatile struct foo"
> X() X(constX&) this=(X&) n_parents=0 use_template=0 interface-unknown
> pointer_to_this <pointer_type 0x7ffff6835b28>>
> side-effects volatile
> arg 0 <var_decl 0x7ffff683a558 b type <record_type 0x7ffff6835348
> CSPHandleState>
> addressable used tree_1 decl_5 QI file 56961.C line 12 col 18 size
> <integer_cst 0x7ffff66d5918 8> unit size <integer_cst 0x7ffff66d5930 1>
> align 8 context <function_decl 0x7ffff6834b00 fn1>>
> arg 1 <field_decl 0x7ffff683a390 fields type <record_type 0x7ffff6835498
> foo>
> side-effects volatile used nonlocal decl_3 QI file 56961.C line 5 col 16
> size <integer_cst 0x7ffff66d5918 8> unit size <integer_cst 0x7ffff66d5930 1>
> align 8 offset_align 128
> offset <integer_cst 0x7ffff66d5858 constant 0>
> bit offset <integer_cst 0x7ffff66d58a0 constant 0> context <record_type
> 0x7ffff6835348 CSPHandleState> chain <type_decl 0x7ffff682eac8 ._0>>>>
>
>
>

Re: [C++ RFH] PR 56961

[Jason Merrill]

On 06/05/2014 09:05 AM, Richard Biener wrote:
> +               *expr_p = build2 (COMPOUND_EXPR, TREE_TYPE (*expr_p),
> +                                 op0, build_fold_addr_expr (op1));

That seems like a fine approach.

Jason

Re: [C++ RFH] PR 56961

[Paolo Carlini]

Hi,

On 06/05/2014 03:12 PM, Jason Merrill wrote:
> On 06/05/2014 09:05 AM, Richard Biener wrote:
>> +               *expr_p = build2 (COMPOUND_EXPR, TREE_TYPE (*expr_p),
>> +                                 op0, build_fold_addr_expr (op1));
>
> That seems like a fine approach.
Thanks a lot guys. Therefore I'm going to regtest it and if everything 
goes well commit it with a testcase.

Thanks again,
Paolo.

Re: [C++ RFH] PR 56961

[Richard Biener]

On Thu, Jun 5, 2014 at 3:12 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi,
>
>
> On 06/05/2014 03:12 PM, Jason Merrill wrote:
>>
>> On 06/05/2014 09:05 AM, Richard Biener wrote:
>>>
>>> +               *expr_p = build2 (COMPOUND_EXPR, TREE_TYPE (*expr_p),
>>> +                                 op0, build_fold_addr_expr (op1));
>>
>>
>> That seems like a fine approach.
>
> Thanks a lot guys. Therefore I'm going to regtest it and if everything goes
> well commit it with a testcase.

I think the operands have to be reversed though - the type matches that
of op0.  Sorry ;)

Richard.

> Thanks again,
> Paolo.

Re: [C++ RFH] PR 56961

[Paolo Carlini]

[-- Attachment #1: Type: text/plain, Size: 205 bytes --]

Hi,

On 06/05/2014 03:20 PM, Richard Biener wrote:
> I think the operands have to be reversed though - the type matches 
> that of op0. Sorry ;)
Something like this, then?

Thanks,
Paolo.

///////////////

[-- Attachment #2: p --]
[-- Type: text/plain, Size: 1155 bytes --]

Index: cp-gimplify.c
===================================================================
--- cp-gimplify.c	(revision 211274)
+++ cp-gimplify.c	(working copy)
@@ -629,19 +629,12 @@ cp_gimplify_expr (tree *expr_p, gimple_seq *pre_p,
 
 	       Also drop volatile variables on the RHS to avoid infinite
 	       recursion from gimplify_expr trying to load the value.  */
-	    if (!TREE_SIDE_EFFECTS (op1)
-		|| (DECL_P (op1) && TREE_THIS_VOLATILE (op1)))
+           if (!TREE_SIDE_EFFECTS (op1))
 	      *expr_p = op0;
-	    else if (TREE_CODE (op1) == MEM_REF
-		     && TREE_THIS_VOLATILE (op1))
-	      {
-		/* Similarly for volatile MEM_REFs on the RHS.  */
-		if (!TREE_SIDE_EFFECTS (TREE_OPERAND (op1, 0)))
-		  *expr_p = op0;
-		else
-		  *expr_p = build2 (COMPOUND_EXPR, TREE_TYPE (*expr_p),
-				    TREE_OPERAND (op1, 0), op0);
-	      }
+           else if (TREE_THIS_VOLATILE (op1)
+                    && (REFERENCE_CLASS_P (op1) || DECL_P (op1)))
+	     *expr_p = build2 (COMPOUND_EXPR, TREE_TYPE (*expr_p),
+			       build_fold_addr_expr (op1), op0);
 	    else
 	      *expr_p = build2 (COMPOUND_EXPR, TREE_TYPE (*expr_p),
 				op0, op1);

Re: [C++ RFH] PR 56961

[Richard Biener]

On Thu, Jun 5, 2014 at 3:26 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi,
>
>
> On 06/05/2014 03:20 PM, Richard Biener wrote:
>>
>> I think the operands have to be reversed though - the type matches that of
>> op0. Sorry ;)
>
> Something like this, then?

Yes.  I suppose it's ok to re-order side-effects lhs, rhs to rhs, lhs?
Otherwise you'd need to do sth like

  op0 = save_expr (op0);
  *expr_p = build2 (COMPOUND_EXPR, TREE_TYPE (*expr_p),
                            op0,
                            build2 (COMPOUND_EXPR, TREE_TYPE (*expr_p),
                                       build_fold_addr_expr (op1), op0));

(which may or may not work or be a good idea with zero-size aggregate op0)

Richard.

> Thanks,
> Paolo.
>
> ///////////////

Re: [C++ RFH] PR 56961

[Paolo Carlini]

[-- Attachment #1: Type: text/plain, Size: 1078 bytes --]

Hi,

On 06/05/2014 03:35 PM, Richard Biener wrote:
> On Thu, Jun 5, 2014 at 3:26 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
>> Hi,
>>
>>
>> On 06/05/2014 03:20 PM, Richard Biener wrote:
>>> I think the operands have to be reversed though - the type matches that of
>>> op0. Sorry ;)
>> Something like this, then?
> Yes.  I suppose it's ok to re-order side-effects lhs, rhs to rhs, lhs?
> Otherwise you'd need to do sth like
>
>    op0 = save_expr (op0);
>    *expr_p = build2 (COMPOUND_EXPR, TREE_TYPE (*expr_p),
>                              op0,
>                              build2 (COMPOUND_EXPR, TREE_TYPE (*expr_p),
>                                         build_fold_addr_expr (op1), op0));
>
> (which may or may not work or be a good idea with zero-size aggregate op0)
In any case, I think that we would not regress on this... and, well, we 
have plenty of time for further tweaks (this goes to mainline only, of 
course). The below passes testing, if nobody has further comments 
tomorrow I will commit it.

Thanks again,
Paolo.

//////////////////////////

[-- Attachment #2: CL_56961 --]
[-- Type: text/plain, Size: 355 bytes --]

/cp
2014-06-05  Richard Biener  <rguenther@suse.de>
	    Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/56961
	* cp-gimplify.c (cp_gimplify_expr, [MODIFY_EXPR]): Rework
	handling of empty classes.

/testsuite
2014-06-05  Richard Biener  <rguenther@suse.de>
	    Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/56961
	* g++.dg/parse/pr56961.C: New.

[-- Attachment #3: patch_56961 --]
[-- Type: text/plain, Size: 1585 bytes --]

Index: cp/cp-gimplify.c
===================================================================
--- cp/cp-gimplify.c	(revision 211274)
+++ cp/cp-gimplify.c	(working copy)
@@ -629,19 +629,12 @@ cp_gimplify_expr (tree *expr_p, gimple_seq *pre_p,
 
 	       Also drop volatile variables on the RHS to avoid infinite
 	       recursion from gimplify_expr trying to load the value.  */
-	    if (!TREE_SIDE_EFFECTS (op1)
-		|| (DECL_P (op1) && TREE_THIS_VOLATILE (op1)))
+	    if (!TREE_SIDE_EFFECTS (op1))
 	      *expr_p = op0;
-	    else if (TREE_CODE (op1) == MEM_REF
-		     && TREE_THIS_VOLATILE (op1))
-	      {
-		/* Similarly for volatile MEM_REFs on the RHS.  */
-		if (!TREE_SIDE_EFFECTS (TREE_OPERAND (op1, 0)))
-		  *expr_p = op0;
-		else
-		  *expr_p = build2 (COMPOUND_EXPR, TREE_TYPE (*expr_p),
-				    TREE_OPERAND (op1, 0), op0);
-	      }
+	    else if (TREE_THIS_VOLATILE (op1)
+		     && (REFERENCE_CLASS_P (op1) || DECL_P (op1)))
+	      *expr_p = build2 (COMPOUND_EXPR, TREE_TYPE (*expr_p),
+				build_fold_addr_expr (op1), op0);
 	    else
 	      *expr_p = build2 (COMPOUND_EXPR, TREE_TYPE (*expr_p),
 				op0, op1);
Index: testsuite/g++.dg/parse/pr56961.C
===================================================================
--- testsuite/g++.dg/parse/pr56961.C	(revision 0)
+++ testsuite/g++.dg/parse/pr56961.C	(working copy)
@@ -0,0 +1,16 @@
+// PR c++/56961
+
+struct foo { };
+
+typedef struct
+{
+  volatile foo fields;
+} CSPHandleState;
+ 
+CSPHandleState a;
+
+void fn1 ()
+{
+  CSPHandleState b;
+  b.fields = foo();  // { dg-error "discards qualifiers" }
+}

Re: [C++ RFH] PR 56961

[Jason Merrill]

On 06/05/2014 09:35 AM, Richard Biener wrote:
> I suppose it's ok to re-order side-effects lhs, rhs to rhs, lhs?

Yes.

Jason