c++/modules: Stream unmergeable temporaries by value again [PR114856]

Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk/14.2?

Another alternative would be to stream such !DECL_NAME temporaries with
a merge key of MK_unique rather than attempting to find the matching
(nonexistant) field of the class context.

-- >8 --

In r14-9266-g2823b4d96d9ec4 I gave all temporary vars a DECL_CONTEXT,
including those at namespace or global scope, so that they could be
properly merged across importers.  However, not all of these temporary
vars are actually supposed to be mergeable.

For instance, in the attached testcase we have an unnamed temporary var
used in the NSDMI of a class member, which cannot properly merged -- but
it also doesn't need to be, as it'll be thrown away when the class type
itself is merged anyway.

This patch reverts the change made above and instead makes a weaker
adjustment that only causes temporary vars with linkage have a
DECL_CONTEXT to merge from.  This way these unnamed, "unmergeable"
temporaries are properly streamed by value again.

	PR c++/114856

gcc/cp/ChangeLog:

	* call.cc (make_temporary_var_for_ref_to_temp): Set context for
	temporaries with linkage.
	* init.cc (create_temporary_var): Revert to only set context
	when in a function decl.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/pr114856.h: New test.
	* g++.dg/modules/pr114856_a.H: New test.
	* g++.dg/modules/pr114856_b.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/call.cc                            |  1 +
 gcc/cp/init.cc                            |  2 +-
 gcc/testsuite/g++.dg/modules/pr114856.h   | 12 ++++++++++++
 gcc/testsuite/g++.dg/modules/pr114856_a.H |  5 +++++
 gcc/testsuite/g++.dg/modules/pr114856_b.C |  5 +++++
 5 files changed, 24 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/pr114856.h
 create mode 100644 gcc/testsuite/g++.dg/modules/pr114856_a.H
 create mode 100644 gcc/testsuite/g++.dg/modules/pr114856_b.C

On Thu, 2 May 2024, Nathaniel Shead wrote:

> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk/14.2?
> 
> Another alternative would be to stream such !DECL_NAME temporaries with
> a merge key of MK_unique rather than attempting to find the matching
> (nonexistant) field of the class context.

Both approaches sound good to me, hard to say which one is preferable..

The handling of function-scope vs class-scope temporaries seems to start
diverging in:

@@ -8861,28 +8861,6 @@ trees_out::decl_node (tree decl, walk_kind ref)
       return false;
     }
 
!  tree ctx = CP_DECL_CONTEXT (decl);
!  depset *dep = NULL;
!  if (streaming_p ())
!    dep = dep_hash->find_dependency (decl);
!  else if (TREE_CODE (ctx) != FUNCTION_DECL
!	   || TREE_CODE (decl) == TEMPLATE_DECL
!	   || DECL_IMPLICIT_TYPEDEF_P (decl)
!	   || (DECL_LANG_SPECIFIC (decl)
!	       && DECL_MODULE_IMPORT_P (decl)))
!    {
!      auto kind = (TREE_CODE (decl) == NAMESPACE_DECL
!		   && !DECL_NAMESPACE_ALIAS (decl)
!		   ? depset::EK_NAMESPACE : depset::EK_DECL);
!      dep = dep_hash->add_dependency (decl, kind);
!    }
!
!  if (!dep)
!    {
!      /* Some internal entity of context.  Do by value.  */
!      decl_value (decl, NULL);
!      return false;
!    }
 
   if (dep->get_entity_kind () == depset::EK_REDIRECT)
     {

where for a class-scope temporary we add a dependency for it, stream
it by reference, and then stream it by value separately, which seems
unnecessary.

So if we decide to keep the create_temporary_var change, we probably
would want to unify this code path's handling of temporaries (i.e.
don't add_dependency a temporary regardless of its context).

If we decide your partially revert the create_temporary_var change,
your patch LGTM.

> 
> -- >8 --
> 
> In r14-9266-g2823b4d96d9ec4 I gave all temporary vars a DECL_CONTEXT,
> including those at namespace or global scope, so that they could be
> properly merged across importers.  However, not all of these temporary
> vars are actually supposed to be mergeable.
> 
> For instance, in the attached testcase we have an unnamed temporary var
> used in the NSDMI of a class member, which cannot properly merged -- but
> it also doesn't need to be, as it'll be thrown away when the class type
> itself is merged anyway.
> 
> This patch reverts the change made above and instead makes a weaker
> adjustment that only causes temporary vars with linkage have a
> DECL_CONTEXT to merge from.  This way these unnamed, "unmergeable"
> temporaries are properly streamed by value again.
> 
> 	PR c++/114856
> 
> gcc/cp/ChangeLog:
> 
> 	* call.cc (make_temporary_var_for_ref_to_temp): Set context for
> 	temporaries with linkage.
> 	* init.cc (create_temporary_var): Revert to only set context
> 	when in a function decl.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/pr114856.h: New test.
> 	* g++.dg/modules/pr114856_a.H: New test.
> 	* g++.dg/modules/pr114856_b.C: New test.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>  gcc/cp/call.cc                            |  1 +
>  gcc/cp/init.cc                            |  2 +-
>  gcc/testsuite/g++.dg/modules/pr114856.h   | 12 ++++++++++++
>  gcc/testsuite/g++.dg/modules/pr114856_a.H |  5 +++++
>  gcc/testsuite/g++.dg/modules/pr114856_b.C |  5 +++++
>  5 files changed, 24 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/g++.dg/modules/pr114856.h
>  create mode 100644 gcc/testsuite/g++.dg/modules/pr114856_a.H
>  create mode 100644 gcc/testsuite/g++.dg/modules/pr114856_b.C
> 
> diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
> index dbdd7c29fe8..3b8889ac301 100644
> --- a/gcc/cp/call.cc
> +++ b/gcc/cp/call.cc
> @@ -13799,6 +13799,7 @@ make_temporary_var_for_ref_to_temp (tree decl, tree type)
>  
>        tree name = mangle_ref_init_variable (decl);
>        DECL_NAME (var) = name;
> +      DECL_CONTEXT (var) = current_scope ();
>        SET_DECL_ASSEMBLER_NAME (var, name);
>      }
>    else
> diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc
> index a93ce00800c..e758a8c8568 100644
> --- a/gcc/cp/init.cc
> +++ b/gcc/cp/init.cc
> @@ -4287,7 +4287,7 @@ create_temporary_var (tree type)
>    TREE_USED (decl) = 1;
>    DECL_ARTIFICIAL (decl) = 1;
>    DECL_IGNORED_P (decl) = 1;
> -  DECL_CONTEXT (decl) = current_scope ();
> +  DECL_CONTEXT (decl) = current_function_decl;
>  
>    return decl;
>  }
> diff --git a/gcc/testsuite/g++.dg/modules/pr114856.h b/gcc/testsuite/g++.dg/modules/pr114856.h
> new file mode 100644
> index 00000000000..b1a3c2cd834
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/pr114856.h
> @@ -0,0 +1,12 @@
> +// PR c++/114856
> +
> +#include <initializer_list>
> +struct A {
> +  ~A();
> +};
> +struct V {
> +  V(std::initializer_list<A>);
> +};
> +struct data {
> +  V v{{}};
> +};
> diff --git a/gcc/testsuite/g++.dg/modules/pr114856_a.H b/gcc/testsuite/g++.dg/modules/pr114856_a.H
> new file mode 100644
> index 00000000000..6195277dbde
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/pr114856_a.H
> @@ -0,0 +1,5 @@
> +// PR c++/114856
> +// { dg-additional-options "-fmodule-header" }
> +// { dg-module-cmi {} }
> +
> +#include "pr114856.h"
> diff --git a/gcc/testsuite/g++.dg/modules/pr114856_b.C b/gcc/testsuite/g++.dg/modules/pr114856_b.C
> new file mode 100644
> index 00000000000..f81dc8b81d5
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/pr114856_b.C
> @@ -0,0 +1,5 @@
> +// PR c++/114856
> +// { dg-additional-options "-fmodules-ts" }
> +
> +#include "pr114856.h"
> +import "pr114856_a.H";
> -- 
> 2.43.2
> 
>

On 5/2/24 10:40, Patrick Palka wrote:
> On Thu, 2 May 2024, Nathaniel Shead wrote:
> 
>> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk/14.2?
>>
>> Another alternative would be to stream such !DECL_NAME temporaries with
>> a merge key of MK_unique rather than attempting to find the matching
>> (nonexistant) field of the class context.
> 
> Both approaches sound good to me, hard to say which one is preferable..
> 
> The handling of function-scope vs class-scope temporaries seems to start
> diverging in:
> 
> @@ -8861,28 +8861,6 @@ trees_out::decl_node (tree decl, walk_kind ref)
>         return false;
>       }
>   
> !  tree ctx = CP_DECL_CONTEXT (decl);
> !  depset *dep = NULL;
> !  if (streaming_p ())
> !    dep = dep_hash->find_dependency (decl);
> !  else if (TREE_CODE (ctx) != FUNCTION_DECL
> !	   || TREE_CODE (decl) == TEMPLATE_DECL
> !	   || DECL_IMPLICIT_TYPEDEF_P (decl)
> !	   || (DECL_LANG_SPECIFIC (decl)
> !	       && DECL_MODULE_IMPORT_P (decl)))
> !    {
> !      auto kind = (TREE_CODE (decl) == NAMESPACE_DECL
> !		   && !DECL_NAMESPACE_ALIAS (decl)
> !		   ? depset::EK_NAMESPACE : depset::EK_DECL);
> !      dep = dep_hash->add_dependency (decl, kind);
> !    }
> !
> !  if (!dep)
> !    {
> !      /* Some internal entity of context.  Do by value.  */
> !      decl_value (decl, NULL);
> !      return false;
> !    }
>   
>     if (dep->get_entity_kind () == depset::EK_REDIRECT)
>       {
> 
> where for a class-scope temporary we add a dependency for it, stream
> it by reference, and then stream it by value separately, which seems
> unnecessary.
> 
> So if we decide to keep the create_temporary_var change, we probably
> would want to unify this code path's handling of temporaries (i.e.
> don't add_dependency a temporary regardless of its context).
> 
> If we decide your partially revert the create_temporary_var change,
> your patch LGTM.

Streaming by value sounds right, but as noted an important difference 
between reference temps and others is DECL_NAME.  Perhaps the code 
Patrick quotes could look at that as well as the context?

Jason

On Thu, May 02, 2024 at 01:53:44PM -0400, Jason Merrill wrote:
> On 5/2/24 10:40, Patrick Palka wrote:
> > On Thu, 2 May 2024, Nathaniel Shead wrote:
> > 
> > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk/14.2?
> > > 
> > > Another alternative would be to stream such !DECL_NAME temporaries with
> > > a merge key of MK_unique rather than attempting to find the matching
> > > (nonexistant) field of the class context.
> > 
> > Both approaches sound good to me, hard to say which one is preferable..
> > 
> > The handling of function-scope vs class-scope temporaries seems to start
> > diverging in:
> > 
> > @@ -8861,28 +8861,6 @@ trees_out::decl_node (tree decl, walk_kind ref)
> >         return false;
> >       }
> > !  tree ctx = CP_DECL_CONTEXT (decl);
> > !  depset *dep = NULL;
> > !  if (streaming_p ())
> > !    dep = dep_hash->find_dependency (decl);
> > !  else if (TREE_CODE (ctx) != FUNCTION_DECL
> > !	   || TREE_CODE (decl) == TEMPLATE_DECL
> > !	   || DECL_IMPLICIT_TYPEDEF_P (decl)
> > !	   || (DECL_LANG_SPECIFIC (decl)
> > !	       && DECL_MODULE_IMPORT_P (decl)))
> > !    {
> > !      auto kind = (TREE_CODE (decl) == NAMESPACE_DECL
> > !		   && !DECL_NAMESPACE_ALIAS (decl)
> > !		   ? depset::EK_NAMESPACE : depset::EK_DECL);
> > !      dep = dep_hash->add_dependency (decl, kind);
> > !    }
> > !
> > !  if (!dep)
> > !    {
> > !      /* Some internal entity of context.  Do by value.  */
> > !      decl_value (decl, NULL);
> > !      return false;
> > !    }
> >     if (dep->get_entity_kind () == depset::EK_REDIRECT)
> >       {
> > 
> > where for a class-scope temporary we add a dependency for it, stream
> > it by reference, and then stream it by value separately, which seems
> > unnecessary.
> > 
> > So if we decide to keep the create_temporary_var change, we probably
> > would want to unify this code path's handling of temporaries (i.e.
> > don't add_dependency a temporary regardless of its context).
> > 
> > If we decide your partially revert the create_temporary_var change,
> > your patch LGTM.
> 
> Streaming by value sounds right, but as noted an important difference
> between reference temps and others is DECL_NAME.  Perhaps the code Patrick
> quotes could look at that as well as the context?
> 
> Jason
> 

With my patch we would no longer go through the code that Patrick quotes
for class-scope temporaries that I can see; we would instead first hit
the following code in 'tree_node':


  if (DECL_P (t))
    {
      if (DECL_TEMPLATE_PARM_P (t))
	{
	  tpl_parm_value (t);
	  goto done;
	}

      if (!DECL_CONTEXT (t))
	{
	  /* There are a few cases of decls with no context.  We'll write
	     these by value, but first assert they are cases we expect.  */
	  gcc_checking_assert (ref == WK_normal);
	  switch (TREE_CODE (t))
	    {
	    default: gcc_unreachable ();

	    case LABEL_DECL:
	      /* CASE_LABEL_EXPRs contain uncontexted LABEL_DECLs.  */
	      gcc_checking_assert (!DECL_NAME (t));
	      break;

	    case VAR_DECL:
	      /* AGGR_INIT_EXPRs cons up anonymous uncontexted VAR_DECLs.  */
	      gcc_checking_assert (!DECL_NAME (t)
				   && DECL_ARTIFICIAL (t));
	      break;

	    case PARM_DECL:
	      /* REQUIRES_EXPRs have a tree list of uncontexted
		 PARM_DECLS.  It'd be nice if they had a
		 distinguishing flag to double check.  */
	      break;
	    }
	  goto by_value;
	}
    }

 skip_normal:
  if (DECL_P (t) && !decl_node (t, ref))
    goto done;

  /* Otherwise by value */
 by_value:
  tree_value (t);


I think modifying what Patrick pointed out should only be necessary if
we maintain these nameless temporaries as having a class context; for
clarity, is that the direction you'd prefer me to go in to solve this?

Thanks,
Nathaniel

On 5/7/24 01:35, Nathaniel Shead wrote:
> On Thu, May 02, 2024 at 01:53:44PM -0400, Jason Merrill wrote:
>> On 5/2/24 10:40, Patrick Palka wrote:
>>> On Thu, 2 May 2024, Nathaniel Shead wrote:
>>>
>>>> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk/14.2?
>>>>
>>>> Another alternative would be to stream such !DECL_NAME temporaries with
>>>> a merge key of MK_unique rather than attempting to find the matching
>>>> (nonexistant) field of the class context.
>>>
>>> Both approaches sound good to me, hard to say which one is preferable..
>>>
>>> The handling of function-scope vs class-scope temporaries seems to start
>>> diverging in:
>>>
>>> @@ -8861,28 +8861,6 @@ trees_out::decl_node (tree decl, walk_kind ref)
>>>          return false;
>>>        }
>>> !  tree ctx = CP_DECL_CONTEXT (decl);
>>> !  depset *dep = NULL;
>>> !  if (streaming_p ())
>>> !    dep = dep_hash->find_dependency (decl);
>>> !  else if (TREE_CODE (ctx) != FUNCTION_DECL
>>> !	   || TREE_CODE (decl) == TEMPLATE_DECL
>>> !	   || DECL_IMPLICIT_TYPEDEF_P (decl)
>>> !	   || (DECL_LANG_SPECIFIC (decl)
>>> !	       && DECL_MODULE_IMPORT_P (decl)))
>>> !    {
>>> !      auto kind = (TREE_CODE (decl) == NAMESPACE_DECL
>>> !		   && !DECL_NAMESPACE_ALIAS (decl)
>>> !		   ? depset::EK_NAMESPACE : depset::EK_DECL);
>>> !      dep = dep_hash->add_dependency (decl, kind);
>>> !    }
>>> !
>>> !  if (!dep)
>>> !    {
>>> !      /* Some internal entity of context.  Do by value.  */
>>> !      decl_value (decl, NULL);
>>> !      return false;
>>> !    }
>>>      if (dep->get_entity_kind () == depset::EK_REDIRECT)
>>>        {
>>>
>>> where for a class-scope temporary we add a dependency for it, stream
>>> it by reference, and then stream it by value separately, which seems
>>> unnecessary.
>>>
>>> So if we decide to keep the create_temporary_var change, we probably
>>> would want to unify this code path's handling of temporaries (i.e.
>>> don't add_dependency a temporary regardless of its context).
>>>
>>> If we decide your partially revert the create_temporary_var change,
>>> your patch LGTM.
>>
>> Streaming by value sounds right, but as noted an important difference
>> between reference temps and others is DECL_NAME.  Perhaps the code Patrick
>> quotes could look at that as well as the context?
>>
>> Jason
>>
> 
> With my patch we would no longer go through the code that Patrick quotes
> for class-scope temporaries that I can see; we would instead first hit
> the following code in 'tree_node':
> 
> 
>    if (DECL_P (t))
>      {
>        if (DECL_TEMPLATE_PARM_P (t))
> 	{
> 	  tpl_parm_value (t);
> 	  goto done;
> 	}
> 
>        if (!DECL_CONTEXT (t))
> 	{
> 	  /* There are a few cases of decls with no context.  We'll write
> 	     these by value, but first assert they are cases we expect.  */
> 	  gcc_checking_assert (ref == WK_normal);
> 	  switch (TREE_CODE (t))
> 	    {
> 	    default: gcc_unreachable ();
> 
> 	    case LABEL_DECL:
> 	      /* CASE_LABEL_EXPRs contain uncontexted LABEL_DECLs.  */
> 	      gcc_checking_assert (!DECL_NAME (t));
> 	      break;
> 
> 	    case VAR_DECL:
> 	      /* AGGR_INIT_EXPRs cons up anonymous uncontexted VAR_DECLs.  */
> 	      gcc_checking_assert (!DECL_NAME (t)
> 				   && DECL_ARTIFICIAL (t));
> 	      break;
> 
> 	    case PARM_DECL:
> 	      /* REQUIRES_EXPRs have a tree list of uncontexted
> 		 PARM_DECLS.  It'd be nice if they had a
> 		 distinguishing flag to double check.  */
> 	      break;
> 	    }
> 	  goto by_value;
> 	}
>      }
> 
>   skip_normal:
>    if (DECL_P (t) && !decl_node (t, ref))
>      goto done;
> 
>    /* Otherwise by value */
>   by_value:
>    tree_value (t);
> 
> 
> I think modifying what Patrick pointed out should only be necessary if
> we maintain these nameless temporaries as having a class context; for
> clarity, is that the direction you'd prefer me to go in to solve this?

I was thinking in this code that it seems fragile to require null 
DECL_CONTEXT to identify something produced by 
create_temporary_var/build_local_temp (which should really be merged).
We could even use is_local_temp instead of checking particular qualities 
directly.

But your patch is OK; please just add a comment to the 
make_temporary_var_for_ref_to_temp change indicating that you're setting 
DECL_CONTEXT to make the variable mergeable.

Thanks,
Jason