[PATCH] c++/modules: Support thread_local statics in header modules [PR113292]

[PATCH] c++/modules: Support thread_local statics in header modules [PR113292]

[Nathaniel Shead]

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 ()
+	       && (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>();
+}
-- 
2.43.0

Re: [PATCH] c++/modules: Support thread_local statics in header modules [PR113292]

[Jason Merrill]

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

Re: [PATCH] c++/modules: Support thread_local statics in header modules [PR113292]

[Nathaniel Shead]

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.