From patchwork Fri Mar 8 02:55:22 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Nathaniel Shead X-Patchwork-Id: 1909518 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=SQ0swtQi; 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 4TrW5D57Ykz1yX8 for ; Fri, 8 Mar 2024 13:55:58 +1100 (AEDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 88EB9385AC11 for ; Fri, 8 Mar 2024 02:55:54 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mail-pf1-x42d.google.com (mail-pf1-x42d.google.com [IPv6:2607:f8b0:4864:20::42d]) by sourceware.org (Postfix) with ESMTPS id CDB6D3858D35 for ; Fri, 8 Mar 2024 02:55:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org CDB6D3858D35 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 CDB6D3858D35 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::42d ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1709866532; cv=none; b=J3ezECcHoZp0/ff+7zP8vAj0E64WsCUBlOGwOBHBuel53Yv6EPt/CxRaoNSfzH1bPyCOiE2cT8BknAzPgWHCtqtTpJhD+yKCdW1dl0nJtG//eZFwbKPT3jC2YgqPnM9xc2Dd6CBrEnBiD8jC3tUNzD0VF8HCTDRQGIPD+ur5q58= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1709866532; c=relaxed/simple; bh=yMFZiqCcXQ6FNMJpnDh+bItChGQVEFRR+EkdGisfksI=; h=DKIM-Signature:Message-ID:Date:From:To:Subject:MIME-Version; b=Ub5JCPY+IpFneOHXvbXQCHU8sN6pBy8bqjRfm0S8b56FOLdOlzZBgzg5miHfzH8cUp/vgIdTFK4sJCpkTPDG2ze7X4mcB7bHbBs2LzX/NtLeorxPXfCcmZzO5d39lII1OXD4ZS6Jq9QkZwe1Q3Zy1y/9omUBDyRqdAhLC1/Nlvg= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-pf1-x42d.google.com with SMTP id d2e1a72fcca58-6e55b33ad14so991743b3a.1 for ; Thu, 07 Mar 2024 18:55:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1709866528; x=1710471328; 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=k05U4aXdDU6PhLWpzHmONie0MlihdPfhP08fwBWMj58=; b=SQ0swtQiDXbBZeGNpmCEPALM9Uo43r4Feq3q0332rC27uKI8EhpqQesbxmEygXuZC2 iaLOGJOJ3fFbrGFxLCN6aUe+31R/jXfmECfVXioSgDVTYReMN7OZKaTPr7PGmXjju2p7 rzD4eIYTTBM9fBg0CnnObAGn7WFdh1CjqPVRp4/JKoJK4IBm7NovpzPKjRmjj+mzzBT5 urQJtu6oMjjNNhSIrKgVHnA5nyTTnNdz30vlr225kqljnT57wR6AyFqsHv8NivVtPwq4 zodYER7zu6V3vs04yHjez/8h4xbFG+Yii1wpAxlHKekXeXjiowjbzIexaOlRCbe9gJzl aGFQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1709866528; x=1710471328; 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=k05U4aXdDU6PhLWpzHmONie0MlihdPfhP08fwBWMj58=; b=pztn2z9HiairLcZc6GNZHlFFGOYXxU0TY/6h+1EMMX3gjRfaQE61xE1ZahOC5+QBgv /Li67hcOmv5hAFsG7POPbAR9P1zF4wzkr74/9WoUZ10J6YyaX9wIP7t65l4zfeH4Vmdu bzRZEdNOsEJ41ZAJIDz8N+zjucTFp2KgHrc8JY/oF1PY6erz9ubyuiSJyfuvfZN9uaD7 Ku8gfUCoHmaxazcXUiCiaVwS2sEAS+KooKITy2zzCnEWw/x2idMVs+mB5tmk6HCmD2hL aRLikScT4yNUOIN/5YHDzT0U901DNOg03ibdz/wHjCaP7auUWArv8SLa28tRe0HAKMRl zx7Q== X-Gm-Message-State: AOJu0YwlDIAiTYB3Bl0YvtddhIWy/7Gg3/mTCkr980Xf4v9m9OjuY+Mp 88D2C+8zcrTVjEQLJNdo+hgC2jO7n+JbNxahfwxKZBNuzzfUe/mblH4kPZ1a X-Google-Smtp-Source: AGHT+IFz5ZK28M27teB0hF/+8qoBrjPVVq6BtX+JWqGlCLkLa3NNUFgT7Qlh8zb1hMqMrVBwi8Z1IA== X-Received: by 2002:a05:6a00:a8e:b0:6e6:211e:185a with SMTP id b14-20020a056a000a8e00b006e6211e185amr14678017pfl.20.1709866527517; Thu, 07 Mar 2024 18:55:27 -0800 (PST) Received: from Thaum. (202-161-100-107.tpgi.com.au. [202.161.100.107]) by smtp.gmail.com with ESMTPSA id 11-20020a63104b000000b005d8e30897e4sm13175278pgq.69.2024.03.07.18.55.25 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 07 Mar 2024 18:55:26 -0800 (PST) Message-ID: <65ea7e1e.630a0220.d1c27.476d@mx.google.com> X-Google-Original-Message-ID: Date: Fri, 8 Mar 2024 13:55:22 +1100 From: Nathaniel Shead To: gcc-patches@gcc.gnu.org Cc: Nathan Sidwell , Jason Merrill Subject: [PATCH v2] c++: Check module attachment instead of just purview when necessary [PR112631] References: <655b2b3f.a70a0220.ca3c4.d564@mx.google.com> <65642242.170a0220.8bb8e.28e1@mx.google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <65642242.170a0220.8bb8e.28e1@mx.google.com> 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, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE, WEIRD_PORT 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 Mon, Nov 27, 2023 at 03:59:39PM +1100, Nathaniel Shead wrote: > On Thu, Nov 23, 2023 at 03:03:37PM -0500, Nathan Sidwell wrote: > > On 11/20/23 04:47, Nathaniel Shead wrote: > > > Bootstrapped and regtested on x86_64-pc-linux-gnu. I don't have write > > > access. > > > > > > -- >8 -- > > > > > > Block-scope declarations of functions or extern values are not allowed > > > when attached to a named module. Similarly, class member functions are > > > not inline if attached to a named module. However, in both these cases > > > we currently only check if the declaration is within the module purview; > > > it is possible for such a declaration to occur within the module purview > > > but not be attached to a named module (e.g. in an 'extern "C++"' block). > > > This patch makes the required adjustments. > > > > > > Ah I'd been puzzling over the default inlinedness of member-fns of > > block-scope structs. Could you augment the testcase to make sure that's > > right too? > > > > Something like: > > > > // dg-module-do link > > export module Mod; > > > > export auto Get () { > > struct X { void Fn () {} }; > > return X(); > > } > > > > > > /// > > import Mod > > void Frob () { Get().Fn(); } > > > > I gave this a try and it indeed doesn't work correctly; 'Fn' needs to be > marked 'inline' for this to link (whether or not 'Get' itself is > inline). I've tried tracing the code to work out what's going on but > I've been struggling to work out how all the different flags (e.g. > TREE_PUBLIC, TREE_EXTERNAL, TREE_COMDAT, DECL_NOT_REALLY_EXTERN) > interact, which flags we want to be set where, and where the decision of > what function definitions to emit is actually made. > > I did find that doing 'mark_used(decl)' on all member functions in > block-scope structs seems to work however, but I wonder if that's maybe > too aggressive or if there's something else we should be doing? I got around to looking at this again, here's an updated version of this patch. Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? (I'm not sure if 'start_preparsed_function' is the right place to be putting this kind of logic or if it should instead be going in 'grokfndecl', e.g. decl.cc:10761 where the rules for making local functions have no linkage are initially determined, but I found this easier to implement: happy to move around though if preferred.) -- >8 -- Block-scope declarations of functions or extern values are not allowed when attached to a named module. Similarly, class member functions are not inline if attached to a named module. However, in both these cases we currently only check if the declaration is within the module purview; it is possible for such a declaration to occur within the module purview but not be attached to a named module (e.g. in an 'extern "C++"' block). This patch makes the required adjustments. While implementing this we discovered that block-scope local functions are not correctly emitted, causing link failures; this patch also corrects some assumptions here and ensures that they are emitted when needed. PR c++/112631 gcc/cp/ChangeLog: * cp-tree.h (named_module_attach_p): New function. * decl.cc (start_decl): Check for attachment not purview. (grokmethod): Likewise. (start_preparsed_function): Ensure block-scope functions are emitted in module interfaces. * decl2.cc (determine_visibility): Likewise. gcc/testsuite/ChangeLog: * g++.dg/modules/block-decl-1_a.C: New test. * g++.dg/modules/block-decl-1_b.C: New test. * g++.dg/modules/block-decl-2_a.C: New test. * g++.dg/modules/block-decl-2_b.C: New test. * g++.dg/modules/block-decl-2_c.C: New test. * g++.dg/modules/block-decl-3.C: New test. Signed-off-by: Nathaniel Shead --- gcc/cp/cp-tree.h | 2 + gcc/cp/decl.cc | 22 ++- gcc/cp/decl2.cc | 23 +-- gcc/testsuite/g++.dg/modules/block-decl-1_a.C | 9 ++ gcc/testsuite/g++.dg/modules/block-decl-1_b.C | 10 ++ gcc/testsuite/g++.dg/modules/block-decl-2_a.C | 143 ++++++++++++++++++ gcc/testsuite/g++.dg/modules/block-decl-2_b.C | 8 + gcc/testsuite/g++.dg/modules/block-decl-2_c.C | 25 +++ gcc/testsuite/g++.dg/modules/block-decl-3.C | 21 +++ 9 files changed, 249 insertions(+), 14 deletions(-) create mode 100644 gcc/testsuite/g++.dg/modules/block-decl-1_a.C create mode 100644 gcc/testsuite/g++.dg/modules/block-decl-1_b.C create mode 100644 gcc/testsuite/g++.dg/modules/block-decl-2_a.C create mode 100644 gcc/testsuite/g++.dg/modules/block-decl-2_b.C create mode 100644 gcc/testsuite/g++.dg/modules/block-decl-2_c.C create mode 100644 gcc/testsuite/g++.dg/modules/block-decl-3.C diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 14895bc6585..05913861e06 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -7381,6 +7381,8 @@ inline bool module_attach_p () inline bool named_module_purview_p () { return named_module_p () && module_purview_p (); } +inline bool named_module_attach_p () +{ return named_module_p () && module_attach_p (); } /* We're currently exporting declarations. */ inline bool module_exporting_p () diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc index dbc3df24e77..92475ecc28f 100644 --- a/gcc/cp/decl.cc +++ b/gcc/cp/decl.cc @@ -6092,10 +6092,10 @@ start_decl (const cp_declarator *declarator, { /* A function-scope decl of some namespace-scope decl. */ DECL_LOCAL_DECL_P (decl) = true; - if (named_module_purview_p ()) + if (named_module_attach_p ()) error_at (declarator->id_loc, - "block-scope extern declaration %q#D not permitted" - " in module purview", decl); + "block-scope extern declaration %q#D must not be" + " attached to a named module", decl); } /* Enter this declaration into the symbol table. Don't push the plain @@ -18054,6 +18054,18 @@ start_preparsed_function (tree decl1, tree attrs, int flags) /* This is a function in a local class in an extern inline or template function. */ comdat_linkage (decl1); + + if (ctx != NULL_TREE && TREE_PUBLIC (ctx) && module_has_cmi_p ()) + { + /* Ensure that functions in local classes within named modules + have their definitions exported, in case they are (directly + or indirectly) used by an importer. */ + TREE_PUBLIC (decl1) = true; + if (DECL_DECLARED_INLINE_P (decl1)) + comdat_linkage (decl1); + else + mark_needed (decl1); + } } /* If this function belongs to an interface, it is public. If it belongs to someone else's interface, it is also external. @@ -18907,10 +18919,10 @@ grokmethod (cp_decl_specifier_seq *declspecs, check_template_shadow (fndecl); /* p1779 ABI-Isolation makes inline not a default for in-class - definitions in named module purview. If the user explicitly + definitions attached to a named module. If the user explicitly made it inline, grokdeclarator will already have done the right things. */ - if ((!named_module_purview_p () + if ((!named_module_attach_p () || flag_module_implicit_inline /* Lambda's operator function remains inline. */ || LAMBDA_TYPE_P (DECL_CONTEXT (fndecl))) diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc index 6c9fd415d40..94eaf98c609 100644 --- a/gcc/cp/decl2.cc +++ b/gcc/cp/decl2.cc @@ -3050,15 +3050,20 @@ determine_visibility (tree decl) constrain_visibility (decl, tvis, false); } else if (no_linkage_check (TREE_TYPE (decl), /*relaxed_p=*/true)) - /* DR 757: A type without linkage shall not be used as the type of a - variable or function with linkage, unless - o the variable or function has extern "C" linkage (7.5 [dcl.link]), or - o the variable or function is not used (3.2 [basic.def.odr]) or is - defined in the same translation unit. - - Since non-extern "C" decls need to be defined in the same - translation unit, we can make the type internal. */ - constrain_visibility (decl, VISIBILITY_ANON, false); + { + /* DR 757: A type without linkage shall not be used as the type of a + variable or function with linkage, unless + o the variable or function has extern "C" linkage (7.5 [dcl.link]), or + o the variable or function is not used (3.2 [basic.def.odr]) or is + defined in the same translation unit. + + Since non-extern "C" decls need to be defined in the same + translation unit, we can make the type internal, unless this + type is part of a module CMI, in which case importers may need + to access it. */ + if (!module_has_cmi_p ()) + constrain_visibility (decl, VISIBILITY_ANON, false); + } /* If visibility changed and DECL already has DECL_RTL, ensure symbol flags are updated. */ diff --git a/gcc/testsuite/g++.dg/modules/block-decl-1_a.C b/gcc/testsuite/g++.dg/modules/block-decl-1_a.C new file mode 100644 index 00000000000..e7ffc629192 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/block-decl-1_a.C @@ -0,0 +1,9 @@ +// { dg-additional-options "-fmodules-ts" } +// { dg-module-cmi bla } + +export module bla; + +export extern "C++" inline void fun() { + void oops(); // { dg-bogus "block-scope extern declaration" } + oops(); +} diff --git a/gcc/testsuite/g++.dg/modules/block-decl-1_b.C b/gcc/testsuite/g++.dg/modules/block-decl-1_b.C new file mode 100644 index 00000000000..c0d724f25ac --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/block-decl-1_b.C @@ -0,0 +1,10 @@ +// { dg-module-do link } +// { dg-additional-options "-fmodules-ts" } + +import bla; + +void oops() {} + +int main() { + fun(); +} diff --git a/gcc/testsuite/g++.dg/modules/block-decl-2_a.C b/gcc/testsuite/g++.dg/modules/block-decl-2_a.C new file mode 100644 index 00000000000..00a4f229ae8 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/block-decl-2_a.C @@ -0,0 +1,143 @@ +// { dg-additional-options "-fmodules-ts" } +// { dg-module-cmi mod } + +export module mod; + +namespace { + void internal() {} +} + +// singly-nested (i=inline, n=non-inline) + +export auto n_n() { + internal(); + struct X { void f() { internal(); } }; + return X{}; +} + +export auto n_i() { + internal(); + struct X { inline void f() {} }; + return X{}; +} + +export inline auto i_n() { + // `f` is not inline here, so this is OK + struct X { void f() { internal(); } }; + return X{}; +} + +export inline auto i_i() { + struct X { inline void f() {} }; + return X{}; +} + + +// doubly nested + +export auto n_n_n() { + struct X { + auto f() { + struct Y { + void g() { internal(); } + }; + return Y{}; + } + }; + return X{}; +} + +export auto n_i_n() { + struct X { + inline auto f() { + struct Y { + void g() { internal(); } + }; + return Y{}; + } + }; + return X{}; +} + +export inline auto i_n_i() { + struct X { + auto f() { + struct Y { + inline void g() {} + }; + return Y {}; + } + }; + return X{}; +} + +export inline auto i_i_i() { + struct X { + inline auto f() { + struct Y { + inline void g() {} + }; + return Y{}; + } + }; + return X{}; +} + + +// multiple types + +export auto multi_n_n() { + struct X { + void f() { internal(); } + }; + struct Y { + X x; + }; + return Y {}; +} + +export auto multi_n_i() { + struct X { + inline void f() {} + }; + struct Y { + X x; + }; + return Y {}; +} + +export inline auto multi_i_i() { + struct X { + inline void f() {} + }; + struct Y { + X x; + }; + return Y {}; +}; + + +// extern "C++" + +export extern "C++" auto extern_n_i() { + struct X { + void f() {} // implicitly inline + }; + return X{}; +}; + +export extern "C++" inline auto extern_i_i() { + struct X { + void f() {} + }; + return X{}; +}; + + +// can access from implementation unit + +auto only_used_in_impl() { + struct X { void f() {} }; + return X{}; +} +export void test_from_impl_unit(); diff --git a/gcc/testsuite/g++.dg/modules/block-decl-2_b.C b/gcc/testsuite/g++.dg/modules/block-decl-2_b.C new file mode 100644 index 00000000000..bc9b2a213f0 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/block-decl-2_b.C @@ -0,0 +1,8 @@ +// { dg-additional-options "-fmodules-ts" } + +module mod; + +// Test that we can access (and link) to declarations from the interface +void test_from_impl_unit() { + only_used_in_impl().f(); +} diff --git a/gcc/testsuite/g++.dg/modules/block-decl-2_c.C b/gcc/testsuite/g++.dg/modules/block-decl-2_c.C new file mode 100644 index 00000000000..ef275d10f0e --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/block-decl-2_c.C @@ -0,0 +1,25 @@ +// { dg-module-do link } +// { dg-additional-options "-fmodules-ts" } + +import mod; + +int main() { + n_n().f(); + n_i().f(); + i_n().f(); + i_i().f(); + + n_n_n().f().g(); + n_i_n().f().g(); + i_n_i().f().g(); + i_i_i().f().g(); + + multi_n_n().x.f(); + multi_n_i().x.f(); + multi_i_i().x.f(); + + extern_n_i().f(); + extern_i_i().f(); + + test_from_impl_unit(); +} diff --git a/gcc/testsuite/g++.dg/modules/block-decl-3.C b/gcc/testsuite/g++.dg/modules/block-decl-3.C new file mode 100644 index 00000000000..8bbebd06bab --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/block-decl-3.C @@ -0,0 +1,21 @@ +// { dg-additional-options "-fmodules-ts" } +// { dg-module-cmi !mod } + +export module mod; + +namespace { + void internal(); +} + +export extern "C++" auto foo() { + struct X { + // `foo` is not attached to a named module, and as such + // `X::f` should be implicitly `inline` here + void f() { // { dg-error "references internal linkage entity" } + internal(); + } + }; + return X{}; +} + +// { dg-prune-output "failed to write compiled module" }