From patchwork Tue Apr 25 16:17:47 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Marek Polacek X-Patchwork-Id: 754914 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3wC7fh2sSwz9sCZ for ; Wed, 26 Apr 2017 02:18:03 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="OpLTDBzi"; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; q=dns; s=default; b=cdx8QIZOAO9PCGhzK TXMaE/1ZoKYjRItInwoy4hxSCrNRjH7lD6R0vdr0k0le9JsXyf2y8GTFYrft2LFp 24WB3LSR/fev4jiHwwPzkICxTDeDY82+tOiBOY+qoG7t+shyxls/V4ixNca16TwV S3EkLUuMaIMWkZbRcxWSWM1wf0= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=default; bh=nk6VZnun03jhaifj3yTkSST Ibjw=; b=OpLTDBzik78nxSAJcKKgynt671nKr+866eZUevAoJ4a1DcYBGJP8Aj/ pVFwrEyAbEqRVc1zF6UJYCXYQYdtyB6kvlp0klRlcE91VfqYhZTod3Z7pcrYhAQB UTUVxTBG4nCTcTgIGGbqeeo0CrSm4juKZPeSND2Zi4MVYU5tnacw= Received: (qmail 98735 invoked by alias); 25 Apr 2017 16:17:54 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 98664 invoked by uid 89); 25 Apr 2017 16:17:53 -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=30pm, occurred, slide 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; Tue, 25 Apr 2017 16:17:51 +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 1877B65D1A for ; Tue, 25 Apr 2017 16:17:52 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 1877B65D1A Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=polacek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 1877B65D1A Received: from redhat.com ([10.40.205.13]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v3PGHmlD031050 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Tue, 25 Apr 2017 12:17:50 -0400 Date: Tue, 25 Apr 2017 18:17:47 +0200 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: <20170425161747.GV4255@redhat.com> References: <20170307171047.GW3172@redhat.com> <20170323203411.GZ3172@redhat.com> <20170324162200.GA3172@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.8.0 (2017-02-23) On Fri, Apr 07, 2017 at 03:27:36PM -0400, Jason Merrill wrote: > On Fri, Mar 24, 2017 at 12:22 PM, 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... > > I was thinking that we want to replace the type of the first entry in > the stack (B, C, B respectively), and leave others alone. Even that didn't work for me, because we may end up with a case of C c = bar (T_E >) Oh well. So I abandoned the idea of stacks and tried this straightforward approach, which seems to work fine: Bootstrapped/regtested on x86_64-linux, ok for trunk? 2017-04-25 Marek Polacek PR c++/79937 * tree.c (replace_placeholders_r): Don't substitute an object of unrelated type. * g++.dg/cpp1y/nsdmi-aggr10.C: New test. * g++.dg/cpp1y/nsdmi-aggr9.C: New test. Marek diff --git gcc/cp/tree.c gcc/cp/tree.c index 15b3ad9..5391df8 100644 --- gcc/cp/tree.c +++ gcc/cp/tree.c @@ -2789,11 +2789,20 @@ replace_placeholders_r (tree* t, int* walk_subtrees, void* data_) 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 = false; + while (!(same_type_ignoring_top_level_qualifiers_p + (TREE_TYPE (*t), TREE_TYPE (x)))) + if (TREE_CODE (x) != COMPONENT_REF) + { + /* An object of unrelated type; don't substitute. */ + skip = true; + break; + } + else + x = TREE_OPERAND (x, 0); + + if (!skip) + *t = x; *walk_subtrees = false; d->seen = true; } diff --git gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr10.C gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr10.C index e69de29..1e051d8 100644 --- gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr10.C +++ gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr10.C @@ -0,0 +1,16 @@ +// PR c++/79937 +// { dg-do compile { target c++14 } } + +extern int frob (int); + +struct A +{ + int i; + int j = frob (this->i); +}; + +void +foo () +{ + A a = { 1 }; +} diff --git gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr9.C gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr9.C index e69de29..c2fd404 100644 --- gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr9.C +++ gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr9.C @@ -0,0 +1,21 @@ +// PR c++/79937 +// { dg-do compile { target c++14 } } + +struct C {}; + +struct X { + unsigned i; + unsigned n = i; +}; + +C +bar (X) +{ + return {}; +} + +void +foo () +{ + C c = bar (X{1}); +}