| Message ID | 659f86dd.050a0220.c8110.0a09@mx.google.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++/modules: Support thread_local statics in header modules [PR113292] | expand |
On 1/11/24 01:12, Nathaniel Shead wrote: > Bootstrapped and regtested on x86_64-pc-linux-gnu. OK for trunk? > > -- >8 -- > > Currently, thread_locals in header modules cause ICEs. This patch makes > the required changes for them to work successfully. > > Functions exported by a module need DECL_CONTEXT to be set, so we > inherit it from the variable we're wrapping. > > We additionally require writing the DECL_TLS_MODEL for thread-local > variables to the module interface, and the TLS wrapper function needs to > have its DECL_BEFRIENDING_CLASSES written too as this is used to > retrieve what VAR_DECL it's a wrapper for when emitting a definition at > end of TU processing. > > PR c++/113292 > > gcc/cp/ChangeLog: > * decl2.cc (get_tls_wrapper_fn): Set DECL_CONTEXT. > (c_parse_final_cleanups): Suppress warning for no definition of > TLS wrapper functions in header modules. > * module.cc (trees_out::lang_decl_vals): Write wrapped variable > for TLS wrapper functions. > (trees_in::lang_decl_vals): Read it. > (trees_out::decl_value): Write TLS model for thread-local vars. > (trees_in::decl_value): Read it for new decls. Remember to emit > definitions of TLS wrapper functions later. > > gcc/testsuite/ChangeLog: > > * g++.dg/modules/pr113292_a.H: New test. > * g++.dg/modules/pr113292_b.C: New test. > * g++.dg/modules/pr113292_c.C: New test. > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> > --- > gcc/cp/decl2.cc | 10 ++++--- > gcc/cp/module.cc | 22 +++++++++++++++ > gcc/testsuite/g++.dg/modules/pr113292_a.H | 34 +++++++++++++++++++++++ > gcc/testsuite/g++.dg/modules/pr113292_b.C | 13 +++++++++ > gcc/testsuite/g++.dg/modules/pr113292_c.C | 11 ++++++++ > 5 files changed, 86 insertions(+), 4 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/modules/pr113292_a.H > create mode 100644 gcc/testsuite/g++.dg/modules/pr113292_b.C > create mode 100644 gcc/testsuite/g++.dg/modules/pr113292_c.C > > diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc > index fb996561f1b..ab348f8ecb7 100644 > --- a/gcc/cp/decl2.cc > +++ b/gcc/cp/decl2.cc > @@ -3860,6 +3860,7 @@ get_tls_wrapper_fn (tree var) > TREE_PUBLIC (fn) = TREE_PUBLIC (var); > DECL_ARTIFICIAL (fn) = true; > DECL_IGNORED_P (fn) = 1; > + DECL_CONTEXT (fn) = DECL_CONTEXT (var); > /* The wrapper is inline and emitted everywhere var is used. */ > DECL_DECLARED_INLINE_P (fn) = true; > if (TREE_PUBLIC (var)) > @@ -5289,10 +5290,11 @@ c_parse_final_cleanups (void) > #pragma interface, etc.) we decided not to emit the > definition here. */ > && !DECL_INITIAL (decl) > - /* A defaulted fn in a header module can be synthesized on > - demand later. (In non-header modules we should have > - synthesized it above.) */ > - && !(DECL_DEFAULTED_FN (decl) && header_module_p ()) > + /* A defaulted fn or TLS wrapper in a header module can be > + synthesized on demand later. (In non-header modules we > + should have synthesized it above.) */ > + && !(header_module_p () Hmm, should this be !module_attach_p instead of header_module_p? The patch is OK, that can change separately if appropriate. Jason
On Mon, Jan 15, 2024 at 05:38:25PM -0500, Jason Merrill wrote: > On 1/11/24 01:12, Nathaniel Shead wrote: > > Bootstrapped and regtested on x86_64-pc-linux-gnu. OK for trunk? > > > > -- >8 -- > > > > Currently, thread_locals in header modules cause ICEs. This patch makes > > the required changes for them to work successfully. > > > > Functions exported by a module need DECL_CONTEXT to be set, so we > > inherit it from the variable we're wrapping. > > > > We additionally require writing the DECL_TLS_MODEL for thread-local > > variables to the module interface, and the TLS wrapper function needs to > > have its DECL_BEFRIENDING_CLASSES written too as this is used to > > retrieve what VAR_DECL it's a wrapper for when emitting a definition at > > end of TU processing. > > > > PR c++/113292 > > > > gcc/cp/ChangeLog: > > * decl2.cc (get_tls_wrapper_fn): Set DECL_CONTEXT. > > (c_parse_final_cleanups): Suppress warning for no definition of > > TLS wrapper functions in header modules. > > * module.cc (trees_out::lang_decl_vals): Write wrapped variable > > for TLS wrapper functions. > > (trees_in::lang_decl_vals): Read it. > > (trees_out::decl_value): Write TLS model for thread-local vars. > > (trees_in::decl_value): Read it for new decls. Remember to emit > > definitions of TLS wrapper functions later. > > > > gcc/testsuite/ChangeLog: > > > > * g++.dg/modules/pr113292_a.H: New test. > > * g++.dg/modules/pr113292_b.C: New test. > > * g++.dg/modules/pr113292_c.C: New test. > > > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> > > --- > > gcc/cp/decl2.cc | 10 ++++--- > > gcc/cp/module.cc | 22 +++++++++++++++ > > gcc/testsuite/g++.dg/modules/pr113292_a.H | 34 +++++++++++++++++++++++ > > gcc/testsuite/g++.dg/modules/pr113292_b.C | 13 +++++++++ > > gcc/testsuite/g++.dg/modules/pr113292_c.C | 11 ++++++++ > > 5 files changed, 86 insertions(+), 4 deletions(-) > > create mode 100644 gcc/testsuite/g++.dg/modules/pr113292_a.H > > create mode 100644 gcc/testsuite/g++.dg/modules/pr113292_b.C > > create mode 100644 gcc/testsuite/g++.dg/modules/pr113292_c.C > > > > diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc > > index fb996561f1b..ab348f8ecb7 100644 > > --- a/gcc/cp/decl2.cc > > +++ b/gcc/cp/decl2.cc > > @@ -3860,6 +3860,7 @@ get_tls_wrapper_fn (tree var) > > TREE_PUBLIC (fn) = TREE_PUBLIC (var); > > DECL_ARTIFICIAL (fn) = true; > > DECL_IGNORED_P (fn) = 1; > > + DECL_CONTEXT (fn) = DECL_CONTEXT (var); > > /* The wrapper is inline and emitted everywhere var is used. */ > > DECL_DECLARED_INLINE_P (fn) = true; > > if (TREE_PUBLIC (var)) > > @@ -5289,10 +5290,11 @@ c_parse_final_cleanups (void) > > #pragma interface, etc.) we decided not to emit the > > definition here. */ > > && !DECL_INITIAL (decl) > > - /* A defaulted fn in a header module can be synthesized on > > - demand later. (In non-header modules we should have > > - synthesized it above.) */ > > - && !(DECL_DEFAULTED_FN (decl) && header_module_p ()) > > + /* A defaulted fn or TLS wrapper in a header module can be > > + synthesized on demand later. (In non-header modules we > > + should have synthesized it above.) */ > > + && !(header_module_p () > > Hmm, should this be !module_attach_p instead of header_module_p? > > The patch is OK, that can change separately if appropriate. > > Jason > Thanks. I used 'header_module_p' because that matches this condition in decl2.cc:5117: if (header_module_p ()) /* A header modules initializations are handled in its importer. */ continue; Which makes sense to me, I think: a header unit doesn't have an object file to emit initialisations into, and it must instead be handled in its importer, whereas other modules should be able to emit such initialisations into their own TU.
diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc index fb996561f1b..ab348f8ecb7 100644 --- a/gcc/cp/decl2.cc +++ b/gcc/cp/decl2.cc @@ -3860,6 +3860,7 @@ get_tls_wrapper_fn (tree var) TREE_PUBLIC (fn) = TREE_PUBLIC (var); DECL_ARTIFICIAL (fn) = true; DECL_IGNORED_P (fn) = 1; + DECL_CONTEXT (fn) = DECL_CONTEXT (var); /* The wrapper is inline and emitted everywhere var is used. */ DECL_DECLARED_INLINE_P (fn) = true; if (TREE_PUBLIC (var)) @@ -5289,10 +5290,11 @@ c_parse_final_cleanups (void) #pragma interface, etc.) we decided not to emit the definition here. */ && !DECL_INITIAL (decl) - /* A defaulted fn in a header module can be synthesized on - demand later. (In non-header modules we should have - synthesized it above.) */ - && !(DECL_DEFAULTED_FN (decl) && header_module_p ()) + /* A defaulted fn or TLS wrapper in a header module can be + synthesized on demand later. (In non-header modules we + should have synthesized it above.) */ + && !(header_module_p () + && (DECL_DEFAULTED_FN (decl) || decl_tls_wrapper_p (decl))) /* Don't complain if the template was defined. */ && !(DECL_TEMPLATE_INSTANTIATION (decl) && DECL_INITIAL (DECL_TEMPLATE_RESULT diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc index 9bb6d2643d8..e66838b2e6e 100644 --- a/gcc/cp/module.cc +++ b/gcc/cp/module.cc @@ -6948,6 +6948,9 @@ trees_out::lang_decl_vals (tree t) if (streaming_p ()) wi (lang->u.fn.u5.fixed_offset); } + else if (decl_tls_wrapper_p (t)) + /* The wrapped variable. */ + WT (lang->u.fn.befriending_classes); else WT (lang->u.fn.u5.cloned_function); @@ -7027,6 +7030,8 @@ trees_in::lang_decl_vals (tree t) RT (lang->u.fn.befriending_classes); lang->u.fn.u5.fixed_offset = wi (); } + else if (decl_tls_wrapper_p (t)) + RT (lang->u.fn.befriending_classes); else RT (lang->u.fn.u5.cloned_function); @@ -7926,6 +7931,9 @@ trees_out::decl_value (tree decl, depset *dep) decl, cloned_p ? "" : "not "); } + if (streaming_p () && VAR_P (decl) && CP_DECL_THREAD_LOCAL_P (decl)) + u (decl_tls_model (decl)); + if (streaming_p ()) dump (dumper::TREE) && dump ("Written decl:%d %C:%N", tag, TREE_CODE (decl), decl); @@ -8273,6 +8281,13 @@ trees_in::decl_value () look like templates. */ if (!install_implicit_member (inner)) set_overrun (); + + /* When importing a TLS wrapper from a header unit, we haven't + actually emitted its definition yet. Remember it so we can + do this later. */ + if (state->is_header () + && decl_tls_wrapper_p (decl)) + note_vague_linkage_fn (decl); } else { @@ -8356,6 +8371,13 @@ trees_in::decl_value () } } + if (VAR_P (decl) && CP_DECL_THREAD_LOCAL_P (decl)) + { + enum tls_model model = tls_model (u ()); + if (is_new) + set_decl_tls_model (decl, model); + } + if (!NAMESPACE_SCOPE_P (inner) && ((TREE_CODE (inner) == TYPE_DECL && !is_typedef diff --git a/gcc/testsuite/g++.dg/modules/pr113292_a.H b/gcc/testsuite/g++.dg/modules/pr113292_a.H new file mode 100644 index 00000000000..90ece2ec3f3 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr113292_a.H @@ -0,0 +1,34 @@ +// PR c++/113292 +// { dg-additional-options "-fmodule-header" } +// { dg-module-cmi {} } + +struct test { + static const test& get_instance() { + return instance; + } + static thread_local test instance; +}; + + +template <typename T> +struct test_template { + static const test_template& get_instance() { + return instance; + } + static thread_local test_template instance; + + template <typename U> + static const test_template& get_template_instance() { + return template_instance<U>; + } + + template <typename U> + static thread_local test_template template_instance; +}; + +template <typename T> +thread_local test_template<T> test_template<T>::instance; + +template <typename T> +template <typename U> +thread_local test_template<T> test_template<T>::template_instance; diff --git a/gcc/testsuite/g++.dg/modules/pr113292_b.C b/gcc/testsuite/g++.dg/modules/pr113292_b.C new file mode 100644 index 00000000000..fc582a5a0cf --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr113292_b.C @@ -0,0 +1,13 @@ +// PR c++/113292 +// { dg-additional-options "-fmodules-ts" } + +import "pr113292_a.H"; + +// provide a definition of 'instance' so things link +thread_local test test::instance; + +void instantiate() { + auto& instance = test::get_instance(); + auto& t_instance = test_template<int>::get_instance(); + auto& tt_instance = test_template<int>::get_template_instance<double>(); +}; diff --git a/gcc/testsuite/g++.dg/modules/pr113292_c.C b/gcc/testsuite/g++.dg/modules/pr113292_c.C new file mode 100644 index 00000000000..aa3f32ae818 --- /dev/null +++ b/gcc/testsuite/g++.dg/modules/pr113292_c.C @@ -0,0 +1,11 @@ +// PR c++/113292 +// { dg-module-do link } +// { dg-additional-options "-fmodules-ts" } + +import "pr113292_a.H"; + +int main() { + auto& instance = test::get_instance(); + auto& t_instance = test_template<int>::get_instance(); + auto& tt_instance = test_template<int>::get_template_instance<double>(); +}
Bootstrapped and regtested on x86_64-pc-linux-gnu. OK for trunk? -- >8 -- Currently, thread_locals in header modules cause ICEs. This patch makes the required changes for them to work successfully. Functions exported by a module need DECL_CONTEXT to be set, so we inherit it from the variable we're wrapping. We additionally require writing the DECL_TLS_MODEL for thread-local variables to the module interface, and the TLS wrapper function needs to have its DECL_BEFRIENDING_CLASSES written too as this is used to retrieve what VAR_DECL it's a wrapper for when emitting a definition at end of TU processing. PR c++/113292 gcc/cp/ChangeLog: * decl2.cc (get_tls_wrapper_fn): Set DECL_CONTEXT. (c_parse_final_cleanups): Suppress warning for no definition of TLS wrapper functions in header modules. * module.cc (trees_out::lang_decl_vals): Write wrapped variable for TLS wrapper functions. (trees_in::lang_decl_vals): Read it. (trees_out::decl_value): Write TLS model for thread-local vars. (trees_in::decl_value): Read it for new decls. Remember to emit definitions of TLS wrapper functions later. gcc/testsuite/ChangeLog: * g++.dg/modules/pr113292_a.H: New test. * g++.dg/modules/pr113292_b.C: New test. * g++.dg/modules/pr113292_c.C: New test. Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> --- gcc/cp/decl2.cc | 10 ++++--- gcc/cp/module.cc | 22 +++++++++++++++ gcc/testsuite/g++.dg/modules/pr113292_a.H | 34 +++++++++++++++++++++++ gcc/testsuite/g++.dg/modules/pr113292_b.C | 13 +++++++++ gcc/testsuite/g++.dg/modules/pr113292_c.C | 11 ++++++++ 5 files changed, 86 insertions(+), 4 deletions(-) create mode 100644 gcc/testsuite/g++.dg/modules/pr113292_a.H create mode 100644 gcc/testsuite/g++.dg/modules/pr113292_b.C create mode 100644 gcc/testsuite/g++.dg/modules/pr113292_c.C