From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 51738 invoked by alias); 14 Dec 2018 23:05:50 -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 51725 invoked by uid 89); 14 Dec 2018 23:05:49 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= 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; Fri, 14 Dec 2018 23:05:44 +0000 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1E2E032F95 for ; Fri, 14 Dec 2018 23:05:43 +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 8F6636374B; Fri, 14 Dec 2018 23:05:42 +0000 (UTC) Received: from livre (livre.home [172.31.160.2]) by free.home (8.15.2/8.15.2) with ESMTP id wBEN5S14361609; Fri, 14 Dec 2018 21:05:28 -0200 From: Alexandre Oliva To: Jason Merrill Cc: gcc-patches@gcc.gnu.org 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: Fri, 14 Dec 2018 23:05:00 -0000 In-Reply-To: <12883fc9-4e44-e16d-fdc6-9a90c21bf01c@redhat.com> (Jason Merrill's message of "Fri, 14 Dec 2018 17:44:47 -0500") 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/msg01123.txt.bz2 On Dec 14, 2018, Jason Merrill wrote: > Let's move the initialization of "fields" inside the 'then' block here > with the initialization of "cvquals", rather than clear it in the > 'else'. We'd still have to NULL-initialize it somewhere, so I'd rather just move the entire loop into the conditional, and narrow the scope of variables only used within the loop, like this. The full patch below is very hard to read because of the reindentation, so here's a diff -b. diff --git a/gcc/cp/method.c b/gcc/cp/method.c index fd023e200538..17404a65b0fd 100644 --- a/gcc/cp/method.c +++ b/gcc/cp/method.c @@ -675,12 +675,9 @@ do_build_copy_constructor (tree fndecl) } else { - tree fields =3D TYPE_FIELDS (current_class_type); tree member_init_list =3D NULL_TREE; - int cvquals =3D cp_type_quals (TREE_TYPE (parm)); int i; tree binfo, base_binfo; - tree init; vec *vbases; =20 /* Initialize all the base-classes with the parameter converted @@ -704,15 +701,18 @@ do_build_copy_constructor (tree fndecl) inh, member_init_list); } =20 - for (; fields; fields =3D DECL_CHAIN (fields)) + if (!inh) + { + int cvquals =3D cp_type_quals (TREE_TYPE (parm)); + + for (tree fields =3D TYPE_FIELDS (current_class_type); + fields; fields =3D DECL_CHAIN (fields)) { tree field =3D fields; tree expr_type; =20 if (TREE_CODE (field) !=3D FIELD_DECL) continue; - if (inh) - continue; =20 expr_type =3D TREE_TYPE (field); if (DECL_NAME (field)) @@ -742,7 +742,7 @@ do_build_copy_constructor (tree fndecl) expr_type =3D cp_build_qualified_type (expr_type, quals); } =20 - init =3D build3 (COMPONENT_REF, expr_type, parm, field, NULL_TREE); + tree init =3D build3 (COMPONENT_REF, expr_type, parm, field, NULL_T= REE); if (move_p && !TYPE_REF_P (expr_type) /* 'move' breaks bit-fields, and has no effect for scalars. */ && !scalarish_type_p (expr_type)) @@ -751,6 +751,8 @@ do_build_copy_constructor (tree fndecl) =20 member_init_list =3D tree_cons (field, init, member_init_list); } + } + finish_mem_initializers (member_init_list); } } @@ -891,6 +893,7 @@ synthesize_method (tree fndecl) =20 /* Reset the source location, we might have been previously deferred, and thus have saved where we were first needed. */ + if (!DECL_INHERITED_CTOR (fndecl)) DECL_SOURCE_LOCATION (fndecl) =3D DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (fndecl))); =20 Is this OK too? (pending regstrapping) [PR c++/88146] do not crash synthesizing inherited ctor(...) This patch started out from the testcase in PR88146, that attempted to synthesize an inherited ctor without any args before a varargs ellipsis and crashed while at that, because of the unguarded dereferencing of the parm type list, that usually contains a terminator. The terminator is not there for varargs functions, however, and without any other args, we ended up dereferencing a NULL pointer. Oops. Guarding accesses to parm would be easy, but not necessary. In do_build_copy_constructor, non-inherited ctors are copy-ctors, that always have at least one parm, so parm needs not be guarded when we know the access will only take place when we're dealing with an inherited ctor. The only other problematic use was in the cvquals initializer, a variable only used in a loop over fields, that we skipped individually in inherited ctors. I've guarded the cvquals initialization and the entire loop over fields so they only run for copy-ctors. Avoiding the crash from unguarded accesses was easy, but I thought we should still produce the sorry message we got in other testcases that passed arguments through the ellipsis in inherited ctors. I put a check in, and noticed the inherited ctors were synthesized with the location assigned to the class name, although they were initially assigned the location of the using declaration. I decided the latter was better, and arranged for the better location to be retained. Further investigation revealed the lack of a sorry message had to do with the call being in a non-evaluated context, in this case, a noexcept expression. The sorry would be correctly reported in other contexts, so I rolled back the check I'd added, but retained the source location improvement. I was still concerned about issuing sorry messages while instantiating template ctors even in non-evaluated contexts, e.g., if a template ctor had a base initializer that used an inherited ctor with enough arguments that they'd go through an ellipsis. I wanted to defer the instantiation of such template ctors, but that would have been wrong for constexpr template ctors, and already done for non-constexpr ones. So, I just consolidated multiple test variants into a single testcase that explores and explains various of the possibilities I thought of. for gcc/cp/ChangeLog PR c++/88146 * method.c (do_build_copy_constructor): Guard cvquals init and loop over fields to run for non-inherited ctors only. (synthesize_method): Retain location of inherited ctor. for gcc/testsuite/ChangeLog PR c++/88146 * g++.dg/cpp0x/inh-ctor32.C: New. --- gcc/cp/method.c | 89 ++++++------ gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C | 229 +++++++++++++++++++++++++++= ++++ 2 files changed, 275 insertions(+), 43 deletions(-) create mode 100644 gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C diff --git a/gcc/cp/method.c b/gcc/cp/method.c index fd023e200538..17404a65b0fd 100644 --- a/gcc/cp/method.c +++ b/gcc/cp/method.c @@ -675,12 +675,9 @@ do_build_copy_constructor (tree fndecl) } else { - tree fields =3D TYPE_FIELDS (current_class_type); tree member_init_list =3D NULL_TREE; - int cvquals =3D cp_type_quals (TREE_TYPE (parm)); int i; tree binfo, base_binfo; - tree init; vec *vbases; =20 /* Initialize all the base-classes with the parameter converted @@ -704,53 +701,58 @@ do_build_copy_constructor (tree fndecl) inh, member_init_list); } =20 - for (; fields; fields =3D DECL_CHAIN (fields)) + if (!inh) { - tree field =3D fields; - tree expr_type; - - if (TREE_CODE (field) !=3D FIELD_DECL) - continue; - if (inh) - continue; + int cvquals =3D cp_type_quals (TREE_TYPE (parm)); =20 - expr_type =3D TREE_TYPE (field); - if (DECL_NAME (field)) + for (tree fields =3D TYPE_FIELDS (current_class_type); + fields; fields =3D DECL_CHAIN (fields)) { - if (VFIELD_NAME_P (DECL_NAME (field))) + tree field =3D fields; + tree expr_type; + + if (TREE_CODE (field) !=3D FIELD_DECL) continue; - } - else if (ANON_AGGR_TYPE_P (expr_type) && TYPE_FIELDS (expr_type)) - /* Just use the field; anonymous types can't have - nontrivial copy ctors or assignment ops or this - function would be deleted. */; - else - continue; =20 - /* Compute the type of "init->field". If the copy-constructor - parameter is, for example, "const S&", and the type of - the field is "T", then the type will usually be "const - T". (There are no cv-qualified variants of reference - types.) */ - if (!TYPE_REF_P (expr_type)) - { - int quals =3D cvquals; + expr_type =3D TREE_TYPE (field); + if (DECL_NAME (field)) + { + if (VFIELD_NAME_P (DECL_NAME (field))) + continue; + } + else if (ANON_AGGR_TYPE_P (expr_type) && TYPE_FIELDS (expr_type)) + /* Just use the field; anonymous types can't have + nontrivial copy ctors or assignment ops or this + function would be deleted. */; + else + continue; =20 - if (DECL_MUTABLE_P (field)) - quals &=3D ~TYPE_QUAL_CONST; - quals |=3D cp_type_quals (expr_type); - expr_type =3D cp_build_qualified_type (expr_type, quals); - } + /* Compute the type of "init->field". If the copy-constructor + parameter is, for example, "const S&", and the type of + the field is "T", then the type will usually be "const + T". (There are no cv-qualified variants of reference + types.) */ + if (!TYPE_REF_P (expr_type)) + { + int quals =3D cvquals; =20 - init =3D build3 (COMPONENT_REF, expr_type, parm, field, NULL_TREE); - if (move_p && !TYPE_REF_P (expr_type) - /* 'move' breaks bit-fields, and has no effect for scalars. */ - && !scalarish_type_p (expr_type)) - init =3D move (init); - init =3D build_tree_list (NULL_TREE, init); + if (DECL_MUTABLE_P (field)) + quals &=3D ~TYPE_QUAL_CONST; + quals |=3D cp_type_quals (expr_type); + expr_type =3D cp_build_qualified_type (expr_type, quals); + } + + tree init =3D build3 (COMPONENT_REF, expr_type, parm, field, NULL_T= REE); + if (move_p && !TYPE_REF_P (expr_type) + /* 'move' breaks bit-fields, and has no effect for scalars. */ + && !scalarish_type_p (expr_type)) + init =3D move (init); + init =3D build_tree_list (NULL_TREE, init); =20 - member_init_list =3D tree_cons (field, init, member_init_list); + member_init_list =3D tree_cons (field, init, member_init_list); + } } + finish_mem_initializers (member_init_list); } } @@ -891,8 +893,9 @@ synthesize_method (tree fndecl) =20 /* Reset the source location, we might have been previously deferred, and thus have saved where we were first needed. */ - DECL_SOURCE_LOCATION (fndecl) - =3D DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (fndecl))); + if (!DECL_INHERITED_CTOR (fndecl)) + DECL_SOURCE_LOCATION (fndecl) + =3D DECL_SOURCE_LOCATION (TYPE_NAME (DECL_CONTEXT (fndecl))); =20 /* If we've been asked to synthesize a clone, just synthesize the cloned function instead. Doing so will automatically fill in the diff --git a/gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C b/gcc/testsuite/g++.dg= /cpp0x/inh-ctor32.C new file mode 100644 index 000000000000..c40412fc5346 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/inh-ctor32.C @@ -0,0 +1,229 @@ +// { dg-do compile { target c++11 } } +// Minimized from the testcase for PR c++/88146, +// then turned into multiple variants.=20 + +// We issue an error when calling an inherited ctor with at least one +// argument passed through a varargs ellipsis, if the call is in an +// evaluated context. Even in nonevaluated contexts, we will +// instantiate constexpr templates (unlike non-constexpr templates), +// which might then issue errors that in nonevlauated contexts +// wouldn't be issued. + +// In these variants, the inherited ctor is constexpr, but it's only +// called in unevaluated contexts, so no error is issued. The +// templateness of the ctor doesn't matter, because the only call that +// passes args through the ellipsis is in a noexcept expr, that is not +// evaluated. The ctors in derived classes are created and +// instantiated, discarding arguments passed through the ellipsis when +// calling base ctors, but that's not reported: we only report a +// problem when *calling* ctors that behave this way. +namespace unevaled_call { + namespace no_arg_before_ellipsis { + namespace without_template { + struct foo { + constexpr foo(...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0})); + } + + namespace with_template { + struct foo { + template + constexpr foo(...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0})); + } + } + + namespace one_arg_before_ellipsis { + namespace without_template { + struct foo { + constexpr foo(int, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0,1})); + } + + namespace with_template { + struct foo { + template + constexpr foo(T, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0,1})); + } + } +} + +// In these variants, the inherited ctor is constexpr, and it's called +// in unevaluated contexts in ways that would otherwise trigger the +// sorry message. Here we check that the message is not issued at +// those calls, nor at subsequent calls that use the same ctor without +// passing arguments through its ellipsis. We check that it is issued +// later, when we pass the ctor arguments through the ellipsis. +namespace evaled_bad_call_in_u { + namespace one_arg_before_ellipsis { + namespace without_template { + struct foo { + constexpr foo(int, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0, 1})); + bar t(0); + bar u(0, 1); // { dg-message "sorry, unimplemented: passing argument= s to ellipsis" } + } + + namespace with_template { + struct foo { + template + constexpr foo(T, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0,1})); + bar t(0); + bar u(0,1); // { dg-message "sorry, unimplemented: passing arguments= to ellipsis" } + } + } + + namespace no_arg_before_ellipsis { + namespace without_template { + struct foo { + constexpr foo(...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0})); + bar u(0); // { dg-message "sorry, unimplemented: passing arguments t= o ellipsis" } + } + + namespace with_template { + struct foo { + template + constexpr foo(...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + using boo::boo; + }; + void f() noexcept(noexcept(bar{0})); + bar u(0); // { dg-message "sorry, unimplemented: passing arguments t= o ellipsis" } + } + } +} + +// Now, instead of instantiating a class that uses a derived ctor, we +// introduce another template ctor that will use the varargs ctor to +// initialize its base class. The idea is to verify that the error +// message is issued, even if the instantiation occurs in a +// nonevaluated context, e.g., for constexpr templates. In the +// inherited_derived_ctor, we check that even an inherited ctor of a +// constexpr ctor is instantiated and have an error message issued. +namespace derived_ctor { + namespace direct_derived_ctor { + namespace constexpr_noninherited_ctor { + struct foo { + template + constexpr foo(T, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + template + constexpr bar(T ... args) : boo(args...) {} // { dg-message "sorry, unimp= lemented: passing arguments to ellipsis" } + }; + void f() noexcept(noexcept(bar{0,1})); + } + + namespace no_constexpr_noninherited_ctor { + struct foo { + template + constexpr foo(T, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bar : boo { + template + /* constexpr */ bar(T ... args) : boo(args...) {} + }; + void f() noexcept(noexcept(bar{0,1})); + } + } + + namespace inherited_derived_ctor { + namespace constexpr_noninherited_ctor { + struct foo { + template + constexpr foo(T, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bor : boo { + template + constexpr bor(T ... args) : boo(args...) {} // { dg-message "sorry, unimp= lemented: passing arguments to ellipsis" } + }; + struct bar : bor { + using bor::bor; + }; + void f() noexcept(noexcept(bar{0,1})); // { dg-message "'constexpr' = expansion" } + } + + namespace no_constexpr_noninherited_ctor { + struct foo { + template + constexpr foo(T, ...) {} + }; + struct boo : foo { + using foo::foo; + }; + struct bor : boo { + template + /* constexpr */ bor(T ... args) : boo(args...) {} + }; + struct bar : bor { + using bor::bor; + }; + void f() noexcept(noexcept(bar{0,1})); + } + } +} --=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