Re: [PATCH] c++ modules: streaming constexpr_fundef [PR101449]

[Nathan Sidwell <nathan@acm.org>]

On 10/14/22 13:00, Patrick Palka wrote:
> IIUC we currently avoid streaming the RESULT_DECL and PARM_DECLs
> of a constexpr_fundef entry under the assumption that they're just
> copies of the DECL_RESULT and DECL_ARGUMENTS of the FUNCTION_DECL.
> Thus we can just make new copies of DECL_RESULT and DECL_ARGUMENTs
> on stream in rather than separately streaming them.
> 
> Unfortunately this assumption isn't true generally: the FUNCTION_DECL
> contains genericized trees, whereas the constexpr_fundef entry contains
> pre-GENERIC trees.  So in particular DECL_RESULT and DECL_ARGUMENTs
> may have been turned into invisiref parms which we don't handle during
> during constexpr evaluation and so we ICE in the below testcase.
> 
> This patch fixes this by faithfully streaming the RESULT_DECL and
> PARM_DECLs of a constexpr_fundef entry.

Hm, the reason for the complexity was that I wanted to recreate the tree graph 
where the fndecl came from one TU and the defn came from another one -- we need 
the definition to refer to argument decls from the already-read decl.  However, 
it seems that for constexpr fns here, that is not needed, resulting in a 
significant simplification.

ok.

nathan

> 
> 	PR c++/101449
> 
> gcc/cp/ChangeLog:
> 
> 	* module.cc (trees_out::write_function_def): Stream the
> 	parms and result of the constexpr_fundef entry.
> 	(trees_in::read_function_def): Likewise.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/cexpr-3_a.C: New test.
> 	* g++.dg/modules/cexpr-3_b.C: New test.
> ---
>   gcc/cp/module.cc                         | 59 ++++--------------------
>   gcc/testsuite/g++.dg/modules/cexpr-3_a.C | 14 ++++++
>   gcc/testsuite/g++.dg/modules/cexpr-3_b.C |  7 +++
>   3 files changed, 29 insertions(+), 51 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/modules/cexpr-3_a.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/cexpr-3_b.C
> 
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index 7ffeefa7c1f..999ff3faafc 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -11553,34 +11553,13 @@ trees_out::write_function_def (tree decl)
>     tree_node (DECL_FRIEND_CONTEXT (decl));
>   
>     constexpr_fundef *cexpr = retrieve_constexpr_fundef (decl);
> -  int tag = 0;
> -  if (cexpr)
> -    {
> -      if (cexpr->result == error_mark_node)
> -	/* We'll stream the RESULT_DECL naturally during the
> -	   serialization.  We never need to fish it back again, so
> -	   that's ok.  */
> -	tag = 0;
> -      else
> -	tag = insert (cexpr->result);
> -    }
> +
>     if (streaming_p ())
> +    u (cexpr != nullptr);
> +  if (cexpr)
>       {
> -      i (tag);
> -      if (tag)
> -	dump (dumper::TREE)
> -	  && dump ("Constexpr:%d result %N", tag, cexpr->result);
> -    }
> -  if (tag)
> -    {
> -      unsigned ix = 0;
> -      for (tree parm = cexpr->parms; parm; parm = DECL_CHAIN (parm), ix++)
> -	{
> -	  tag = insert (parm);
> -	  if (streaming_p ())
> -	    dump (dumper::TREE)
> -	      && dump ("Constexpr:%d parm:%u %N", tag, ix, parm);
> -	}
> +      chained_decls (cexpr->parms);
> +      tree_node (cexpr->result);
>         tree_node (cexpr->body);
>       }
>   
> @@ -11613,32 +11592,10 @@ trees_in::read_function_def (tree decl, tree maybe_template)
>     tree maybe_dup = odr_duplicate (maybe_template, DECL_SAVED_TREE (decl));
>     bool installing = maybe_dup && !DECL_SAVED_TREE (decl);
>   
> -  if (int wtag = i ())
> +  if (u ())
>       {
> -      int tag = 1;
> -      cexpr.result = error_mark_node;
> -
> -      cexpr.result = copy_decl (result);
> -      tag = insert (cexpr.result);
> -
> -      if (wtag != tag)
> -	set_overrun ();
> -      dump (dumper::TREE)
> -	&& dump ("Constexpr:%d result %N", tag, cexpr.result);
> -
> -      cexpr.parms = NULL_TREE;
> -      tree *chain = &cexpr.parms;
> -      unsigned ix = 0;
> -      for (tree parm = DECL_ARGUMENTS (maybe_dup ? maybe_dup : decl);
> -	   parm; parm = DECL_CHAIN (parm), ix++)
> -	{
> -	  tree p = copy_decl (parm);
> -	  tag = insert (p);
> -	  dump (dumper::TREE)
> -	    && dump ("Constexpr:%d parm:%u %N", tag, ix, p);
> -	  *chain = p;
> -	  chain = &DECL_CHAIN (p);
> -	}
> +      cexpr.parms = chained_decls ();
> +      cexpr.result = tree_node ();
>         cexpr.body = tree_node ();
>         cexpr.decl = decl;
>       }
> diff --git a/gcc/testsuite/g++.dg/modules/cexpr-3_a.C b/gcc/testsuite/g++.dg/modules/cexpr-3_a.C
> new file mode 100644
> index 00000000000..be24bb43a7b
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/cexpr-3_a.C
> @@ -0,0 +1,14 @@
> +// PR c++/101449
> +// { dg-additional-options -fmodules-ts }
> +// { dg-module-cmi pr101449 }
> +
> +export module pr101449;
> +
> +struct X {
> +  bool b = true;
> +  constexpr X() { }
> +  constexpr X(const X&) { }
> +};
> +
> +export constexpr X f() { return {}; }
> +export constexpr bool g(X x) { return x.b; }
> diff --git a/gcc/testsuite/g++.dg/modules/cexpr-3_b.C b/gcc/testsuite/g++.dg/modules/cexpr-3_b.C
> new file mode 100644
> index 00000000000..cbf3be4fcab
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/cexpr-3_b.C
> @@ -0,0 +1,7 @@
> +// PR c++/101449
> +// { dg-additional-options -fmodules-ts }
> +
> +import pr101449;
> +
> +static_assert(f().b);
> +static_assert(g(f()));

-- 
Nathan Sidwell