diff mbox series

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

Message ID 65aba461.a70a0220.7fd5c.ad24@mx.google.com
State New
Headers
Series [v3] c++/modules: Emit definitions of ODR-used static members imported from modules [PR112899] |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gcc_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 success Testing passed

Commit Message

Nathaniel Shead Jan. 20, 2024, 10:45 a.m. UTC 
  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

Comments

Jason Merrill Jan. 24, 2024, 8:24 p.m. UTC | #1 
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
diff mbox series

Patch

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