[Bug c++/79937] [6/7/8 Regression] ICE in replace_placeholders_r

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79937

--- Comment #22 from Jakub Jelinek  ---
Author: jakub
Date: Fri Mar 16 12:46:12 2018
New Revision: 258593

URL: https://gcc.gnu.org/viewcvs?rev=258593=gcc=rev
Log:
PR c++/79937
PR c++/82410
* tree.h (TARGET_EXPR_NO_ELIDE): Define.
* gimplify.c (gimplify_modify_expr_rhs): Don't elide TARGET_EXPRs with
TARGET_EXPR_NO_ELIDE flag set unless *expr_p is INIT_EXPR.

* cp-tree.h (CONSTRUCTOR_PLACEHOLDER_BOUNDARY): Define.
(find_placeholder): Declare.
* tree.c (struct replace_placeholders_t): Add exp member.
(replace_placeholders_r): Don't walk into ctors with
CONSTRUCTOR_PLACEHOLDER_BOUNDARY flag set, unless they are equal to
d->exp.  Replace PLACEHOLDER_EXPR with unshare_expr (x) rather than x.
(replace_placeholders): Initialize data.exp.
(find_placeholders_r, find_placeholders): New functions.
* typeck2.c (process_init_constructor_record,
process_init_constructor_union): Set CONSTRUCTOR_PLACEHOLDER_BOUNDARY
if adding NSDMI on which find_placeholder returns true.
* call.c (build_over_call): Don't call replace_placeholders here.
* cp-gimplify.c (cp_genericize_r): Set TARGET_EXPR_NO_ELIDE on
TARGET_EXPRs with CONSTRUCTOR_PLACEHOLDER_BOUNDARY set on
TARGET_EXPR_INITIAL.
(cp_fold): Copy over CONSTRUCTOR_PLACEHOLDER_BOUNDARY bit to new
ctor.

* g++.dg/cpp1y/pr79937-1.C: New test.
* g++.dg/cpp1y/pr79937-2.C: New test.
* g++.dg/cpp1y/pr79937-3.C: New test.
* g++.dg/cpp1y/pr79937-4.C: New test.
* g++.dg/cpp1y/pr82410.C: New test.

Added:
trunk/gcc/testsuite/g++.dg/cpp1y/pr79937-1.C
trunk/gcc/testsuite/g++.dg/cpp1y/pr79937-2.C
trunk/gcc/testsuite/g++.dg/cpp1y/pr79937-3.C
trunk/gcc/testsuite/g++.dg/cpp1y/pr79937-4.C
trunk/gcc/testsuite/g++.dg/cpp1y/pr82410.C
Modified:
trunk/gcc/ChangeLog
trunk/gcc/cp/ChangeLog
trunk/gcc/cp/call.c
trunk/gcc/cp/cp-gimplify.c
trunk/gcc/cp/cp-tree.h
trunk/gcc/cp/tree.c
trunk/gcc/cp/typeck2.c
trunk/gcc/gimplify.c
trunk/gcc/testsuite/ChangeLog
trunk/gcc/tree.h

[Bug c++/79937] [6/7/8 Regression] ICE in replace_placeholders_r

[jason at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79937

Jason Merrill  changed:

   What|Removed |Added

 Status|NEW |ASSIGNED
   Assignee|unassigned at gcc dot gnu.org  |jakub at gcc dot gnu.org

--- Comment #21 from Jason Merrill  ---
That looks like a good approach.

[Bug c++/79937] [6/7/8 Regression] ICE in replace_placeholders_r

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79937

Jakub Jelinek  changed:

   What|Removed |Added

  Attachment #43577|0   |1
is obsolete||
  Attachment #43578|0   |1
is obsolete||

--- Comment #20 from Jakub Jelinek  ---
Created attachment 43657
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=43657=edit
gcc8-pr79937.patch

This patch seems to fix all the testcase and passes check-c++-all.
Given that #c18 is rejected, I wonder if it wouldn't be sufficient.

[Bug c++/79937] [6/7/8 Regression] ICE in replace_placeholders_r

[jason at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79937

--- Comment #19 from Jason Merrill  ---
*** Bug 82410 has been marked as a duplicate of this bug. ***

[Bug c++/79937] [6/7/8 Regression] ICE in replace_placeholders_r

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79937

--- Comment #18 from Jakub Jelinek  ---
struct Y
{
  static Y bar (Y y) { return y; }
  int i;
  int n = bar (Y{2,i}).m + bar {Y{2,i,i}).n;
  int m = i;
};

is rejected, so I can't find a counter-example where PLACEHOLDER_EXPRs for
different objects would be intermixed in the same CONSTRUCTOR (assuming there
must be a CONSTRUCTOR).

[Bug c++/79937] [6/7/8 Regression] ICE in replace_placeholders_r

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79937

--- Comment #17 from Jakub Jelinek  ---
(In reply to Jason Merrill from comment #16)
> (In reply to Jakub Jelinek from comment #13)
> > E.g. could we walk into TARGET_EXPRs that have TARGET_EXPR_INITIAL
> > AGGR_INIT_EXPR, but avoid those that have TARGET_EXPR_INITIAL a CONSTRUCTOR,
> > or a CONSTRUCTOR with certain flags, or have some flags on TARGET_EXPRs?
> 
> No, the form of the TARGET_EXPR isn't sufficient to distinguish.  Here's a
> complete testcase:
> 
> struct X {
>   unsigned i;
>   unsigned n = i;
> };
> 
> X bar(X x) {
>   return x;
> }
> 
> struct Y
> {
>   static Y bar(Y y) { return y; }
>   unsigned i;
>   unsigned n = bar(Y{2,i}).n;
> };
> 
> int main()
> {
>   X x { 1, bar(X{2}).n };
>   if (x.n != 2)
> __builtin_abort();
>   
>   Y y { 1 };
>   if (y.n != 1)
> __builtin_abort();
> }
> 
> Here, the initializers for x and y end up looking equivalent within
> store_init_value, but the PLACEHOLDER_EXPRs are meant to refer to different
> objects.  There's no way for replace_placeholders to tell the difference.
> 
> I think to handle this we'll need to change process_init_constructor_record
> to either not expose PLACEHOLDER_EXPRs, or adjust them to indicate better
> which CONSTRUCTOR they refer to.

Ah, ok, that indeed fails with my patch.  Couldn't we mark somewhere using a
new lang flag the CONSTRUCTORs that are supposed to be boundaries for the
PLACEHOLDER_EXPRs
(in the above testcase on
{.i=1, .n=(TARGET_EXPR i}>)>).n}
it would be the {.i=2, .n=(&)->i} CONSTRUCTOR
and on
{.i=1, .n=(TARGET_EXPR i}>)>).n}
it wouldn't mark any)?  Is it the case that PLACEHOLDER_EXPRs should always
bind to some containing CONSTRUCTOR and can't be intermixed (say one
PLACEHOLDER_EXPR binding to inner CONSTRUCTOR and some other PLACEHOLDER_EXPR
within the same CONSTRUCTOR binding to an outer CONSTRUCTOR?

Or do you want the #c12 patch as a partial fix for GCC8 and come up with a real
fix for GCC9?  Or do nothing for now, something else?

[Bug c++/79937] [6/7/8 Regression] ICE in replace_placeholders_r

[jason at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79937

--- Comment #16 from Jason Merrill  ---
(In reply to Jakub Jelinek from comment #13)
> E.g. could we walk into TARGET_EXPRs that have TARGET_EXPR_INITIAL
> AGGR_INIT_EXPR, but avoid those that have TARGET_EXPR_INITIAL a CONSTRUCTOR,
> or a CONSTRUCTOR with certain flags, or have some flags on TARGET_EXPRs?

No, the form of the TARGET_EXPR isn't sufficient to distinguish.  Here's a
complete testcase:

struct X {
  unsigned i;
  unsigned n = i;
};

X bar(X x) {
  return x;
}

struct Y
{
  static Y bar(Y y) { return y; }
  unsigned i;
  unsigned n = bar(Y{2,i}).n;
};

int main()
{
  X x { 1, bar(X{2}).n };
  if (x.n != 2)
__builtin_abort();

  Y y { 1 };
  if (y.n != 1)
__builtin_abort();
}

Here, the initializers for x and y end up looking equivalent within
store_init_value, but the PLACEHOLDER_EXPRs are meant to refer to different
objects.  There's no way for replace_placeholders to tell the difference.

I think to handle this we'll need to change process_init_constructor_record to
either not expose PLACEHOLDER_EXPRs, or adjust them to indicate better which
CONSTRUCTOR they refer to.

[Bug c++/79937] [6/7/8 Regression] ICE in replace_placeholders_r

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79937

--- Comment #15 from Jakub Jelinek  ---
Created attachment 43578
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=43578=edit
gcc8-pr79937.patch

Actually, seems TREE_HAS_CONSTRUCTOR is set on a CONSTRUCTOR only by
finish_compound_literal.  So if those are something we don't want to walk into
and everything else we do, then this patch might be it (passed make
check-c++-all, but otherwise untested).

[Bug c++/79937] [6/7/8 Regression] ICE in replace_placeholders_r

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79937

--- Comment #14 from Jakub Jelinek  ---
The CONSTRUCTORS in TARGET_EXPR in the pr79937-{1,2,3}.C testcases have all
CONSTRUCTOR_IS_DIRECT_INIT and TREE_HAS_CONSTRUCTOR set, while nsdmi13.C
doesn't.
Does any of those matter?

In the nsdmi13.C case, there is just a single replace_placeholders call on the
whole testcase, so if we don't replace everything, we die during
gimplification,
but in pr79937-{1,2,3}.C the gimplification of the CONSTRUCTOR results in
cp_gimplify_init_expr and calls it there and handles the replacements that
aren't otherwise done.

[Bug c++/79937] [6/7/8 Regression] ICE in replace_placeholders_r

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79937

--- Comment #13 from Jakub Jelinek  ---
That said, I've tried:
--- gcc/cp/semantics.c.jj   2018-03-06 08:01:37.851883447 +0100
+++ gcc/cp/semantics.c  2018-03-06 15:19:27.685013764 +0100
@@ -2814,6 +2814,11 @@ finish_compound_literal (tree type, tree
   if (!VECTOR_TYPE_P (type))
 compound_literal = get_target_expr_sfinae (compound_literal, complain);

+  if (TREE_CODE (compound_literal) == TARGET_EXPR)
+compound_literal
+  = replace_placeholders (compound_literal,
+ TARGET_EXPR_SLOT (compound_literal));
+
   return compound_literal;
 }

instead, but that fails miserably during gimplification, seems we optimize away
those TARGET_EXPRs at some point just to their TARGET_EXPR_INITIAL and
referring in that CONSTRUCTOR to the TARGET_EXPR_SLOT then doesn't really work.

The regression was introduced by the:
-case TARGET_EXPR:
-  /* Don't mess with placeholders in an unrelated object.  */
-  *walk_subtrees = false;
-  break;
replace_placeholders_r change, which makes me wonder if replace_placeholders
could somehow make a difference between TARGET_EXPRs it wants to walk into
(e.g. the TARGET_EXPR created in nsdmi-aggr3.C:
#0  make_node (code=TARGET_EXPR) at ../../gcc/tree.c:1056
#1  0x015d4974 in build4 (code=TARGET_EXPR, tt=, arg0=, 
arg1=, arg2=, arg3=) at
../../gcc/tree.c:4797
#2  0x00ace5e4 in build_target_expr (decl=,
value=, complain=3)
at ../../gcc/cp/tree.c:454
#3  0x00acf39c in build_cplus_new (type=,
init=, complain=3)
at ../../gcc/cp/tree.c:631
#4  0x00ad8b16 in bot_manip (tp=0x7fffc3c8,
walk_subtrees=0x7fffc37c, data=0x2dd3c80) at ../../gcc/cp/tree.c:2915
#5  0x015f02f2 in walk_tree_1 (tp=0x7fffc3c8, func=0xad89ae
, data=0x2dd3c80, pset=0x0, 
lh=0xae085b  >*)>) at ../../gcc/tree.c:11400
#6  0x00ad94e2 in break_out_target_exprs (t=) at ../../gcc/cp/tree.c:3050
#7  0x0093e0f2 in get_nsdmi (member=,
in_ctor=false, complain=3) at ../../gcc/cp/init.c:637
and the TARGET_EXPRs created by finish_compound_literal by the testcase
included in the above patch or the:
// PR c++/79937
// { dg-do run { target c++14 } }

struct X {
  unsigned i;
  unsigned n = i;
  unsigned m = i;
};

X
bar (X x)
{
  if (x.i != 1 || x.n != 2 || x.m != 1)
__builtin_abort ();
  return x;
}

int
main ()
{
  X x = bar (X {1, X {2}.n});
  if (x.i != 1 || x.n != 2 || x.m != 1)
__builtin_abort ();
}

E.g. could we walk into TARGET_EXPRs that have TARGET_EXPR_INITIAL
AGGR_INIT_EXPR, but avoid those that have TARGET_EXPR_INITIAL a CONSTRUCTOR, or
a CONSTRUCTOR with certain flags, or have some flags on TARGET_EXPRs?

Tried:
--- gcc/cp/tree.c.jj2018-03-06 08:01:37.851883447 +0100
+++ gcc/cp/tree.c   2018-03-06 16:07:53.296061505 +0100
@@ -3165,6 +3165,13 @@ replace_placeholders_r (tree* t, int* wa
break;
   }

+case TARGET_EXPR:
+  /* Don't mess with placeholders in finish_compound_literal
+created TARGET_EXPRs.  */
+  if (TREE_CODE (TARGET_EXPR_INITIAL (*t)) == CONSTRUCTOR)
+   *walk_subtrees = false;
+  break;
+
 default:
   if (d->pset->add (*t))
*walk_subtrees = false;
but that FAILs nsdmi13.C, where the TARGET_EXPR is created by:
#4  0x015c5ff9 in make_node (code=TARGET_EXPR) at ../../gcc/tree.c:1053
#5  0x015d49ca in build4 (code=TARGET_EXPR, tt=, arg0=, 
arg1=, arg2=, arg3=) at
../../gcc/tree.c:4797
#6  0x00ace5e4 in build_target_expr (decl=,
value=, complain=3)
at ../../gcc/cp/tree.c:454
#7  0x00acfaee in force_target_expr (type=, init=, complain=3)
at ../../gcc/cp/tree.c:782
#8  0x00acfa92 in build_target_expr_with_type (init=, type=, complain=3)
at ../../gcc/cp/tree.c:766
#9  0x00acfc0e in get_target_expr_sfinae (init=, complain=3) at ../../gcc/cp/tree.c:797
#10 0x00821df6 in convert_like_real (convs=0x2e5f280, expr=, fn=, 
argnum=0, issue_conversion_warnings=true, c_cast_p=false, complain=3) at
../../gcc/cp/call.c:6902
#11 0x00821ede in convert_like_real (convs=0x2e5f2b0, expr=, fn=, 
argnum=0, issue_conversion_warnings=true, c_cast_p=false, complain=3) at
../../gcc/cp/call.c:6912
#12 0x00826426 in build_over_call (cand=0x2e5f2e0, flags=1, complain=3)
at ../../gcc/cp/call.c:7945
#13 0x0082d3fb in build_new_method_call_1 (instance=, fns=,

[Bug c++/79937] [6/7/8 Regression] ICE in replace_placeholders_r

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79937

--- Comment #12 from Jakub Jelinek  ---
Created attachment 43577
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=43577=edit
gcc8-pr79937.patch

My #c8 patch doesn't work at all, but this one at least fixes the two testcases
(but indeed doesn't fix one where bar returns X rather than C).

[Bug c++/79937] [6/7/8 Regression] ICE in replace_placeholders_r

[jason at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79937

Jason Merrill  changed:

   What|Removed |Added

   Assignee|jason at gcc dot gnu.org   |unassigned at gcc dot 
gnu.org

--- Comment #11 from Jason Merrill  ---
Consider also

struct X
{
  unsigned i;
  unsigned n = bar(X{2,i});
};

which would also have an X PLACEHOLDER_EXPR, but it would refer to the outer X
rather than the temporary.

It seems like maybe we need to defer NSDMI expansion in aggregate
initialization until we know which object we're initializing.

But Jakub's patch is an improvement over what we have now, and may be the right
choice for GCC 8.

[Bug c++/79937] [6/7/8 Regression] ICE in replace_placeholders_r

[jason at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79937

Jason Merrill  changed:

   What|Removed |Added

   Assignee|unassigned at gcc dot gnu.org  |jason at gcc dot gnu.org

--- Comment #10 from Jason Merrill  ---
Here's a wrong-code version that isn't fixed by Jakub's patch:

struct X {
  unsigned i;
  unsigned n = i;
};

X bar(X x) {
  return x;
}

int main()
{
  X x { 1, bar(X{2}).n };
  if (x.n != 2)
__builtin_abort();
}

though this isn't a regression.

[Bug c++/79937] [6/7/8 Regression] ICE in replace_placeholders_r

[paolo.carlini at oracle dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79937

Paolo Carlini  changed:

   What|Removed |Added

 CC||paolo.carlini at oracle dot com

--- Comment #9 from Paolo Carlini  ---
Note that the ICE in PR82410 happens in exactly the same way and Jakub's few
lines avoid that one too. Shall we further pursue the approach? Jason?

[Bug c++/79937] [6/7/8 Regression] ICE in replace_placeholders_r

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79937

Jakub Jelinek  changed:

   What|Removed |Added

 CC||jakub at gcc dot gnu.org

--- Comment #8 from Jakub Jelinek  ---
--- gcc/cp/tree.c.jj2018-01-11 18:58:48.348391787 +0100
+++ gcc/cp/tree.c   2018-01-16 17:15:52.510371663 +0100
@@ -3101,7 +3101,12 @@ replace_placeholders_r (tree* t, int* wa
for (; !same_type_ignoring_top_level_qualifiers_p (TREE_TYPE (*t),
   TREE_TYPE (x));
 x = TREE_OPERAND (x, 0))
- gcc_assert (TREE_CODE (x) == COMPONENT_REF);
+ if (TREE_CODE (x) != COMPONENT_REF)
+   {
+ /* PLACEHOLDER_EXPR for some other object.  */
+ *walk_subtrees = false;
+ break;
+   }
*t = x;
*walk_subtrees = false;
d->seen = true;
avoids the ICE, but whether it is correct or not I have no idea.  So, not
really working on this.

[Bug c++/79937] [6/7/8 Regression] ICE in replace_placeholders_r

[paolo.carlini at oracle dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79937

Paolo Carlini  changed:

   What|Removed |Added

 CC|paolo.carlini at oracle dot com|

--- Comment #7 from Paolo Carlini  ---
Thanks Marek, no problem. Just wanted to avoid somebody else (not me, at the
moment) to be prevented from working on a fix.

[Bug c++/79937] [6/7/8 Regression] ICE in replace_placeholders_r

[mpolacek at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79937

Marek Polacek  changed:

   What|Removed |Added

 Status|ASSIGNED|NEW
   Assignee|mpolacek at gcc dot gnu.org|unassigned at gcc dot 
gnu.org

--- Comment #6 from Marek Polacek  ---
Hi, not right now, and I won't get to this soon.  Sorry.

[Bug c++/79937] [6/7/8 Regression] ICE in replace_placeholders_r

[paolo.carlini at oracle dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79937

Paolo Carlini  changed:

   What|Removed |Added

 CC||paolo.carlini at oracle dot com

--- Comment #5 from Paolo Carlini  ---
Hi Marek, are you still working on this?