From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14052 invoked by alias); 3 Apr 2017 11:55:33 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 14028 invoked by uid 89); 3 Apr 2017 11:55:31 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=00pm, 30pm, slide, dmi X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 03 Apr 2017 11:55:29 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9B0D2155E4 for ; Mon, 3 Apr 2017 11:55:27 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 9B0D2155E4 Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=polacek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 9B0D2155E4 Received: from redhat.com (ovpn-204-110.brq.redhat.com [10.40.204.110]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v33BtD2e024523 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Mon, 3 Apr 2017 07:55:17 -0400 Date: Mon, 03 Apr 2017 11:55:00 -0000 From: Marek Polacek To: Jason Merrill Cc: GCC Patches Subject: Re: C++ PATCH to fix ICE in replace_placeholders_r (PR c++/79937) Message-ID: <20170403115511.GY3172@redhat.com> References: <20170307171047.GW3172@redhat.com> <20170323203411.GZ3172@redhat.com> <20170324162200.GA3172@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170324162200.GA3172@redhat.com> User-Agent: Mutt/1.7.1 (2016-10-04) X-SW-Source: 2017-04/txt/msg00075.txt.bz2 Ping. Any ideas how to move this forward? On Fri, Mar 24, 2017 at 05:22:00PM +0100, Marek Polacek wrote: > On Thu, Mar 23, 2017 at 05:09:58PM -0400, Jason Merrill wrote: > > On Thu, Mar 23, 2017 at 4:34 PM, Marek Polacek wrote: > > > On Tue, Mar 14, 2017 at 02:34:30PM -0400, Jason Merrill wrote: > > >> On Tue, Mar 14, 2017 at 2:33 PM, Jason Merrill wrote: > > >> > On Tue, Mar 7, 2017 at 12:10 PM, Marek Polacek wrote: > > >> >> In this testcase we have > > >> >> C c = bar (X{1}); > > >> >> which store_init_value sees as > > >> >> c = TARGET_EXPR )->i}>)> > > >> >> i.e. we're initializing "c" with a TARGET_EXPR. We call replace_placeholders > > >> >> that walks the whole tree to substitute the placeholders. Eventually we find > > >> >> the nested but that's for another object, so we > > >> >> crash. Seems that we shouldn't have stepped into the second TARGET_EXPR at > > >> >> all; it has nothing to with "c", it's bar's argument. > > >> >> > > >> >> It occurred to me that we shouldn't step into CALL_EXPRs and leave the > > >> >> placeholders in function arguments to cp_gimplify_init_expr which calls > > >> >> replace_placeholders for constructors. Not sure if it's enough to handle > > >> >> CALL_EXPRs like this, anything else? > > >> > > > >> > Hmm, we might have a DMI containing a call with an argument referring > > >> > to *this, i.e. > > >> > > > >> > struct A > > >> > { > > >> > int i; > > >> > int j = frob (this->i); > > >> > }; > > >> > > > >> > The TARGET_EXPR seems like a more likely barrier, but even there we > > >> > could have something like > > >> > > > >> > struct A { int i; }; > > >> > struct B > > >> > { > > >> > int i; > > >> > A a = A{this->i}; > > >> > }; > > >> > > > >> > I think we need replace_placeholders to keep a stack of objects, so > > >> > that when we see a TARGET_EXPR we add it to the stack and therefore > > >> > can properly replace a PLACEHOLDER_EXPR of its type. > > >> > > >> Or actually, avoid replacing such a PLACEHOLDER_EXPR, but rather leave > > >> it for later when we lower the TARGET_EXPR. > > > > > > Sorry, I don't really follow. I have a patch that puts TARGET_EXPRs on > > > a stack, but I don't know how that helps. E.g. with nsdmi-aggr3.C > > > we have > > > B b = TARGET_EXPR >}> > > > so when we get to that PLACEHOLDER_EXPR, on the stack there's > > > TARGET_EXPR with type struct A > > > TARGET_EXPR with type struct B > > > so the type of the PLACEHOLDER_EXPR doesn't match the type of the current > > > TARGET_EXPR, but we still want to replace it in this case. > > > > > > So -- could you expand a bit on what you had in mind, please? > > > > So then when we see a placeholder, we walk the stack to find the > > object of the matching type. > > > > But if the object we find was collected from walking through a > > TARGET_EXPR, we should leave the PLACEHOLDER_EXPR alone, so that it > > can be replaced later with the actual target of the initialization. > > Unfortunately, I still don't understand; guess I'll have to drop this PR. > > With this we put TARGET_EXPRs on a stack, and then when we find a > PLACEHOLDER_EXPR we walk the stack to find a TARGET_EXPR of the same type as > the PLACEHOLDER_EXPR. There are three simplified examples I've been playing > with: > > B b = T_E >}> > > - here we should replace the P_E; on the stack there are two > TARGET_EXPRs of types B and A > > C c = T_E >)> > > - here we shouldn't replace the P_E; on the stack there are two > TARGET_EXPRs of types X and C > > B b = T_E }}> > > - here we should replace the P_E; on the stack there's one TARGET_EXPR > of type B > > In each case we find a TARGET_EXPR of the type of the PLACEHOLDER_EXPR, but I > don't see how to decide which PLACEHOLDER_EXPR we should let slide. Sorry for > being dense... > > diff --git gcc/cp/tree.c gcc/cp/tree.c > index 2757af6..2439a00 100644 > --- gcc/cp/tree.c > +++ gcc/cp/tree.c > @@ -2741,8 +2741,12 @@ build_ctor_subob_ref (tree index, tree type, tree obj) > > struct replace_placeholders_t > { > - tree obj; /* The object to be substituted for a PLACEHOLDER_EXPR. */ > - bool seen; /* Whether we've encountered a PLACEHOLDER_EXPR. */ > + /* The object to be substituted for a PLACEHOLDER_EXPR. */ > + tree obj; > + /* Whether we've encountered a PLACEHOLDER_EXPR. */ > + bool seen; > + /* A stack of TARGET_EXPRs we've found ourselves in. */ > + vec target_expr_stack; > }; > > /* Like substitute_placeholder_in_expr, but handle C++ tree codes and > @@ -2762,14 +2766,35 @@ replace_placeholders_r (tree* t, int* walk_subtrees, void* data_) > > switch (TREE_CODE (*t)) > { > + case TARGET_EXPR: > + d->target_expr_stack.safe_push (*t); > + cp_walk_tree (&TARGET_EXPR_INITIAL (*t), replace_placeholders_r, data_, > + NULL); > + d->target_expr_stack.pop (); > + *walk_subtrees = 0; > + break; > + > case PLACEHOLDER_EXPR: > { > - tree x = obj; > - 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); > - *t = x; > + bool skip_it = false; > + unsigned ix; > + tree targ; > + /* Walk the stack to find the object of the matching type. */ > + FOR_EACH_VEC_ELT_REVERSE (d->target_expr_stack, ix, targ) > + if (same_type_ignoring_top_level_qualifiers_p > + (TREE_TYPE (*t), TREE_TYPE (targ))) > + { > + // ... > + } > + if (!skip_it) > + { > + tree x = obj; > + 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); > + *t = x; > + } > *walk_subtrees = false; > d->seen = true; > } > @@ -2813,14 +2838,23 @@ replace_placeholders_r (tree* t, int* walk_subtrees, void* data_) > return NULL_TREE; > } > > +/* Replace PLACEHOLDER_EXPRs in EXP with object OBJ. */ > + > tree > replace_placeholders (tree exp, tree obj, bool *seen_p) > { > tree *tp = &exp; > - replace_placeholders_t data = { obj, false }; > + replace_placeholders_t data; > + data.obj = obj; > + data.seen = false; > + data.target_expr_stack.create (0); > if (TREE_CODE (exp) == TARGET_EXPR) > - tp = &TARGET_EXPR_INITIAL (exp); > + { > + tp = &TARGET_EXPR_INITIAL (exp); > + data.target_expr_stack.safe_push (exp); > + } > cp_walk_tree (tp, replace_placeholders_r, &data, NULL); > + data.target_expr_stack.release (); > if (seen_p) > *seen_p = data.seen; > return exp; > > Marek Marek