From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 95000 invoked by alias); 20 Dec 2018 15:28: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 94986 invoked by uid 89); 20 Dec 2018 15:28:32 -0000 Authentication-Results: sourceware.org; auth=none 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,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=Hx-spam-relays-external:sk:mail-ua, H*r:sk:mail-ua, H*RU:sk:mail-ua, HX-Received:ab0 X-HELO: mail-ua1-f51.google.com Received: from mail-ua1-f51.google.com (HELO mail-ua1-f51.google.com) (209.85.222.51) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 20 Dec 2018 15:28:30 +0000 Received: by mail-ua1-f51.google.com with SMTP id z24so684378ual.8 for ; Thu, 20 Dec 2018 07:28:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=kZlf6X3LjZkI6on9X3VDGoc1max7QlsgWDfpRx23fuc=; b=LVAGlypTkE4gmFEEsXZj75//AuXlSp7QYuFjyGkqnzfakY07oDyzZc3Gg8BFpIxBL+ OrdWfSHJ2Vg//R3h6Y93GpQHHsWQ+zAc3f0jCnvvezunkC5Mu654C+EHGkY4R6T/GRDm q4o4Eiuij8jooc2wucdfdXBMf7ysIllnlrFEc= MIME-Version: 1.0 References: <81ac6c22-8907-a166-b8df-80e06d2850da@redhat.com> <12883fc9-4e44-e16d-fdc6-9a90c21bf01c@redhat.com> In-Reply-To: From: Christophe Lyon Date: Thu, 20 Dec 2018 16:00:00 -0000 Message-ID: Subject: Re: [C++ Patch] [PR c++/88146] do not crash synthesizing inherited ctor(...) To: Alexandre Oliva Cc: Jason Merrill , gcc-patches List Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2018-12/txt/msg01472.txt.bz2 On Thu, 20 Dec 2018 at 01:04, Alexandre Oliva wrote: > > Christophe, > > Thanks again for the report. This was quite an adventure to figure > out ;-) See below. > Glad I've helped. I wouldn't have been able to do the analysis :) > > [PR88146] avoid diagnostics diffs if cdtor_returns_this > > Diagnostics for testsuite/g++.dg/cpp0x/inh-ctor32.C varied across > platforms. Specifically, on ARM, the diagnostics within the subtest > derived_ctor::inherited_derived_ctor::constexpr_noninherited_ctor did > not match those displayed on other platforms, and the test failed. > > The difference seemed to have to do with locations assigned to ctors, > but it was more subtle: on ARM, the instantiation of bor's template > ctor was nested within the instantiation of bar's template ctor > inherited from bor. The reason turned out to be related with the > internal return type of ctors: arm_cxx_cdtor_returns_this is enabled > for because of AAPCS, while cxx.cdtor_returns_this is disabled on most > other platforms. While convert_to_void returns early with a VOID > expr, the non-VOID return type of the base ctor CALL_EXPR causes > convert_to_void to inspect the called decl for nodiscard attributes: > maybe_warn_nodiscard -> cp_get_fndecl_from_callee -> > maybe_constant_init -> cxx_eval_outermost_constant_expr -> > instantiate_constexpr_fns -> nested instantiation. > > The internal return type assigned to a cdtor should not affect > instantiation (constexpr or template) decisions, IMHO. We know it > affects diagnostics, but I have a hunch this might bring deeper issues > with it, so I've arranged for the CALL_EXPR handler in convert_to_void > to disregard cdtors, regardless of the ABI. > > > The patch is awkward on purpose: it's meant to illustrate both > portions of the affected code, to draw attention to a potential > problem, and to get bootstrap-testing coverage for the path that will > be taken on ARM. I envision removing the first hunk, and the else > from the second hunk, once testing is done. > > The first hunk is there to highlight where convert_to_void returns > early on x86, instead of handling the CALL_EXPR. > > BTW (here's the potential problem), shouldn't we go into the CALL_EXPR > case for the volatile void mentioned in comments next to the case, or > won't that match VOID_TYPE_P? > > Finally, I shall mention the possibility of taking the opposite > direction, and actually looking for nodiscard in cdtor calls so as to > trigger the constexpr side effects that we've inadvertently triggered > and observed with the inh-ctor32.C testcase. It doesn't feel right to > me, but I've been wrong many times before ;-) > > Would a rearranged version of the patch, dropping the redundant tests > and retaining only the addition of the test for cdtor identifiers, be > ok to install, provided that it passes regression testing? > > > Note this patch does NOT carry a ChangeLog entry. That's also on > purpose, to indicate it's not meant to be included as is. > --- > gcc/cp/cvt.c | 21 ++++++++++++++++++++- > 1 file changed, 20 insertions(+), 1 deletion(-) > > diff --git a/gcc/cp/cvt.c b/gcc/cp/cvt.c > index eb1687377c3e..1a15af8a6e99 100644 > --- a/gcc/cp/cvt.c > +++ b/gcc/cp/cvt.c > @@ -1112,7 +1112,8 @@ convert_to_void (tree expr, impl_conv_void implicit= , tsubst_flags_t complain) > error_at (loc, "pseudo-destructor is not called"); > return error_mark_node; > } > - if (VOID_TYPE_P (TREE_TYPE (expr))) > + if (VOID_TYPE_P (TREE_TYPE (expr)) > + && TREE_CODE (expr) !=3D CALL_EXPR) > return expr; > switch (TREE_CODE (expr)) > { > @@ -1169,6 +1170,24 @@ convert_to_void (tree expr, impl_conv_void implici= t, tsubst_flags_t complain) > break; > > case CALL_EXPR: /* We have a special meaning for volatile void fn(= ). */ > + /* cdtors may return this or void, depending on > + targetm.cxx.cdtor_returns_this, but this shouldn't affect our > + decisions here: nodiscard cdtors are nonsensical, and we > + don't want to call maybe_warn_nodiscard because it may > + trigger constexpr or template instantiation in a way that > + changes their instantiaton nesting. This changes the way > + contexts are printed in diagnostics, with bad consequences > + for the testsuite, but there may be other undesirable > + consequences of visiting referenced ctors too soon. */ > + if (DECL_P (TREE_OPERAND (expr, 0)) > + && IDENTIFIER_CDTOR_P (DECL_NAME (TREE_OPERAND (expr, 0)))) > + return expr; > + /* FIXME: Move this test before the one above, after a round of > + testing as it is, to get coverage of the behavior we'd get on > + ARM. */ > + else if (VOID_TYPE_P (TREE_TYPE (expr))) > + return expr; > + > maybe_warn_nodiscard (expr, implicit); > break; > > > > -- > Alexandre Oliva, freedom fighter https://FSFLA.org/blogs/lxo > Be the change, be Free! FSF Latin America board member > GNU Toolchain Engineer Free Software Evangelist > Hay que enGNUrecerse, pero sin perder la terGNUra jam=C3=A1s-GNUChe