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

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

[Nathaniel Shead]

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 <nathanieloshead@gmail.com>
---
 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);
 	}
       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);
       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

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

[Nathaniel Shead]

(Whoops, forgot the '[PATCH]', fixed the subject in email.)

On Wed, Jan 03, 2024 at 09:40:55AM +1100, 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 <nathanieloshead@gmail.com>
> ---
>  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);
>  	}
>        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);
>        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
> 

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

[Nathaniel Shead]

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.

	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);
       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

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

[Patrick Palka]

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
> 
> 

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

[Jason Merrill]

On 1/2/24 17:40, Nathaniel Shead wrote:
> 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.

Hmm, this seems wrong to me; I'd think that static data members marked 
inline should be emitted in the module, and not in importers.

Jason

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

[Nathaniel Shead]

On Thu, Jan 04, 2024 at 03:31:50PM -0500, Jason Merrill wrote:
> On 1/2/24 17:40, Nathaniel Shead wrote:
> > 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.
> 
> Hmm, this seems wrong to me; I'd think that static data members marked
> inline should be emitted in the module, and not in importers.
> 
> Jason
> 

That's what I'd initially thought too, but this is at least consistent
with non-class inlines (variables and functions), which similarly only
get emitted in TUs that they're ODR-used rather than always (and only)
being emitted within the module.

I guess an alternative would be to change it around so that all
exported definitions are marked as needed in the module interface file
(and emitted there), and then setting some flag so that they're never
emitted in importers. I'm not entirely sure what flag that would be
though, I still haven't quite wrapped my head what controls what with
regards to this, and I'm not convinced it wouldn't break template
instantiations.

I wonder if this might also be related to the issue Nathan noted with
regards to block-scope class methods, which I haven't completely worked
out how to solve yet otherwise (see
https://gcc.gnu.org/pipermail/gcc-patches/2023-November/638223.html).

Nathaniel

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

[Jason Merrill]

On 1/4/24 17:24, Nathaniel Shead wrote:
> On Thu, Jan 04, 2024 at 03:31:50PM -0500, Jason Merrill wrote:
>> On 1/2/24 17:40, Nathaniel Shead wrote:
>>> 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.
>>
>> Hmm, this seems wrong to me; I'd think that static data members marked
>> inline should be emitted in the module, and not in importers.
> 
> That's what I'd initially thought too, but this is at least consistent
> with non-class inlines (variables and functions), which similarly only
> get emitted in TUs that they're ODR-used rather than always (and only)
> being emitted within the module.
> 
> I guess an alternative would be to change it around so that all
> exported definitions are marked as needed in the module interface file
> (and emitted there), and then setting some flag so that they're never
> emitted in importers.

Yes, that would be my expectation.  What do other modules 
implementations do?

> I'm not entirely sure what flag that would be
> though, I still haven't quite wrapped my head what controls what with
> regards to this, and I'm not convinced it wouldn't break template
> instantiations.

I would guess avoid emitting if DECL_MODULE_IMPORT_P && 
DECL_MODULE_ATTACH_P.

> I wonder if this might also be related to the issue Nathan noted with
> regards to block-scope class methods, which I haven't completely worked
> out how to solve yet otherwise (see
> https://gcc.gnu.org/pipermail/gcc-patches/2023-November/638223.html).

Indeed.

Jason

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

[Nathaniel Shead]

On Thu, Jan 04, 2024 at 05:42:34PM -0500, Jason Merrill wrote:
> On 1/4/24 17:24, Nathaniel Shead wrote:
> > On Thu, Jan 04, 2024 at 03:31:50PM -0500, Jason Merrill wrote:
> > > On 1/2/24 17:40, Nathaniel Shead wrote:
> > > > 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.
> > > 
> > > Hmm, this seems wrong to me; I'd think that static data members marked
> > > inline should be emitted in the module, and not in importers.
> > 
> > That's what I'd initially thought too, but this is at least consistent
> > with non-class inlines (variables and functions), which similarly only
> > get emitted in TUs that they're ODR-used rather than always (and only)
> > being emitted within the module.
> > 
> > I guess an alternative would be to change it around so that all
> > exported definitions are marked as needed in the module interface file
> > (and emitted there), and then setting some flag so that they're never
> > emitted in importers.
> 
> Yes, that would be my expectation.  What do other modules implementations
> do?

Clang only emits ODR-used declarations (same as GCC currently).

MSVC emits all inline variables (whether exported or not) but no inline
functions.

> > I'm not entirely sure what flag that would be
> > though, I still haven't quite wrapped my head what controls what with
> > regards to this, and I'm not convinced it wouldn't break template
> > instantiations.
> 
> I would guess avoid emitting if DECL_MODULE_IMPORT_P &&
> DECL_MODULE_ATTACH_P.

Ah yup, that would make sense. I guess, thinking about it more, we
should then also ensure that all TREE_PUBLIC declarations are emitted in
the module interface even if not exported, since they may be needed in
implementation units?

> > I wonder if this might also be related to the issue Nathan noted with
> > regards to block-scope class methods, which I haven't completely worked
> > out how to solve yet otherwise (see
> > https://gcc.gnu.org/pipermail/gcc-patches/2023-November/638223.html).
> 
> Indeed.
> 
> Jason
> 

I'll give implementing this a try then, if you think that would be
sensible. (Where by "this" I mean "emit all public declarations in
module interface files, whether used or not".)

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

[Jason Merrill]

On 1/4/24 18:02, Nathaniel Shead wrote:
> On Thu, Jan 04, 2024 at 05:42:34PM -0500, Jason Merrill wrote:
>> On 1/4/24 17:24, Nathaniel Shead wrote:
>>> On Thu, Jan 04, 2024 at 03:31:50PM -0500, Jason Merrill wrote:
>>>> On 1/2/24 17:40, Nathaniel Shead wrote:
>>>>> 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.
>>>>
>>>> Hmm, this seems wrong to me; I'd think that static data members marked
>>>> inline should be emitted in the module, and not in importers.
>>>
>>> That's what I'd initially thought too, but this is at least consistent
>>> with non-class inlines (variables and functions), which similarly only
>>> get emitted in TUs that they're ODR-used rather than always (and only)
>>> being emitted within the module.
>>>
>>> I guess an alternative would be to change it around so that all
>>> exported definitions are marked as needed in the module interface file
>>> (and emitted there), and then setting some flag so that they're never
>>> emitted in importers.
>>
>> Yes, that would be my expectation.  What do other modules implementations
>> do?
> 
> Clang only emits ODR-used declarations (same as GCC currently).
> 
> MSVC emits all inline variables (whether exported or not) but no inline
> functions.

Hmm, not a strong vote for my direction.

>>> I'm not entirely sure what flag that would be
>>> though, I still haven't quite wrapped my head what controls what with
>>> regards to this, and I'm not convinced it wouldn't break template
>>> instantiations.
>>
>> I would guess avoid emitting if DECL_MODULE_IMPORT_P &&
>> DECL_MODULE_ATTACH_P.
> 
> Ah yup, that would make sense. I guess, thinking about it more, we
> should then also ensure that all TREE_PUBLIC declarations are emitted in
> the module interface even if not exported, since they may be needed in
> implementation units?

That would also make sense to me; since we know the module interface 
unit is compiled to an object file, everything vague linkage in it can 
go there.

>>> I wonder if this might also be related to the issue Nathan noted with
>>> regards to block-scope class methods, which I haven't completely worked
>>> out how to solve yet otherwise (see
>>> https://gcc.gnu.org/pipermail/gcc-patches/2023-November/638223.html).
>>
>> Indeed.
> 
> I'll give implementing this a try then, if you think that would be
> sensible. (Where by "this" I mean "emit all public declarations in
> module interface files, whether used or not".)

I'd like to hear Nathan's thoughts on the matter first, since he's the 
modules implementation designer.

Jason

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

[Nathan Sidwell]

Richard Smith & I discussed whether we should use the module interface's 
capability of giving vague linkage entities a strong location. I didn't want to 
go messing with that, 'cos it was changing yet more stuff.

But, perhaps we should revisit that?  Any keyless polymorphic class in module 
purview gets its vtables etc emitted in the module's object file?  Likewise 
these kinds of entities.

cc'ing Iain, who probably knows more about Clang's state here.

nathan


On 1/4/24 21:06, Jason Merrill wrote:
> On 1/4/24 18:02, Nathaniel Shead wrote:
>> On Thu, Jan 04, 2024 at 05:42:34PM -0500, Jason Merrill wrote:
>>> On 1/4/24 17:24, Nathaniel Shead wrote:
>>>> On Thu, Jan 04, 2024 at 03:31:50PM -0500, Jason Merrill wrote:
>>>>> On 1/2/24 17:40, Nathaniel Shead wrote:
>>>>>> 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.
>>>>>
>>>>> Hmm, this seems wrong to me; I'd think that static data members marked
>>>>> inline should be emitted in the module, and not in importers.
>>>>
>>>> That's what I'd initially thought too, but this is at least consistent
>>>> with non-class inlines (variables and functions), which similarly only
>>>> get emitted in TUs that they're ODR-used rather than always (and only)
>>>> being emitted within the module.
>>>>
>>>> I guess an alternative would be to change it around so that all
>>>> exported definitions are marked as needed in the module interface file
>>>> (and emitted there), and then setting some flag so that they're never
>>>> emitted in importers.
>>>
>>> Yes, that would be my expectation.  What do other modules implementations
>>> do?
>>
>> Clang only emits ODR-used declarations (same as GCC currently).
>>
>> MSVC emits all inline variables (whether exported or not) but no inline
>> functions.
> 
> Hmm, not a strong vote for my direction.
> 
>>>> I'm not entirely sure what flag that would be
>>>> though, I still haven't quite wrapped my head what controls what with
>>>> regards to this, and I'm not convinced it wouldn't break template
>>>> instantiations.
>>>
>>> I would guess avoid emitting if DECL_MODULE_IMPORT_P &&
>>> DECL_MODULE_ATTACH_P.
>>
>> Ah yup, that would make sense. I guess, thinking about it more, we
>> should then also ensure that all TREE_PUBLIC declarations are emitted in
>> the module interface even if not exported, since they may be needed in
>> implementation units?
> 
> That would also make sense to me; since we know the module interface unit is 
> compiled to an object file, everything vague linkage in it can go there.
> 
>>>> I wonder if this might also be related to the issue Nathan noted with
>>>> regards to block-scope class methods, which I haven't completely worked
>>>> out how to solve yet otherwise (see
>>>> https://gcc.gnu.org/pipermail/gcc-patches/2023-November/638223.html).
>>>
>>> Indeed.
>>
>> I'll give implementing this a try then, if you think that would be
>> sensible. (Where by "this" I mean "emit all public declarations in
>> module interface files, whether used or not".)
> 
> I'd like to hear Nathan's thoughts on the matter first, since he's the modules 
> implementation designer.
> 
> Jason
> 

-- 
Nathan Sidwell

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

[Iain Sandoe]

> On 6 Jan 2024, at 22:30, Nathan Sidwell <nathan@acm.org> wrote:
> 
> Richard Smith & I discussed whether we should use the module interface's capability of giving vague linkage entities a strong location. I didn't want to go messing with that, 'cos it was changing yet more stuff.
> 
> But, perhaps we should revisit that?  Any keyless polymorphic class in module purview gets its vtables etc emitted in the module's object file?  Likewise these kinds of entities.
> 
> cc'ing Iain, who probably knows more about Clang's state here.

I have been trying to keep up with this thread, but not sure if I can throw a whole lot of light on things.

There is an on-going attempt (now some 3 or 4 papers in) to try and figure out how to handle `static inline` entities at least at file scope - but that appears to be a different case (I can try an locate the latest paper on this if needed; the topic was discussed in Varna and Kona, but no new paper yet - perhaps Michael [Spencer] will bring a paper in Tokyo).

clang ran into some issues with vtables and that resulted in some discussion about whether there should be an amendment to the Itanium ABI to deal with the module-specific stuff.

https://github.com/itanium-cxx-abi/cxx-abi/issues/170

https://github.com/llvm/llvm-project/pull/75912#discussion_r1444150069

Sorry I cannot be much more specific at present,
Iain

> 
> nathan
> 
> 
> On 1/4/24 21:06, Jason Merrill wrote:
>> On 1/4/24 18:02, Nathaniel Shead wrote:
>>> On Thu, Jan 04, 2024 at 05:42:34PM -0500, Jason Merrill wrote:
>>>> On 1/4/24 17:24, Nathaniel Shead wrote:
>>>>> On Thu, Jan 04, 2024 at 03:31:50PM -0500, Jason Merrill wrote:
>>>>>> On 1/2/24 17:40, Nathaniel Shead wrote:
>>>>>>> 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.
>>>>>> 
>>>>>> Hmm, this seems wrong to me; I'd think that static data members marked
>>>>>> inline should be emitted in the module, and not in importers.
>>>>> 
>>>>> That's what I'd initially thought too, but this is at least consistent
>>>>> with non-class inlines (variables and functions), which similarly only
>>>>> get emitted in TUs that they're ODR-used rather than always (and only)
>>>>> being emitted within the module.
>>>>> 
>>>>> I guess an alternative would be to change it around so that all
>>>>> exported definitions are marked as needed in the module interface file
>>>>> (and emitted there), and then setting some flag so that they're never
>>>>> emitted in importers.
>>>> 
>>>> Yes, that would be my expectation.  What do other modules implementations
>>>> do?
>>> 
>>> Clang only emits ODR-used declarations (same as GCC currently).
>>> 
>>> MSVC emits all inline variables (whether exported or not) but no inline
>>> functions.
>> Hmm, not a strong vote for my direction.
>>>>> I'm not entirely sure what flag that would be
>>>>> though, I still haven't quite wrapped my head what controls what with
>>>>> regards to this, and I'm not convinced it wouldn't break template
>>>>> instantiations.
>>>> 
>>>> I would guess avoid emitting if DECL_MODULE_IMPORT_P &&
>>>> DECL_MODULE_ATTACH_P.
>>> 
>>> Ah yup, that would make sense. I guess, thinking about it more, we
>>> should then also ensure that all TREE_PUBLIC declarations are emitted in
>>> the module interface even if not exported, since they may be needed in
>>> implementation units?
>> That would also make sense to me; since we know the module interface unit is compiled to an object file, everything vague linkage in it can go there.
>>>>> I wonder if this might also be related to the issue Nathan noted with
>>>>> regards to block-scope class methods, which I haven't completely worked
>>>>> out how to solve yet otherwise (see
>>>>> https://gcc.gnu.org/pipermail/gcc-patches/2023-November/638223.html).
>>>> 
>>>> Indeed.
>>> 
>>> I'll give implementing this a try then, if you think that would be
>>> sensible. (Where by "this" I mean "emit all public declarations in
>>> module interface files, whether used or not".)
>> I'd like to hear Nathan's thoughts on the matter first, since he's the modules implementation designer.
>> Jason
> 
> -- 
> Nathan Sidwell
> 

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

[Jason Merrill]

On 1/8/24 04:21, Iain Sandoe wrote:
>> On 6 Jan 2024, at 22:30, Nathan Sidwell <nathan@acm.org> wrote:
>>
>> Richard Smith & I discussed whether we should use the module interface's capability of giving vague linkage entities a strong location. I didn't want to go messing with that, 'cos it was changing yet more stuff.
>>
>> But, perhaps we should revisit that?  Any keyless polymorphic class in module purview gets its vtables etc emitted in the module's object file?  Likewise these kinds of entities.
>>
>> cc'ing Iain, who probably knows more about Clang's state here.
> 
> I have been trying to keep up with this thread, but not sure if I can throw a whole lot of light on things.
> 
> There is an on-going attempt (now some 3 or 4 papers in) to try and figure out how to handle `static inline` entities at least at file scope - but that appears to be a different case (I can try an locate the latest paper on this if needed; the topic was discussed in Varna and Kona, but no new paper yet - perhaps Michael [Spencer] will bring a paper in Tokyo).
> 
> clang ran into some issues with vtables and that resulted in some discussion about whether there should be an amendment to the Itanium ABI to deal with the module-specific stuff.
> 
> https://github.com/itanium-cxx-abi/cxx-abi/issues/170
> 
> https://github.com/llvm/llvm-project/pull/75912#discussion_r1444150069
> 
> Sorry I cannot be much more specific at present,

That's pretty specific that vtables at least get emitted in the module 
whether or not there's a key function.  I've asked on that issue why 
this only applies to vtables.

Jason

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

[Patrick Palka]

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 <nathanieloshead@gmail.com>
> ---
>  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<bool _DecOnly>
  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<false>::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<false>::value'


By the way I ran into this when testing out std::print with modules:

  $ cat std.C
  export module std;
  export import <bits/stdc++.h>;

  $ 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<false>(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<false>::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!

>  	}
>        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);
>        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
> 
> 

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

[Nathaniel Shead]

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 <nathanieloshead@gmail.com>
> > ---
> >  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<bool _DecOnly>
>   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<false>::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<false>::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 <bits/stdc++.h>;
> 
>   $ 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<false>(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<false>::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 <nathanieloshead@gmail.com>
Reviewed-by: Patrick Palka <ppalka@redhat.com>
---
 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 <bool _DecOnly>
+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<false>::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();
+}
-- 
2.43.0

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

[Jason Merrill]

On 1/20/24 05:45, Nathaniel Shead wrote:
> On Fri, Jan 19, 2024 at 01:57:18PM -0500, Patrick Palka wrote:
>> On Wed, 3 Jan 2024, Nathaniel Shead wrote:
>>> +	  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<bool _DecOnly>
>>    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<false>::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<false>::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 <bits/stdc++.h>;
>>
>>    $ 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<false>(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<false>::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.

That seems wrong; the 'static_decls' vec is just for checking that 
static/inline variables got defined.

pending_statics has been used for template instantiations for a long 
time, for non-module code; let's not mess with that in a modules patch.

> 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.

Makes sense.

> 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'?

Let's call it note_vague_linkage_variable, to go with _fn just above.

> -- >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.

What about non-member variables marked inline, and non-member variable 
template instantiations?

Jason

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

[Nathaniel Shead]

On Wed, Jan 24, 2024 at 03:24:42PM -0500, Jason Merrill wrote:
> On 1/20/24 05:45, Nathaniel Shead wrote:
> > 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.
> 
> That seems wrong; the 'static_decls' vec is just for checking that
> static/inline variables got defined.
> 
> pending_statics has been used for template instantiations for a long time,
> for non-module code; let's not mess with that in a modules patch.
> 

OK, makes sense.

> > 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.
> 
> Makes sense.
> 
> > 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'?
> 
> Let's call it note_vague_linkage_variable, to go with _fn just above.
> 

Sounds good.

> > -- >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.
> 
> What about non-member variables marked inline, and non-member variable
> template instantiations?
> 
> Jason
> 

Non-member variables marked inline are already handled by 'static_decls'
via 'add_module_namespace_decl' and 'add_decl_to_level' during
stream-in, and then are later emitted from 'wrapup_namespace_globals'.

I'd assumed that non-member variable template instantiations would also
be handled by here, but that turns out not to be the case, since the
instantiations themselves are not (of course) namespace-scope decls.
I've added a case to the tests for this.

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 inlines 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_vague_linkage_variable): ...this.
	* decl2.cc (note_variable_template_instantiation): Rename to...
	(note_vague_linkage_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.
	* g++.dg/modules/init-6_a.H: New test.
	* g++.dg/modules/init-6_b.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
Reviewed-by: Patrick Palka <ppalka@redhat.com>
Reviewed-by: Jason Merrill <jason@redhat.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 +++++++++++
 gcc/testsuite/g++.dg/modules/init-6_a.H | 20 ++++++++++++++++++++
 gcc/testsuite/g++.dg/modules/init-6_b.C |  9 +++++++++
 8 files changed, 58 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 60e6dafc549..f46b448ce0d 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -7151,7 +7151,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_vague_linkage_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..f569d4045ec 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.  */
 
 void
-note_variable_template_instantiation (tree decl)
+note_vague_linkage_variable (tree decl)
 {
   vec_safe_push (pending_statics, decl);
 }
diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index f26b2265bce..6176801b7a7 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -11789,6 +11789,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_IMPLICIT_INSTANTIATION (decl)
+	      || (DECL_CLASS_SCOPE_P (decl)
+		  && !DECL_VTABLE_OR_VTT_P (decl)
+		  && !DECL_TEMPLATE_INFO (decl)))
+	    note_vague_linkage_variable (decl);
 	}
       DECL_INITIAL (decl) = init;
       if (!dyn_init)
diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index 74013533b0f..f5bf159a879 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -27300,7 +27300,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_vague_linkage_variable (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..1c3fe4535d4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/init-6_a.H
@@ -0,0 +1,20 @@
+// { dg-additional-options "-fmodule-header" }
+// { dg-module-cmi {} }
+
+template <bool _DecOnly>
+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<false>::value;
+}
+
+template <bool Foo>
+static inline int nonclass_value = 42;
+
+inline unsigned char
+get_nonclass_val() {
+  return nonclass_value<false>;
+}
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..d704968ec37
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/init-6_b.C
@@ -0,0 +1,9 @@
+// { dg-module-do link }
+// { dg-additional-options "-fmodules-ts" }
+
+import "init-6_a.H";
+
+int main() {
+  __from_chars_alnum_to_val();
+  get_nonclass_val();
+}
-- 
2.43.0

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

[Jason Merrill]

On 1/25/24 21:28, Nathaniel Shead wrote:
> On Wed, Jan 24, 2024 at 03:24:42PM -0500, Jason Merrill wrote:
>> On 1/20/24 05:45, Nathaniel Shead wrote:
>>> 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.
>>
>> That seems wrong; the 'static_decls' vec is just for checking that
>> static/inline variables got defined.
>>
>> pending_statics has been used for template instantiations for a long time,
>> for non-module code; let's not mess with that in a modules patch.
>>
> 
> OK, makes sense.
> 
>>> 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.
>>
>> Makes sense.
>>
>>> 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'?
>>
>> Let's call it note_vague_linkage_variable, to go with _fn just above.
>>
> 
> Sounds good.
> 
>>> -- >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.
>>
>> What about non-member variables marked inline, and non-member variable
>> template instantiations?
>>
>> Jason
>>
> 
> Non-member variables marked inline are already handled by 'static_decls'
> via 'add_module_namespace_decl' and 'add_decl_to_level' during
> stream-in, and then are later emitted from 'wrapup_namespace_globals'.
> 
> I'd assumed that non-member variable template instantiations would also
> be handled by here, but that turns out not to be the case, since the
> instantiations themselves are not (of course) namespace-scope decls.
> I've added a case to the tests for this.
> 
> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?

OK.

> -- >8 --
> 
> Static data members marked 'inline' should be emitted in TUs where they
> are ODR-used.  We need to make sure that inlines 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_vague_linkage_variable): ...this.
> 	* decl2.cc (note_variable_template_instantiation): Rename to...
> 	(note_vague_linkage_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.
> 	* g++.dg/modules/init-6_a.H: New test.
> 	* g++.dg/modules/init-6_b.C: New test.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> Reviewed-by: Patrick Palka <ppalka@redhat.com>
> Reviewed-by: Jason Merrill <jason@redhat.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 +++++++++++
>   gcc/testsuite/g++.dg/modules/init-6_a.H | 20 ++++++++++++++++++++
>   gcc/testsuite/g++.dg/modules/init-6_b.C |  9 +++++++++
>   8 files changed, 58 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 60e6dafc549..f46b448ce0d 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -7151,7 +7151,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_vague_linkage_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..f569d4045ec 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.  */
>   
>   void
> -note_variable_template_instantiation (tree decl)
> +note_vague_linkage_variable (tree decl)
>   {
>     vec_safe_push (pending_statics, decl);
>   }
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index f26b2265bce..6176801b7a7 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -11789,6 +11789,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_IMPLICIT_INSTANTIATION (decl)
> +	      || (DECL_CLASS_SCOPE_P (decl)
> +		  && !DECL_VTABLE_OR_VTT_P (decl)
> +		  && !DECL_TEMPLATE_INFO (decl)))
> +	    note_vague_linkage_variable (decl);
>   	}
>         DECL_INITIAL (decl) = init;
>         if (!dyn_init)
> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index 74013533b0f..f5bf159a879 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -27300,7 +27300,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_vague_linkage_variable (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..1c3fe4535d4
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/init-6_a.H
> @@ -0,0 +1,20 @@
> +// { dg-additional-options "-fmodule-header" }
> +// { dg-module-cmi {} }
> +
> +template <bool _DecOnly>
> +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<false>::value;
> +}
> +
> +template <bool Foo>
> +static inline int nonclass_value = 42;
> +
> +inline unsigned char
> +get_nonclass_val() {
> +  return nonclass_value<false>;
> +}
> 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..d704968ec37
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/init-6_b.C
> @@ -0,0 +1,9 @@
> +// { dg-module-do link }
> +// { dg-additional-options "-fmodules-ts" }
> +
> +import "init-6_a.H";
> +
> +int main() {
> +  __from_chars_alnum_to_val();
> +  get_nonclass_val();
> +}