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

[Nathaniel Shead <nathanieloshead@gmail.com>]

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.