From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 129249 invoked by alias); 20 Dec 2018 00:04:49 -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 129230 invoked by uid 89); 20 Dec 2018 00:04:48 -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,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=portions, sk:bootstr 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; Thu, 20 Dec 2018 00:04:46 +0000 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C6214C028351; Thu, 20 Dec 2018 00:04:44 +0000 (UTC) Received: from free.home (ovpn04.gateway.prod.ext.phx2.redhat.com [10.5.9.4]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 3C55A19745; Thu, 20 Dec 2018 00:04:43 +0000 (UTC) Received: from livre (livre.home [172.31.160.2]) by free.home (8.15.2/8.15.2) with ESMTP id wBK04CYU873209; Wed, 19 Dec 2018 22:04:12 -0200 From: Alexandre Oliva To: Christophe Lyon Cc: Jason Merrill , gcc-patches List Subject: Re: [C++ Patch] [PR c++/88146] do not crash synthesizing inherited ctor(...) References: <81ac6c22-8907-a166-b8df-80e06d2850da@redhat.com> <12883fc9-4e44-e16d-fdc6-9a90c21bf01c@redhat.com> Date: Thu, 20 Dec 2018 00:04:00 -0000 In-Reply-To: (Christophe Lyon's message of "Wed, 19 Dec 2018 15:36:17 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SW-Source: 2018-12/txt/msg01430.txt.bz2 Christophe, Thanks again for the report. This was quite an adventure to figure out ;-) See below. [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 implicit,= tsubst_flags_t complain) break; =20 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; =20 --=20 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