c++/modules: Stream section, tls_model, and comdat_group

  Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
Or should this wait for GCC16?

-- >8 --

While looking at PR c++/119154 I noticed some more properties that we
don't stream or check properly.  This patch adds the ones we were
missing, and adds checks that the values don't clash with existing
decls.

There's probably a lot more duplicate_decls-like work that should be
done in is_matching_decl (notably decl attributes?) but this is at least
a slight improvement to the status-quo.

gcc/cp/ChangeLog:

	* module.cc (trees_out::core_vals): Stream TLS model, section
	name, and comdat group.
	(trees_in::core_vals): Read them.
	(trees_out::decl_value): No longer need to stream TLS model.
	(trees_in::decl_value): Likewise; free symtab node of discarded
	decls when we're done with them.
	(trees_in::is_matching_decl): Add checks for corresponding TLS
	model and section name.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/attrib-3_a.C: New test.
	* g++.dg/modules/attrib-3_b.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/module.cc                          | 104 +++++++++++++++++++---
 gcc/testsuite/g++.dg/modules/attrib-3_a.C |   9 ++
 gcc/testsuite/g++.dg/modules/attrib-3_b.C |  14 +++
 3 files changed, 117 insertions(+), 10 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/attrib-3_a.C
 create mode 100644 gcc/testsuite/g++.dg/modules/attrib-3_b.C
  

On 3/10/25 9:52 AM, Nathaniel Shead wrote:
> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> Or should this wait for GCC16?
> 
> -- >8 --
> 
> While looking at PR c++/119154 I noticed some more properties that we
> don't stream or check properly.  This patch adds the ones we were
> missing, and adds checks that the values don't clash with existing
> decls.

These seem to me like properties that should be recomputed rather than 
streamed; aren't we already streaming the section attribute?

This comment also applies to the existing streaming of tls_model.

> There's probably a lot more duplicate_decls-like work that should be
> done in is_matching_decl (notably decl attributes?) but this is at least
> a slight improvement to the status-quo.
> 
> gcc/cp/ChangeLog:
> 
> 	* module.cc (trees_out::core_vals): Stream TLS model, section
> 	name, and comdat group.
> 	(trees_in::core_vals): Read them.
> 	(trees_out::decl_value): No longer need to stream TLS model.
> 	(trees_in::decl_value): Likewise; free symtab node of discarded
> 	decls when we're done with them.
> 	(trees_in::is_matching_decl): Add checks for corresponding TLS
> 	model and section name.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/attrib-3_a.C: New test.
> 	* g++.dg/modules/attrib-3_b.C: New test.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>   gcc/cp/module.cc                          | 104 +++++++++++++++++++---
>   gcc/testsuite/g++.dg/modules/attrib-3_a.C |   9 ++
>   gcc/testsuite/g++.dg/modules/attrib-3_b.C |  14 +++
>   3 files changed, 117 insertions(+), 10 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/modules/attrib-3_a.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/attrib-3_b.C
> 
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index 8e0fa3c5bfc..9aa86c939f6 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -6399,6 +6399,9 @@ trees_out::core_vals (tree t)
>   
>         /* Decls.  */
>       case VAR_DECL:
> +      if (streaming_p ())
> +	if (CP_DECL_THREAD_LOCAL_P (t))
> +	  u (decl_tls_model (t));
>         if (DECL_CONTEXT (t)
>   	  && TREE_CODE (DECL_CONTEXT (t)) != FUNCTION_DECL)
>   	{
> @@ -6691,6 +6694,22 @@ trees_out::core_vals (tree t)
>         break;
>       }
>   
> +  if (VAR_OR_FUNCTION_DECL_P (t)
> +      && (DECL_EXTERNAL (t) || TREE_PUBLIC (t) || TREE_STATIC (t)))
> +    {
> +      if (streaming_p ())
> +	{
> +	  const char *name = DECL_SECTION_NAME (t);
> +	  size_t len = name ? strlen (name) : 0;
> +	  str (name, len);
> +	}
> +
> +      tree comdat_group = NULL_TREE;
> +      if (DECL_ONE_ONLY (t))
> +	comdat_group = symtab_node::get (t)->get_comdat_group ();
> +      WT (comdat_group);
> +    }
> +
>     if (CODE_CONTAINS_STRUCT (code, TS_TYPED))
>       {
>         /* We want to stream the type of a expression-like nodes /after/
> @@ -6933,6 +6952,11 @@ trees_in::core_vals (tree t)
>   
>         /* Decls.  */
>       case VAR_DECL:
> +      if (CP_DECL_THREAD_LOCAL_P (t))
> +	{
> +	  enum tls_model model = tls_model (u());
> +	  set_decl_tls_model (t, model);
> +	}
>         if (DECL_CONTEXT (t)
>   	  && TREE_CODE (DECL_CONTEXT (t)) != FUNCTION_DECL)
>   	{
> @@ -7227,6 +7251,23 @@ trees_in::core_vals (tree t)
>         break;
>       }
>   
> +  if (VAR_OR_FUNCTION_DECL_P (t)
> +      && (DECL_EXTERNAL (t) || TREE_PUBLIC (t) || TREE_STATIC (t)))
> +    {
> +      size_t section_len = 0;
> +      const char *section_name = str (&section_len);
> +      if (section_len)
> +	set_decl_section_name (t, section_name);
> +
> +      if (tree comdat_group = tree_node ())
> +	{
> +	  if (TREE_CODE (t) == FUNCTION_DECL)
> +	    cgraph_node::get_create (t)->set_comdat_group (comdat_group);
> +	  else
> +	    varpool_node::get_create (t)->set_comdat_group (comdat_group);
> +	}
> +    }
> +
>     if (CODE_CONTAINS_STRUCT (code, TS_TYPED))
>       {
>         tree type = tree_node ();
> @@ -8316,9 +8357,6 @@ trees_out::decl_value (tree decl, depset *dep)
>   				   decl, cloned_p ? "" : "not ");
>       }
>   
> -  if (streaming_p () && VAR_P (decl) && CP_DECL_THREAD_LOCAL_P (decl))
> -    u (decl_tls_model (decl));
> -
>     if (streaming_p ())
>       dump (dumper::TREE) && dump ("Written decl:%d %C:%N", tag,
>   				 TREE_CODE (decl), decl);
> @@ -8757,6 +8795,12 @@ trees_in::decl_value ()
>   	    DECL_CONTEXT (parm) = e_inner;
>   	}
>   
> +      /* We will not need to track the comdat group etc of the
> +	 to-be-discarded decl, so remove it from the symbol table.  */
> +      if (VAR_OR_FUNCTION_DECL_P (inner))
> +	if (struct symtab_node *snode = symtab_node::get (inner))
> +	  snode->remove ();
> +
>         /* And our result is the existing node.  */
>         decl = existing;
>       }
> @@ -8802,13 +8846,6 @@ trees_in::decl_value ()
>   	}
>       }
>   
> -  if (VAR_P (decl) && CP_DECL_THREAD_LOCAL_P (decl))
> -    {
> -      enum tls_model model = tls_model (u ());
> -      if (is_new)
> -	set_decl_tls_model (decl, model);
> -    }
> -
>     if (!NAMESPACE_SCOPE_P (inner)
>         && ((TREE_CODE (inner) == TYPE_DECL
>   	   && !is_typedef
> @@ -12098,6 +12135,53 @@ trees_in::is_matching_decl (tree existing, tree decl, bool is_typedef)
>     if (!DECL_EXTERNAL (d_inner))
>       DECL_EXTERNAL (e_inner) = false;
>   
> +  if (VAR_P (d_inner))
> +    {
> +      if (CP_DECL_THREAD_LOCAL_P (d_inner) != CP_DECL_THREAD_LOCAL_P (e_inner))
> +	{
> +	  auto_diagnostic_group d;
> +	  error_at (DECL_SOURCE_LOCATION (decl),
> +		    "conflicting %<thread_local%> for %#qD", decl);
> +	  inform (DECL_SOURCE_LOCATION (existing), "existing declaration here");
> +	  return false;
> +	}
> +      if (CP_DECL_THREAD_LOCAL_P (d_inner)
> +	  && decl_tls_model (d_inner) != decl_tls_model (e_inner))
> +	{
> +	  auto_diagnostic_group d;
> +	  error_at (DECL_SOURCE_LOCATION (decl),
> +		    "conflicting TLS model %qs for %q#D",
> +		    tls_model_names[decl_tls_model (d_inner)], decl);
> +	  inform (DECL_SOURCE_LOCATION (existing),
> +		  "existing declaration here as %qs",
> +		  tls_model_names[decl_tls_model (e_inner)]);
> +	  return false;
> +	}
> +    }
> +
> +  if (VAR_OR_FUNCTION_DECL_P (d_inner))
> +    {
> +      if (DECL_SECTION_NAME (d_inner) != DECL_SECTION_NAME (e_inner))
> +	{
> +	  auto_diagnostic_group d;
> +	  if (DECL_SECTION_NAME (d_inner))
> +	    error_at (DECL_SOURCE_LOCATION (decl),
> +		      "conflicting section %qs for %q#D",
> +		      DECL_SECTION_NAME (d_inner), decl);
> +	  else
> +	    error_at (DECL_SOURCE_LOCATION (decl),
> +		      "conflicting section for %q#D", decl);
> +	  if (DECL_SECTION_NAME (e_inner))
> +	    inform (DECL_SOURCE_LOCATION (existing),
> +		    "existing declaration here in section %qs",
> +		    DECL_SECTION_NAME (e_inner));
> +	  else
> +	    inform (DECL_SOURCE_LOCATION (existing),
> +		    "existing declaration here with no section");
> +	  return false;
> +	}
> +    }
> +
>     if (TREE_CODE (decl) == TEMPLATE_DECL)
>       {
>         /* Merge default template arguments.  */
> diff --git a/gcc/testsuite/g++.dg/modules/attrib-3_a.C b/gcc/testsuite/g++.dg/modules/attrib-3_a.C
> new file mode 100644
> index 00000000000..ea265762cbf
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/attrib-3_a.C
> @@ -0,0 +1,9 @@
> +// { dg-additional-options "-fmodules" }
> +
> +export module M;
> +export extern "C++" {
> +  inline void a() __attribute__((section("bar")));
> +  inline void a() {}
> +
> +  inline void b() {}
> +}
> diff --git a/gcc/testsuite/g++.dg/modules/attrib-3_b.C b/gcc/testsuite/g++.dg/modules/attrib-3_b.C
> new file mode 100644
> index 00000000000..67c78567883
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/attrib-3_b.C
> @@ -0,0 +1,14 @@
> +// { dg-additional-options "-fmodules" }
> +// { dg-error "conflicting section" "" { target *-*-* } 0 }
> +
> +inline void a() {}  // { dg-message "here" }
> +
> +inline void b() __attribute__((section("qux")));
> +inline void b() {}  // { dg-message "here" }
> +
> +import M;
> +
> +int main() {
> +  a();
> +  b();
> +}
  

On Mon, Mar 10, 2025 at 02:52:07PM -0400, Jason Merrill wrote:
> On 3/10/25 9:52 AM, Nathaniel Shead wrote:
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> > Or should this wait for GCC16?
> > 
> > -- >8 --
> > 
> > While looking at PR c++/119154 I noticed some more properties that we
> > don't stream or check properly.  This patch adds the ones we were
> > missing, and adds checks that the values don't clash with existing
> > decls.
> 
> These seem to me like properties that should be recomputed rather than
> streamed; aren't we already streaming the section attribute?
> 

Hm, right; we're streaming the attributes but in general we don't
reapply any of the effects they have that aren't streamed regardless.
We should probably do that somehow; it looks like using decl_attributes
directly for this might do too much so I suppose we would need another
interface for this.

This is probably the way to go to fix PR108080, too; rather than messing
around with trying to work out how to stream OPTIMIZATION_NODEs etc. we
can just reapply the attribute, which also would probably have the
expected behaviour in the case of mismatching optimisation flags between
exporter and consumer.

> This comment also applies to the existing streaming of tls_model.
> 

I assume we can just use 'decl_default_tls_model' in the no-attribute
case?  Is there anything we should worry about wrt to it potentially
providing different results in different modules (due to different
choices of -ftls-model)?

Also, while reworking my other patch I'm now semi-convinced that there's
no need to stream comdat group, it should be recalculated correctly
anyway in all cases that I could find.  But I definitely might have
missed something; either way it hasn't bitten us yet.

Nathaniel
  

On 3/12/25 10:57 AM, Nathaniel Shead wrote:
> On Mon, Mar 10, 2025 at 02:52:07PM -0400, Jason Merrill wrote:
>> On 3/10/25 9:52 AM, Nathaniel Shead wrote:
>>> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
>>> Or should this wait for GCC16?
>>>
>>> -- >8 --
>>>
>>> While looking at PR c++/119154 I noticed some more properties that we
>>> don't stream or check properly.  This patch adds the ones we were
>>> missing, and adds checks that the values don't clash with existing
>>> decls.
>>
>> These seem to me like properties that should be recomputed rather than
>> streamed; aren't we already streaming the section attribute?
>>
> 
> Hm, right; we're streaming the attributes but in general we don't
> reapply any of the effects they have that aren't streamed regardless.
> We should probably do that somehow; it looks like using decl_attributes
> directly for this might do too much so I suppose we would need another
> interface for this.

I'd probably go ahead trying to use decl_attributes and deal with any 
problems.

> This is probably the way to go to fix PR108080, too; rather than messing
> around with trying to work out how to stream OPTIMIZATION_NODEs etc. we
> can just reapply the attribute, which also would probably have the
> expected behaviour in the case of mismatching optimisation flags between
> exporter and consumer.
> 
>> This comment also applies to the existing streaming of tls_model.
> 
> I assume we can just use 'decl_default_tls_model' in the no-attribute
> case?  Is there anything we should worry about wrt to it potentially
> providing different results in different modules (due to different
> choices of -ftls-model)?

I think then you get what you get, just as with headers.  If it breaks, 
you shouldn't build your project with inconsistent ABI flags.

> Also, while reworking my other patch I'm now semi-convinced that there's
> no need to stream comdat group, it should be recalculated correctly
> anyway in all cases that I could find.  But I definitely might have
> missed something; either way it hasn't bitten us yet.
> 
> Nathaniel
>