:Re: [PATCH v2] c++/modules: Emit definitions of ODR-used static members imported from modules [PR112899]

[Patrick Palka <ppalka@redhat.com>]

On Wed, 3 Jan 2024, Nathaniel Shead wrote:

> Linaro CI tells me that this patch caused regressions on ARM. I don't
> have an ARM machine available to test on, but it appears to have been
> caused by attempting to stream vtables as static data members, and ARM
> having different behaviour with regards to when DECL_INTERFACE_KNOWN is
> marked on vtables.
> 
> I don't think treating vtables as static members here is desirable
> anyway, though, so this version of the patch explicitly ignores them.
> 
> Manually checking the assembly output for some of the noted regressions
> on ARM with a cross-compiler, this seems to have worked. Otherwise
> bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk if it
> passes Linaro CI on ARM this time?
> 
> -- >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.

Seems reasonable to me

Just FYI for anyone else wondering about namespace-scope variables,
it seems during stream-in they're added to a separate 'static_decls'
vector via add_module_namespace_decl which wrapup_namespace_globals
later handles emitting their definitions, so it makes sense to me to
have analogous handling for class-scope variables.

> 
> 	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 <nathanieloshead@gmail.com>
> ---
>  gcc/cp/cp-tree.h                        |  2 +-
>  gcc/cp/decl2.cc                         |  4 ++--
>  gcc/cp/module.cc                        |  5 +++++
>  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, 29 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..7edc0e37af0 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -11752,6 +11752,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_CONTEXT (decl)
> +	      && RECORD_OR_UNION_TYPE_P (DECL_CONTEXT (decl))
> +	      && !DECL_TEMPLATE_INFO (decl)
> +	      && !DECL_VTABLE_OR_VTT_P (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 f7063e09581..ce498750758 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -27150,7 +27150,7 @@ 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);
> +	note_static_storage_variable (d);

This is a latent issue/curiousity orthogonal to your patch, but it seems
inconsistent that namespace-scope variable template instantiations get
added to 'pending_statics' (which is intended for class-scope variables) rather
than to 'static_decls' (which is intended for namespace-scope variables).
I wonder if we should instead do:

diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index 154ac76cb65..25921c97ffe 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -27155,7 +27155,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 (gen_tmpl))
+	    note_static_data_member (d);
+	  else
+	    vec_safe_push (static_decls, d);
+	}
       instantiate_body (td, args, d, false);
     }
 

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