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

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

[Patrick Palka]

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.

	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()));
-- 
2.38.0.68.ge85701b4af

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

[Nathan Sidwell]

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