From patchwork Thu Aug 8 02:00:50 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Nathaniel Shead X-Patchwork-Id: 1970305 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256 header.s=20230601 header.b=PAg18tFn; dkim-atps=neutral Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=2620:52:3:1:0:246e:9693:128c; helo=server2.sourceware.org; envelope-from=gcc-patches-bounces~incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=patchwork.ozlabs.org) Received: from server2.sourceware.org (server2.sourceware.org [IPv6:2620:52:3:1:0:246e:9693:128c]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (secp384r1) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4WfVdj2b1cz1ybS for ; Thu, 8 Aug 2024 12:01:26 +1000 (AEST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 524C53858C41 for ; Thu, 8 Aug 2024 02:01:23 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mail-pf1-x431.google.com (mail-pf1-x431.google.com [IPv6:2607:f8b0:4864:20::431]) by sourceware.org (Postfix) with ESMTPS id 7E90A3858CD9 for ; Thu, 8 Aug 2024 02:00:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 7E90A3858CD9 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 7E90A3858CD9 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::431 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1723082460; cv=none; b=PNsKCbx5rH/Erd3JOBIgiIbKWSSYgndWXys4qwenon4I7ms/V0qGZQmDFJn8wpB3N2DQb0LNRGoRygA/JDBMBZsP3CIM3+TTG+/3mm1SI3hKCkSCc5JfdN+vaLdwJ04pftSxnx8gWOqWm5Aa7zWKIRVzuuMKgdAFvtNflDE33EA= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1723082460; c=relaxed/simple; bh=SMC1Fjtjmc3YcMVBExm3Ms48F+VT5bH+dZee9xxqoiI=; h=DKIM-Signature:Message-ID:Date:From:To:Subject:MIME-Version; b=ugxqLo1tMmtSm5eNy1Av18Oiw7ZFZ30pUfJC0qhbgYShngI/xD2RLl2KjA4h+lSRKKL531ijmMW7i1to6R3KMHfiBQ2v1BePkcVOyssqycSnqk9rVPAqg9SwWJr9SYOIm7NwRN8wxlQiBeekocALZHG0GUXn6DXReLxn0cuank4= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-pf1-x431.google.com with SMTP id d2e1a72fcca58-70cec4aa1e4so382439b3a.1 for ; Wed, 07 Aug 2024 19:00:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1723082456; x=1723687256; darn=gcc.gnu.org; h=in-reply-to:content-disposition:mime-version:references:subject:cc :to:from:date:message-id:from:to:cc:subject:date:message-id:reply-to; bh=OnMbsC3j5P0nOrriop5K0mB/6iSMWJiai0GwFSkRimM=; b=PAg18tFnaXEEuQygyFSkq0ueqFip/fYHaI+fnfavdyVJV/MBSowyy8GHyhRzkI7siM VuwIuABk4A/hLSuzHWS8mWg4AAWSq2ODxmppD2dA94TX+XiBpYwelGid0e3BY4BP4pMY sSdqvCLF98Yn5GoHBKgj/DatlB58vJCYx6TsUYtQ0R3twCGKbwPzvokgKmHK1FaU8Bkm oin3p0kSwVfDt0axYBnf7Xrd4zIKEA2ogNFc0vqbZxD8qkrCaUbjFUnYBKxh+c2OzQtd HqOBLuDrzKiNjrjUVFMiD+mDVXS3tY8aLIb7yfIao7wVMg0dQ2KmlwsIQamHgd874U0F +cLQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1723082456; x=1723687256; h=in-reply-to:content-disposition:mime-version:references:subject:cc :to:from:date:message-id:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=OnMbsC3j5P0nOrriop5K0mB/6iSMWJiai0GwFSkRimM=; b=QonJMzVYZMsf8WKBmwtQyOSAvdZ8ZomBaYicziWX4Dm8PrTyGmdUCf9kERVHc0GPWt DU+nKP3fAoVJZL1AA9T/5yk2bMnaK2kmUnIaODLeRwSYKddBMlAy8mpEq/5yzVnCOTMH r/STxLHCWvtkEdMYTruK1nf7A1W9afRBlDTWzNMFLj6CSGQRFTpN+FIuhpZlb142IUPk 9QbCtI8bfHyqfu4CrP4F7Ax5RPciDvQPYf4nUZvDe4pJrlhCz4yIcr7t0bshRI0VxL2x h5a1wChVxFpatGjyNahrUsNWNNdBMQGH8P9+YYX8XQqCk8PkRsB8su8IDlf8dF6mJ6jp PCPQ== X-Forwarded-Encrypted: i=1; AJvYcCVmvUYv2xu3kV7k4OSDnXUxUJzBl24SGbxswk3FpOdHnyDvIxldCncfcH43mS4xaaDIey4Vn5rQZtvPgHkK/hYCBOu+i0Vp0Q== X-Gm-Message-State: AOJu0YyYl6AWYBMQ2P/RiabiEHSCaSGJ0mY3faXLesDpBWTfJpdQwepU 57eKCcCRuNpEADIt99N1whxQyktCFXwye/55GVmLPWSXlllzPKQPdypttw== X-Google-Smtp-Source: AGHT+IEfNs2fBlKDBr46iZMoVpd468yHJFA5oFbOlPS9QMG8mtoSV8koL6BwgRic3mFg9rArzn/4DQ== X-Received: by 2002:a05:6a00:6f27:b0:70d:33b3:2d7f with SMTP id d2e1a72fcca58-710cae6f965mr694256b3a.26.1723082456279; Wed, 07 Aug 2024 19:00:56 -0700 (PDT) Received: from Thaum. (110-174-164-242.static.tpgi.com.au. [110.174.164.242]) by smtp.gmail.com with ESMTPSA id d2e1a72fcca58-710cb20a049sm165050b3a.10.2024.08.07.19.00.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 Aug 2024 19:00:55 -0700 (PDT) Message-ID: <66b426d7.050a0220.2958fc.0959@mx.google.com> X-Google-Original-Message-ID: Date: Thu, 8 Aug 2024 12:00:50 +1000 From: Nathaniel Shead To: Patrick Palka Cc: Jason Merrill , gcc-patches@gcc.gnu.org, Nathan Sidwell Subject: [PATCH v3] c++/modules: Handle instantiating already tsubsted template friend classes [PR115801] References: <66b1c42f.650a0220.209cec.04ea@mx.google.com> <66b40711.170a0220.19ae25.4bc6@mx.google.com> <46841077-6de8-d65b-1df5-d8e5d30200bc@idea> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-Spam-Status: No, score=-12.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gcc-patches-bounces~incoming=patchwork.ozlabs.org@gcc.gnu.org On Wed, Aug 07, 2024 at 09:12:13PM -0400, Patrick Palka wrote: > On Wed, 7 Aug 2024, Patrick Palka wrote: > > > On Thu, 8 Aug 2024, Nathaniel Shead wrote: > > > > > On Wed, Aug 07, 2024 at 01:44:31PM -0400, Jason Merrill wrote: > > > > On 8/6/24 2:35 AM, Nathaniel Shead wrote: > > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? > > > > > > > > > > Another potential approach would be to go searching for this unexported > > > > > type and load it, either with a new LOOK_want::ANY_REACHABLE flag or by > > > > > expanding on the lookup_imported_temploid_friend hack. I'm still not > > > > > exactly sure how name lookup for template friends is supposed to behave > > > > > though, specifically in terms of when and where they should conflict > > > > > with other entities with the same name. > > > > > > > > CWG2588 tried to clarify this in https://eel.is/c++draft/basic#link-4.8 -- > > > > if there's a matching reachable declaration, the friend refers to it even if > > > > it isn't visible to name lookup. > > > > > > > > It seems like an oversight that the new second bullet refers specifically to > > > > functions, it seems natural for it to apply to classes as well. > > > > > > > > So, they correspond but do not conflict because they declare the same > > > > entity. > > > > > > > > > > Right, makes sense. OK, I'll work on filling out our testcases to make > > > sure that we cover everything under that interpretation and potentially > > > come back to making an ANY_REACHABLE flag for this. > > > > > > > > The relevant paragraphs seem to be https://eel.is/c++draft/temp.friend#2 > > > > > and/or https://eel.is/c++draft/dcl.meaning.general#2.2.2, in addition to > > > > > the usual rules in [basic.link] and [basic.scope.scope], but how these > > > > > all are supposed to interact isn't super clear to me right now. > > > > > > > > > > Additionally I wonder if maybe the better approach long-term would be to > > > > > focus on getting textual redefinitions working first, and then reuse > > > > > whatever logic we build for that to handle template friends rather than > > > > > relying on finding these hidden 'mergeable' slots first. > > > > > > > > I have a WIP patch to allow textual redefinitions by teaching > > > > duplicate_decls that it's OK to redefine an imported GM entity, so > > > > check_module_override works. > > > > > > > > My current plan is to then just token-skip the bodies. This won't diagnose > > > > ODR problems, but our module merging doesn't do that consistently either. > > > > > > > > > @@ -11800,6 +11800,15 @@ tsubst_friend_class (tree friend_tmpl, tree args) > > > > > input_location = saved_input_location; > > > > > } > > > > > } > > > > > + else if (TMPL_PARMS_DEPTH (DECL_TEMPLATE_PARMS (friend_tmpl)) > > > > > + <= TMPL_ARGS_DEPTH (args)) > > > > > > > > This condition seems impossible normally; it's only satisfied in this > > > > testcase because friend_tmpl doesn't actually represent the friend > > > > declaration, it's already the named class template. So the substitution in > > > > the next else fails because it was done already. > > > > > > > > If this condition is true, we could set tmpl = friend_tmpl earlier, and skip > > > > doing name lookup at all. > > > > > > > > It's interesting that the previous if does the same depth comparison, and > > > > that dates back to 2002; I wonder why it was needed then? > > I reckon the depth comparison in the previous if is equivalent to: > > if (DECL_UNINSTANTIATED_TEMPLATE_FRIEND_P (friend_tmpl)) > > But unfortunately we can't skip doing name lookup in that case due to > the mentioned example :/ > > > > > > > > > Jason > > > > > > > > > > Ah right, I see. I think the depth comparison in the previous if > > > actually is for exactly the same reason, just for the normal case when > > > the template *is* found by name lookup, e.g. > > > > > > template struct A {}; > > > template struct B { > > > template friend struct ::A; > > > }; > > > > > > This is g++.dg/template/friend5. Here's an updated patch which is so > > > far very lightly tested, OK for trunk if full bootstrap+regtest > > > succeeds? > > > > > > -- >8 -- > > > > > > With modules it may be the case that a template friend class provided > > > with a qualified name is not found by name lookup at instantiation time, > > > due to the class not being exported from its module. This causes issues > > > in tsubst_friend_class which did not handle this case. > > > > > > This is a more general issue, in fact, caused by the named friend class > > > not actually requiring tsubsting. This was already worked around for > > > the "found by name lookup" case (g++.dg/template/friend5.C), but it > > > looks like there's no need to do name lookup at all for this to work. > > > > > > PR c++/115801 > > > > > > gcc/cp/ChangeLog: > > > > > > * pt.cc (tsubst_friend_class): Return the type directly when no > > > tsubsting is required. > > > > > > gcc/testsuite/ChangeLog: > > > > > > * g++.dg/modules/tpl-friend-16_a.C: New test. > > > * g++.dg/modules/tpl-friend-16_b.C: New test. > > > > > > Signed-off-by: Nathaniel Shead > > > --- > > > gcc/cp/pt.cc | 39 ++++++++++-------- > > > .../g++.dg/modules/tpl-friend-16_a.C | 40 +++++++++++++++++++ > > > .../g++.dg/modules/tpl-friend-16_b.C | 17 ++++++++ > > > 3 files changed, 79 insertions(+), 17 deletions(-) > > > create mode 100644 gcc/testsuite/g++.dg/modules/tpl-friend-16_a.C > > > create mode 100644 gcc/testsuite/g++.dg/modules/tpl-friend-16_b.C > > > > > > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc > > > index 2db59213c54..ea00577fd37 100644 > > > --- a/gcc/cp/pt.cc > > > +++ b/gcc/cp/pt.cc > > > @@ -11732,6 +11732,15 @@ tsubst_friend_class (tree friend_tmpl, tree args) > > > return TREE_TYPE (tmpl); > > > } > > > > > > + if (TMPL_PARMS_DEPTH (DECL_TEMPLATE_PARMS (friend_tmpl)) > > > + <= TMPL_ARGS_DEPTH (args)) > > > + /* The template has already been subsituted, e.g. for > > > + > > > + template friend class ::C; > > > + > > > + so we just need to return it directly. */ > > > + return TREE_TYPE (friend_tmpl); > > > > IIUC I don't think this would do the right thing for a template friend > > declaration that directly names a template from an outer current > > instantiation: > > > > template > > struct A { > > template struct B; > > > > template > > struct C { > > template friend class A::B; > > }; > > }; > > > > template struct A::C; > > > > Here TMPL_PARMS_DEPTH (DECL_TEMPLATE_PARMS (friend_tmpl)) is 2, and > > so is TMPL_ARGS_DEPTH (args), so we'd exit early and return a fully > > dependent TEMPLATE_DECL A::B, but what we want to return is > > A::B. > > > > It should always be safe to exit early when > > TMPL_PARMS_DEPTH (DECL_TEMPLATE_PARMS (friend_tmpl)) == 1 > > though, and that should cover the most common case. > > Ah right, I see; a full version of the testcase I guess might be template struct A { template struct B; template struct C { template friend struct A::B; private: int x; }; }; template template struct A::B { int foo(A::C c) { return c.x; } }; template struct A::C; template struct A::B; template struct A::B; // !!! Which is currently correctly rejected by GCC but with my proposed change is no longer rejected. How does this patch look instead? This passes dg.exp and modules.exp still, still waiting for full regtest to complete. -- >8 -- With modules it may be the case that a template friend class provided with a qualified name is not found by name lookup at instantiation time, due to the class not being exported from its module. This causes issues in tsubst_friend_class which did not handle this case. This is caused by the named friend class not actually requiring tsubsting. This was already worked around for the "found by name lookup" case (g++.dg/template/friend5.C), but it looks like there's no need to do name lookup at all for this particular case to work. We do need to be careful to continue to do name lookup to handle templates from an outer current instantiation though; this patch adds a new testcase for this as well. This should not impact modules (because exportingness will only affect namespace lookup). PR c++/115801 gcc/cp/ChangeLog: * pt.cc (tsubst_friend_class): Return the type immediately when no tsubsting or name lookup is required. gcc/testsuite/ChangeLog: * g++.dg/modules/tpl-friend-16_a.C: New test. * g++.dg/modules/tpl-friend-16_b.C: New test. * g++.dg/template/friend82.C: New test. Signed-off-by: Nathaniel Shead Reviewed-by: Patrick Palka Reviewed-by: Jason Merrill --- gcc/cp/pt.cc | 8 ++++ .../g++.dg/modules/tpl-friend-16_a.C | 40 +++++++++++++++++++ .../g++.dg/modules/tpl-friend-16_b.C | 17 ++++++++ gcc/testsuite/g++.dg/template/friend82.C | 23 +++++++++++ 4 files changed, 88 insertions(+) create mode 100644 gcc/testsuite/g++.dg/modules/tpl-friend-16_a.C create mode 100644 gcc/testsuite/g++.dg/modules/tpl-friend-16_b.C create mode 100644 gcc/testsuite/g++.dg/template/friend82.C diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc index 2db59213c54..8abd092116b 100644 --- a/gcc/cp/pt.cc +++ b/gcc/cp/pt.cc @@ -11732,6 +11732,14 @@ tsubst_friend_class (tree friend_tmpl, tree args) return TREE_TYPE (tmpl); } + if (TMPL_PARMS_DEPTH (DECL_TEMPLATE_PARMS (friend_tmpl)) == 1) + /* The template has already been fully substituted, e.g. for + + template friend class ::C; + + so we can just return it directly. */ + return TREE_TYPE (friend_tmpl); + tree context = CP_DECL_CONTEXT (friend_tmpl); if (TREE_CODE (context) == NAMESPACE_DECL) push_nested_namespace (context); diff --git a/gcc/testsuite/g++.dg/modules/tpl-friend-16_a.C b/gcc/testsuite/g++.dg/modules/tpl-friend-16_a.C new file mode 100644 index 00000000000..e1cdcd98e1e --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/tpl-friend-16_a.C @@ -0,0 +1,40 @@ +// PR c++/115801 +// { dg-additional-options "-fmodules-ts -Wno-global-module" } +// { dg-module-cmi test } + +module; + +template struct GMF; +template struct GMF_Hidden { + int go() { GMF gmf; return gmf.x; } +}; + +template struct GMF { +private: + template friend struct ::GMF_Hidden; + int x = 1; +}; + +template int test_gmf() { + GMF_Hidden h; return h.go(); +} + +export module test; + +export using ::GMF; +export using ::test_gmf; + +export template struct Attached; +template struct Attached_Hidden { + int go() { Attached attached; return attached.x; } +}; + +template struct Attached { +private: + template friend struct ::Attached_Hidden; + int x = 2; +}; + +export template int test_attached() { + Attached_Hidden h; return h.go(); +} diff --git a/gcc/testsuite/g++.dg/modules/tpl-friend-16_b.C b/gcc/testsuite/g++.dg/modules/tpl-friend-16_b.C new file mode 100644 index 00000000000..d3484ab19b1 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/tpl-friend-16_b.C @@ -0,0 +1,17 @@ +// PR c++/115801 +// { dg-additional-options "-fmodules-ts" } + +import test; + +int main() { + GMF gmf; + Attached attached; + + int a = test_gmf(); + int b = test_attached(); + + GMF_Hidden gmf_hidden; // { dg-error "not declared" } + Attached_Hidden attached_hidden; // { dg-error "not declared" } +} + +// { dg-prune-output "expected primary-expression" } diff --git a/gcc/testsuite/g++.dg/template/friend82.C b/gcc/testsuite/g++.dg/template/friend82.C new file mode 100644 index 00000000000..28a057dd23e --- /dev/null +++ b/gcc/testsuite/g++.dg/template/friend82.C @@ -0,0 +1,23 @@ +// { dg-do compile } + +template +struct A { + template struct B; + + template + struct C { + template friend struct A::B; + private: + int x; + }; +}; + +template +template +struct A::B { + int foo(A::C c) { return c.x; } // { dg-error "private" } +}; + +template struct A::C; +template struct A::B; // { dg-bogus "" } +template struct A::B; // { dg-message "required from here" }