From patchwork Sat Jan 20 10:45:47 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Nathaniel Shead X-Patchwork-Id: 84482 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 4DD563858414 for ; Sat, 20 Jan 2024 10:46:31 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mail-ot1-x334.google.com (mail-ot1-x334.google.com [IPv6:2607:f8b0:4864:20::334]) by sourceware.org (Postfix) with ESMTPS id 329D23858D39 for ; Sat, 20 Jan 2024 10:45:56 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 329D23858D39 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 329D23858D39 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::334 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1705747558; cv=none; b=mpURNOcwpM3LjEC02GtYgvTjmyZ/tYuC1qXrx7hVtpsHjrIw17WFLCMvxgN/4+BtrvicVRBXU6DYCcO/XuNtr8xKsHhYJX/sDFuX4TawYQyz4L6PA8X4MyuCeDkPJ0wmXYPAtq4BJPf7lpdATNhJJ4Nvq9J7F6q6eg2gk4AwRLU= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1705747558; c=relaxed/simple; bh=ReDH36z0NP9YQiFaQL4FH5Ai1zmGTXetvwHQKUNGQEA=; h=DKIM-Signature:Message-ID:Date:From:To:Subject:MIME-Version; b=MdD0H5PjGVTLiGY2mdTpsStNGzmiRoxeS8+tyj5z1lBmBm+VUBjHYiuUH2FVvNRWlJS3JGtZEB2ZIfmTKrMMUB9vbrNkAoqvnGQtvx2sEW5AbuGWEVtAB38aQnehBNbsjcKdHn+UJkqy+0vUAe6eTYjpG080g0kmgGVJxOOqsAc= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-ot1-x334.google.com with SMTP id 46e09a7af769-6ddf1e88e51so1135507a34.0 for ; Sat, 20 Jan 2024 02:45:56 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1705747554; x=1706352354; 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=Ezt4wQ8qLHixUDtEuE85Sl4Ijj2Wqazxe25Q1JpTh+Y=; b=b3NvEBn3pDR6VPSSzuk9qdjA5NnUIKAfBgud6fKTOC8Coc8aHXVN8GqCiHrE8pG8tg PYcJQOzLmTIDqvbJZ7xyiQVcCAEyxYbOmKmWN3q/7lvfCdQ0lXzxInjYalHDIY6x1ddP QxDa8iN1Tnom0cLjbhgvfzGUWBhjfDOttGx3XuZGvllhwdYS8/PkRwefyY8w9cKF+yox 9ouf9EITIdxA05HqEIlDqRmoAjWpFL9JWdkEauH/VgLbBFwe9xBM6cpm1/6pPZ0v8vqR 7hm5PlSJ+liW+2B+bs1JsTvpzhO/CDM1Xa3ko+QbGXEKVSYKu7eod1ig2dfK3dGPqDAv JcxA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1705747554; x=1706352354; 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=Ezt4wQ8qLHixUDtEuE85Sl4Ijj2Wqazxe25Q1JpTh+Y=; b=QPt1ghuys9z19kvF3OSiWdqD7DDlmbCRd4tpYFBSiIbwe5WeG9CZ9ALMDXZONi3Fth ucryjCpLj3UBHvAl1YCp1nDcOp48PR3ZXXll6Ij05u/NrvXrB29jekJl6pw8yZBajnuQ UH9LFu054xsKY+xf9e0hxDt/fznDxtK0q1KMdD26PYOon2dMkNiUbeahx+FJ2xSlO/gr bRVJOh9xuBZp+odOuE/vWDUO26RZsVueiQywR6RrGRW5i6+kgbMsqwJ2IrGNvDy8A/i1 SN/jTkB775fZfBtpF8Qh4fiygdKY5iBFiG5IpnvDXrMr7yoNe3yNiQdhCPyZz5UoeWR1 LcLQ== X-Gm-Message-State: AOJu0YwphcdUjTdKTkZKlItfT2b8fTB1Wb7JgAt4R1x/OzsFGdhf9sYy d73LIqCHPwXbwlAAr9usMW7a/+FYzfBDxfd2/ljJuVYfhkZJWpmO X-Google-Smtp-Source: AGHT+IF7zV9/Vb4d9kDqW3QBrrt1H9QKfSj0GJ5u4ep2wOHvrRCT19+9dRY1YyFqDTS2aP5Xu2rOEA== X-Received: by 2002:a05:6830:1484:b0:6dd:ee5f:501e with SMTP id s4-20020a056830148400b006ddee5f501emr1301174otq.51.1705747554459; Sat, 20 Jan 2024 02:45:54 -0800 (PST) Received: from Thaum. (123-243-206-49.tpgi.com.au. [123.243.206.49]) by smtp.gmail.com with ESMTPSA id y18-20020aa79e12000000b006d9ac45206bsm6523619pfq.206.2024.01.20.02.45.51 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 20 Jan 2024 02:45:53 -0800 (PST) Message-ID: <65aba461.a70a0220.7fd5c.ad24@mx.google.com> X-Google-Original-Message-ID: Date: Sat, 20 Jan 2024 21:45:47 +1100 From: Nathaniel Shead To: Patrick Palka Cc: gcc-patches@gcc.gnu.org, Jason Merrill , Nathan Sidwell Subject: [PATCH v3] c++/modules: Emit definitions of ODR-used static members imported from modules [PR112899] References: <659490fd.170a0220.1ce2e.503a@mx.google.com> <36bed0fe-5564-60ed-df91-240d78819add@idea> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <36bed0fe-5564-60ed-df91-240d78819add@idea> X-Spam-Status: No, score=-10.9 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, URIBL_BLACK 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+patchwork=sourceware.org@gcc.gnu.org On Fri, Jan 19, 2024 at 01:57:18PM -0500, Patrick Palka wrote: > On Wed, 3 Jan 2024, Nathaniel Shead wrote: > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? > > > > -- >8 -- > > > > Static data members marked 'inline' should be emitted in TUs where they > > are ODR-used. We need to make sure that statics imported from modules > > are correctly added to the 'pending_statics' map so that they get > > emitted if needed, otherwise the attached testcase fails to link. > > > > PR c++/112899 > > > > gcc/cp/ChangeLog: > > > > * cp-tree.h (note_variable_template_instantiation): Rename to... > > (note_static_storage_variable): ...this. > > * decl2.cc (note_variable_template_instantiation): Rename to... > > (note_static_storage_variable): ...this. > > * pt.cc (instantiate_decl): Rename usage of above function. > > * module.cc (trees_in::read_var_def): Remember pending statics > > that we stream in. > > > > gcc/testsuite/ChangeLog: > > > > * g++.dg/modules/init-4_a.C: New test. > > * g++.dg/modules/init-4_b.C: New test. > > > > Signed-off-by: Nathaniel Shead > > --- > > gcc/cp/cp-tree.h | 2 +- > > gcc/cp/decl2.cc | 4 ++-- > > gcc/cp/module.cc | 4 ++++ > > gcc/cp/pt.cc | 2 +- > > gcc/testsuite/g++.dg/modules/init-4_a.C | 9 +++++++++ > > gcc/testsuite/g++.dg/modules/init-4_b.C | 11 +++++++++++ > > 6 files changed, 28 insertions(+), 4 deletions(-) > > create mode 100644 gcc/testsuite/g++.dg/modules/init-4_a.C > > create mode 100644 gcc/testsuite/g++.dg/modules/init-4_b.C > > > > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > > index 1979572c365..ebd2850599a 100644 > > --- a/gcc/cp/cp-tree.h > > +++ b/gcc/cp/cp-tree.h > > @@ -7113,7 +7113,7 @@ extern tree maybe_get_tls_wrapper_call (tree); > > extern void mark_needed (tree); > > extern bool decl_needed_p (tree); > > extern void note_vague_linkage_fn (tree); > > -extern void note_variable_template_instantiation (tree); > > +extern void note_static_storage_variable (tree); > > extern tree build_artificial_parm (tree, tree, tree); > > extern bool possibly_inlined_p (tree); > > extern int parm_index (tree); > > diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc > > index 0850d3f5bce..241216b0dfe 100644 > > --- a/gcc/cp/decl2.cc > > +++ b/gcc/cp/decl2.cc > > @@ -910,10 +910,10 @@ note_vague_linkage_fn (tree decl) > > vec_safe_push (deferred_fns, decl); > > } > > > > -/* As above, but for variable template instantiations. */ > > +/* As above, but for variables with static storage duration. */ > > > > void > > -note_variable_template_instantiation (tree decl) > > +note_static_storage_variable (tree decl) > > { > > vec_safe_push (pending_statics, decl); > > } > > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc > > index 0bd46414da9..14818131a70 100644 > > --- a/gcc/cp/module.cc > > +++ b/gcc/cp/module.cc > > @@ -11752,6 +11752,10 @@ trees_in::read_var_def (tree decl, tree maybe_template) > > DECL_INITIALIZED_P (decl) = true; > > if (maybe_dup && DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P (maybe_dup)) > > DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P (decl) = true; > > + if (DECL_CONTEXT (decl) > > + && RECORD_OR_UNION_TYPE_P (DECL_CONTEXT (decl)) > > + && !DECL_TEMPLATE_INFO (decl)) > > + note_static_storage_variable (decl); > > It seems this should also handle templated inlines via > > && (!DECL_TEMPLATE_INFO (decl) > || DECL_IMPLICIT_INSTANTIATION (decl)) > > otherwise the following fails to link: > > $ cat init-5_a.H > template > struct __from_chars_alnum_to_val_table { > static inline int value = 42; > }; > > inline unsigned char > __from_chars_alnum_to_val() { > return __from_chars_alnum_to_val_table::value; > } > > $ cat init-6_b.C > import "init-5_a.H"; > > int main() { > __from_chars_alnum_to_val(); > } > > $ g++ -fmodules-ts -std=c++20 init-5_a.H init-5_b.C > /usr/bin/ld: /tmp/ccNRaads.o: in function `__from_chars_alnum_to_val()': > init-6_b.C:(.text._Z25__from_chars_alnum_to_valv[_Z25__from_chars_alnum_to_valv]+0x6): undefined reference to `__from_chars_alnum_to_val_table::value' Ah yes, of course, since that's the other context where declarations are added to 'pending_statics'. > > By the way I ran into this when testing out std::print with modules: > > $ cat std.C > export module std; > export import ; > > $ cat hello.C > import std; > > int main() { > std::print("Hello {}!", "World"); > } > > $ g++ -fmodules-ts -std=c++26 -x c++-system-header bits/stdc++.h > $ g++ -fmodules-ts -std=c++26 std.C hello.C && ./a.out # before > /usr/bin/ld: /tmp/ccqNgOM1.o: in function `unsigned char std::__detail::__from_chars_alnum_to_val(unsigned char)': > hello.C:(.text._ZNSt8__detail25__from_chars_alnum_to_valILb0EEEhh[_ZNSt8__detail25__from_chars_alnum_to_valILb0EEEhh]+0x12): undefined reference to `std::__detail::__from_chars_alnum_to_val_table::value' > $ g++ -fmodules-ts -std=c++26 std.C hello.C && ./a.out # after > Hello World! > > It's great that this is so close to working! That's great! Here's a new version of the patch with the above fixed. I also included your change to only add class variable templates to 'pending_statics' (and the normal 'static_decl's for non-class otherwise) as otherwise I could imagine that they would cause issues with this later too. I know that there's been discussion about the correct ABI for inline declarations, but personally I'd like to have this fixed for normal uses in GCC14 at least, and we can revisit the specific cases where various kinds of declarations are emitted in stage 1. Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? P.S. As I go to send this, I wonder if maybe something like 'note_static_member_variable' would be a clearer choice of name than 'note_static_storage_variable'? -- >8 -- Static data members marked 'inline' should be emitted in TUs where they are ODR-used. We need to make sure that statics imported from modules are correctly added to the 'pending_statics' map so that they get emitted if needed, otherwise the attached testcase fails to link. PR c++/112899 gcc/cp/ChangeLog: * cp-tree.h (note_variable_template_instantiation): Rename to... (note_static_storage_variable): ...this. * decl2.cc (note_variable_template_instantiation): Rename to... (note_static_storage_variable): ...this. * pt.cc (instantiate_decl): Rename usage of above function and only use for class-scope variables. * module.cc (trees_in::read_var_def): Remember pending statics that we stream in. gcc/testsuite/ChangeLog: * g++.dg/modules/init-4_a.C: New test. * g++.dg/modules/init-4_b.C: New test. * g++.dg/modules/init-6_a.H: New test. * g++.dg/modules/init-6_b.C: New test. Signed-off-by: Nathaniel Shead Reviewed-by: Patrick Palka --- gcc/cp/cp-tree.h | 2 +- gcc/cp/decl2.cc | 4 ++-- gcc/cp/module.cc | 5 +++++ gcc/cp/pt.cc | 7 ++++++- gcc/testsuite/g++.dg/modules/init-4_a.C | 9 +++++++++ gcc/testsuite/g++.dg/modules/init-4_b.C | 11 +++++++++++ gcc/testsuite/g++.dg/modules/init-6_a.H | 12 ++++++++++++ gcc/testsuite/g++.dg/modules/init-6_b.C | 8 ++++++++ 8 files changed, 54 insertions(+), 4 deletions(-) create mode 100644 gcc/testsuite/g++.dg/modules/init-4_a.C create mode 100644 gcc/testsuite/g++.dg/modules/init-4_b.C create mode 100644 gcc/testsuite/g++.dg/modules/init-6_a.H create mode 100644 gcc/testsuite/g++.dg/modules/init-6_b.C diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index d9b14d7c4f5..ac7531c5a73 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -7146,7 +7146,7 @@ extern tree maybe_get_tls_wrapper_call (tree); extern void mark_needed (tree); extern bool decl_needed_p (tree); extern void note_vague_linkage_fn (tree); -extern void note_variable_template_instantiation (tree); +extern void note_static_storage_variable (tree); extern tree build_artificial_parm (tree, tree, tree); extern bool possibly_inlined_p (tree); extern int parm_index (tree); diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc index 91d4e2e5ea4..5d270bf2cdc 100644 --- a/gcc/cp/decl2.cc +++ b/gcc/cp/decl2.cc @@ -947,10 +947,10 @@ note_vague_linkage_fn (tree decl) vec_safe_push (deferred_fns, decl); } -/* As above, but for variable template instantiations. */ +/* As above, but for variables with static storage duration. */ void -note_variable_template_instantiation (tree decl) +note_static_storage_variable (tree decl) { vec_safe_push (pending_statics, decl); } diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc index 8db662c0267..b68407a3499 100644 --- a/gcc/cp/module.cc +++ b/gcc/cp/module.cc @@ -11775,6 +11775,11 @@ trees_in::read_var_def (tree decl, tree maybe_template) DECL_INITIALIZED_P (decl) = true; if (maybe_dup && DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P (maybe_dup)) DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P (decl) = true; + if (DECL_CLASS_SCOPE_P (decl) + && !DECL_VTABLE_OR_VTT_P (decl) + && (!DECL_TEMPLATE_INFO (decl) + || DECL_IMPLICIT_INSTANTIATION (decl))) + note_static_storage_variable (decl); } DECL_INITIAL (decl) = init; if (!dyn_init) diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc index f82d018c981..7c38594b82d 100644 --- a/gcc/cp/pt.cc +++ b/gcc/cp/pt.cc @@ -27256,7 +27256,12 @@ instantiate_decl (tree d, bool defer_ok, bool expl_inst_class_mem_p) { set_instantiating_module (d); if (variable_template_p (gen_tmpl)) - note_variable_template_instantiation (d); + { + if (DECL_CLASS_SCOPE_P (d)) + note_static_storage_variable (d); + else + vec_safe_push (static_decls, d); + } instantiate_body (td, args, d, false); } diff --git a/gcc/testsuite/g++.dg/modules/init-4_a.C b/gcc/testsuite/g++.dg/modules/init-4_a.C new file mode 100644 index 00000000000..e0eb97b474e --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/init-4_a.C @@ -0,0 +1,9 @@ +// PR c++/112899 +// { dg-additional-options "-fmodules-ts" } +// { dg-module-cmi M } + +export module M; + +export struct A { + static constexpr int x = -1; +}; diff --git a/gcc/testsuite/g++.dg/modules/init-4_b.C b/gcc/testsuite/g++.dg/modules/init-4_b.C new file mode 100644 index 00000000000..d28017a1d14 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/init-4_b.C @@ -0,0 +1,11 @@ +// PR c++/112899 +// { dg-module-do run } +// { dg-additional-options "-fmodules-ts" } + +import M; + +int main() { + const int& x = A::x; + if (x != -1) + __builtin_abort(); +} diff --git a/gcc/testsuite/g++.dg/modules/init-6_a.H b/gcc/testsuite/g++.dg/modules/init-6_a.H new file mode 100644 index 00000000000..a48d90d7aa7 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/init-6_a.H @@ -0,0 +1,12 @@ +// { dg-additional-options "-fmodule-header" } +// { dg-module-cmi {} } + +template +struct __from_chars_alnum_to_val_table { + static inline int value = 42; +}; + +inline unsigned char +__from_chars_alnum_to_val() { + return __from_chars_alnum_to_val_table::value; +} diff --git a/gcc/testsuite/g++.dg/modules/init-6_b.C b/gcc/testsuite/g++.dg/modules/init-6_b.C new file mode 100644 index 00000000000..6bb0e83c3ac --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/init-6_b.C @@ -0,0 +1,8 @@ +// { dg-module-do link } +// { dg-additional-options "-fmodules-ts" } + +import "init-6_a.H"; + +int main() { + __from_chars_alnum_to_val(); +}