diff mbox series

c++/modules: Avoid rechecking initializers when streaming NTTPs [PR116382]

Message ID 66bef7e7.170a0220.bd16d.84ab@mx.google.com
State New
Headers show
Series c++/modules: Avoid rechecking initializers when streaming NTTPs [PR116382] | expand

Commit Message

Nathaniel Shead Aug. 16, 2024, 6:55 a.m. UTC 
Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?

-- >8 --

When reading an NTTP we call get_template_parm_object which delegates
setting of DECL_INITIAL to the general cp_finish_decl procedure, which
calls check_initializer to validate and record it.

Apart from being unnecessary (it must have already been validated by the
writing module), this also causes errors in cases like the linked PR, as
validating may end up needing to call lazy_load_pendings to determine
any specialisations that may exist which violates assumptions of the
modules streaming code.

This patch works around the issue by adding a flag to
get_template_parm_object to disable these checks when not needed.

	PR c++/116382

gcc/cp/ChangeLog:

	* cp-tree.h (get_template_parm_object): Add check_init param.
	* module.cc (trees_in::tree_node): Pass check_init=false when
	building NTTPs.
	* pt.cc (get_template_parm_object): Prevent cp_finish_decl from
	validating the initializer when check_init=false.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/tpl-nttp-1_a.C: New test.
	* g++.dg/modules/tpl-nttp-1_b.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/cp-tree.h                            |  3 ++-
 gcc/cp/module.cc                            |  6 +++++-
 gcc/cp/pt.cc                                | 13 +++++++++++--
 gcc/testsuite/g++.dg/modules/tpl-nttp-1_a.C |  8 ++++++++
 gcc/testsuite/g++.dg/modules/tpl-nttp-1_b.C |  6 ++++++
 5 files changed, 32 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/tpl-nttp-1_a.C
 create mode 100644 gcc/testsuite/g++.dg/modules/tpl-nttp-1_b.C

Comments

Jason Merrill Aug. 19, 2024, 5:38 p.m. UTC | #1 
On 8/16/24 2:55 AM, Nathaniel Shead wrote:
> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?

Maybe also skip the unshare_constructor when !check_init?  OK either way.

> -- >8 --
> 
> When reading an NTTP we call get_template_parm_object which delegates
> setting of DECL_INITIAL to the general cp_finish_decl procedure, which
> calls check_initializer to validate and record it.
> 
> Apart from being unnecessary (it must have already been validated by the
> writing module), this also causes errors in cases like the linked PR, as
> validating may end up needing to call lazy_load_pendings to determine
> any specialisations that may exist which violates assumptions of the
> modules streaming code.
> 
> This patch works around the issue by adding a flag to
> get_template_parm_object to disable these checks when not needed.
> 
> 	PR c++/116382
> 
> gcc/cp/ChangeLog:
> 
> 	* cp-tree.h (get_template_parm_object): Add check_init param.
> 	* module.cc (trees_in::tree_node): Pass check_init=false when
> 	building NTTPs.
> 	* pt.cc (get_template_parm_object): Prevent cp_finish_decl from
> 	validating the initializer when check_init=false.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/tpl-nttp-1_a.C: New test.
> 	* g++.dg/modules/tpl-nttp-1_b.C: New test.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>   gcc/cp/cp-tree.h                            |  3 ++-
>   gcc/cp/module.cc                            |  6 +++++-
>   gcc/cp/pt.cc                                | 13 +++++++++++--
>   gcc/testsuite/g++.dg/modules/tpl-nttp-1_a.C |  8 ++++++++
>   gcc/testsuite/g++.dg/modules/tpl-nttp-1_b.C |  6 ++++++
>   5 files changed, 32 insertions(+), 4 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/modules/tpl-nttp-1_a.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/tpl-nttp-1_b.C
> 
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index 039c70710a2..a9ce44bb214 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -7626,7 +7626,8 @@ enum { nt_opaque = false, nt_transparent = true };
>   extern tree alias_template_specialization_p     (const_tree, bool);
>   extern tree dependent_alias_template_spec_p     (const_tree, bool);
>   extern bool dependent_opaque_alias_p            (const_tree);
> -extern tree get_template_parm_object		(tree expr, tree mangle);
> +extern tree get_template_parm_object		(tree expr, tree mangle,
> +						 bool check_init = true);
>   extern tree tparm_object_argument		(tree);
>   extern bool explicit_class_specialization_p     (tree);
>   extern bool push_tinst_level                    (tree);
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index c3218bd5caf..0a4ceffa3d6 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -9938,7 +9938,11 @@ trees_in::tree_node (bool is_use)
>   	tree name = tree_node ();
>   	if (!get_overrun ())
>   	  {
> -	    res = get_template_parm_object (init, name);
> +	    /* We don't want to check the initializer as that may require
> +	       name lookup, which could recursively start lazy loading.
> +	       Instead we know that INIT is already valid so we can just
> +	       apply that directly.  */
> +	    res = get_template_parm_object (init, name, /*check_init=*/false);
>   	    int tag = insert (res);
>   	    dump (dumper::TREE)
>   	      && dump ("Created nttp object:%d %N", tag, name);
> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index 1c531f456be..a5d52906389 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -7361,10 +7361,11 @@ create_template_parm_object (tree expr, tsubst_flags_t complain)
>   static GTY(()) hash_map<tree, tree> *tparm_obj_values;
>   
>   /* Find or build an nttp object for (already-validated) EXPR with name
> -   NAME.  */
> +   NAME.  When CHECK_INIT is false we don't need to process the initialiser,
> +   it's already been done.  */
>   
>   tree
> -get_template_parm_object (tree expr, tree name)
> +get_template_parm_object (tree expr, tree name, bool check_init/*=true*/)
>   {
>     tree decl = get_global_binding (name);
>     if (decl)
> @@ -7390,6 +7391,14 @@ get_template_parm_object (tree expr, tree name)
>         hash_map_safe_put<hm_ggc> (tparm_obj_values, decl, copy);
>       }
>   
> +  if (!check_init)
> +    {
> +      /* The expr is the already processed initializer, set it on the NTTP
> +	 object now so that cp_finish_decl doesn't do it again later.  */
> +      DECL_INITIAL (decl) = expr;
> +      DECL_INITIALIZED_P (decl) = 1;
> +    }
> +
>     pushdecl_top_level_and_finish (decl, expr);
>   
>     return decl;
> diff --git a/gcc/testsuite/g++.dg/modules/tpl-nttp-1_a.C b/gcc/testsuite/g++.dg/modules/tpl-nttp-1_a.C
> new file mode 100644
> index 00000000000..46f6ecd0c1d
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/tpl-nttp-1_a.C
> @@ -0,0 +1,8 @@
> +// PR c++/116382
> +// { dg-additional-options "-fmodules-ts -std=c++20" }
> +// { dg-module-cmi m:a }
> +
> +module m:a;
> +template <typename> struct X {};
> +template <X<int> nttp> struct index {};
> +template struct index<{}>;
> diff --git a/gcc/testsuite/g++.dg/modules/tpl-nttp-1_b.C b/gcc/testsuite/g++.dg/modules/tpl-nttp-1_b.C
> new file mode 100644
> index 00000000000..a3241e7bc25
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/tpl-nttp-1_b.C
> @@ -0,0 +1,6 @@
> +// PR c++/116382
> +// { dg-additional-options "-fmodules-ts -std=c++20" }
> +// { dg-module-cmi m }
> +
> +export module m;
> +import :a;
Nathaniel Shead Aug. 20, 2024, 7:07 a.m. UTC | #2 
On Mon, Aug 19, 2024 at 01:38:56PM -0400, Jason Merrill wrote:
> On 8/16/24 2:55 AM, Nathaniel Shead wrote:
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> 
> Maybe also skip the unshare_constructor when !check_init?  OK either way.
> 

Makes sense, thanks. This is what I'll push once regtest completes.

-- >8 --

When reading an NTTP we call get_template_parm_object which delegates
setting of DECL_INITIAL to the general cp_finish_decl procedure, which
calls check_initializer to validate and record it.

Apart from being unnecessary (it must have already been validated by the
writing module), this also causes errors in cases like the linked PR, as
validating may end up needing to call lazy_load_pendings to determine
any specialisations that may exist which violates assumptions of the
modules streaming code.

This patch works around the issue by adding a flag to
get_template_parm_object to disable these checks when not needed.

        PR c++/116382

gcc/cp/ChangeLog:

        * cp-tree.h (get_template_parm_object): Add check_init param.
        * module.cc (trees_in::tree_node): Pass check_init=false when
        building NTTPs.
        * pt.cc (get_template_parm_object): Prevent cp_finish_decl from
        validating the initializer when check_init=false.

gcc/testsuite/ChangeLog:

        * g++.dg/modules/tpl-nttp-1_a.C: New test.
        * g++.dg/modules/tpl-nttp-1_b.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
Reviewed-by: Jason Merrill <jason@redhat.com>

diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 039c70710a2..a9ce44bb214 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -7626,7 +7626,8 @@ enum { nt_opaque = false, nt_transparent = true };
 extern tree alias_template_specialization_p     (const_tree, bool);
 extern tree dependent_alias_template_spec_p     (const_tree, bool);
 extern bool dependent_opaque_alias_p            (const_tree);
-extern tree get_template_parm_object		(tree expr, tree mangle);
+extern tree get_template_parm_object		(tree expr, tree mangle,
+						 bool check_init = true);
 extern tree tparm_object_argument		(tree);
 extern bool explicit_class_specialization_p     (tree);
 extern bool push_tinst_level                    (tree);
diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index c3218bd5caf..0a4ceffa3d6 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -9938,7 +9938,11 @@ trees_in::tree_node (bool is_use)
 	tree name = tree_node ();
 	if (!get_overrun ())
 	  {
-	    res = get_template_parm_object (init, name);
+	    /* We don't want to check the initializer as that may require
+	       name lookup, which could recursively start lazy loading.
+	       Instead we know that INIT is already valid so we can just
+	       apply that directly.  */
+	    res = get_template_parm_object (init, name, /*check_init=*/false);
 	    int tag = insert (res);
 	    dump (dumper::TREE)
 	      && dump ("Created nttp object:%d %N", tag, name);
diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index 32d164f0fd5..76edc7aad50 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -7361,10 +7361,11 @@ create_template_parm_object (tree expr, tsubst_flags_t complain)
 static GTY(()) hash_map<tree, tree> *tparm_obj_values;
 
 /* Find or build an nttp object for (already-validated) EXPR with name
-   NAME.  */
+   NAME.  When CHECK_INIT is false we don't need to process the initialiser,
+   it's already been done.  */
 
 tree
-get_template_parm_object (tree expr, tree name)
+get_template_parm_object (tree expr, tree name, bool check_init/*=true*/)
 {
   tree decl = get_global_binding (name);
   if (decl)
@@ -7385,11 +7386,20 @@ get_template_parm_object (tree expr, tree name)
     {
       /* If EXPR contains any PTRMEM_CST, they will get clobbered by
 	 lower_var_init before we're done mangling.  So store the original
-	 value elsewhere.  */
-      tree copy = unshare_constructor (expr);
+	 value elsewhere.  We only need to unshare EXPR if it's not yet
+	 been processed.  */
+      tree copy = check_init ? unshare_constructor (expr) : expr;
       hash_map_safe_put<hm_ggc> (tparm_obj_values, decl, copy);
     }
 
+  if (!check_init)
+    {
+      /* The EXPR is the already processed initializer, set it on the NTTP
+	 object now so that cp_finish_decl doesn't do it again later.  */
+      DECL_INITIAL (decl) = expr;
+      DECL_INITIALIZED_P (decl) = 1;
+    }
+
   pushdecl_top_level_and_finish (decl, expr);
 
   return decl;
diff --git a/gcc/testsuite/g++.dg/modules/tpl-nttp-1_a.C b/gcc/testsuite/g++.dg/modules/tpl-nttp-1_a.C
new file mode 100644
index 00000000000..46f6ecd0c1d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/tpl-nttp-1_a.C
@@ -0,0 +1,8 @@
+// PR c++/116382
+// { dg-additional-options "-fmodules-ts -std=c++20" }
+// { dg-module-cmi m:a }
+
+module m:a;
+template <typename> struct X {};
+template <X<int> nttp> struct index {};
+template struct index<{}>;
diff --git a/gcc/testsuite/g++.dg/modules/tpl-nttp-1_b.C b/gcc/testsuite/g++.dg/modules/tpl-nttp-1_b.C
new file mode 100644
index 00000000000..a3241e7bc25
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/tpl-nttp-1_b.C
@@ -0,0 +1,6 @@
+// PR c++/116382
+// { dg-additional-options "-fmodules-ts -std=c++20" }
+// { dg-module-cmi m }
+
+export module m;
+import :a;
diff mbox series

Patch

diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 039c70710a2..a9ce44bb214 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -7626,7 +7626,8 @@  enum { nt_opaque = false, nt_transparent = true };
 extern tree alias_template_specialization_p     (const_tree, bool);
 extern tree dependent_alias_template_spec_p     (const_tree, bool);
 extern bool dependent_opaque_alias_p            (const_tree);
-extern tree get_template_parm_object		(tree expr, tree mangle);
+extern tree get_template_parm_object		(tree expr, tree mangle,
+						 bool check_init = true);
 extern tree tparm_object_argument		(tree);
 extern bool explicit_class_specialization_p     (tree);
 extern bool push_tinst_level                    (tree);
diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index c3218bd5caf..0a4ceffa3d6 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -9938,7 +9938,11 @@  trees_in::tree_node (bool is_use)
 	tree name = tree_node ();
 	if (!get_overrun ())
 	  {
-	    res = get_template_parm_object (init, name);
+	    /* We don't want to check the initializer as that may require
+	       name lookup, which could recursively start lazy loading.
+	       Instead we know that INIT is already valid so we can just
+	       apply that directly.  */
+	    res = get_template_parm_object (init, name, /*check_init=*/false);
 	    int tag = insert (res);
 	    dump (dumper::TREE)
 	      && dump ("Created nttp object:%d %N", tag, name);
diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index 1c531f456be..a5d52906389 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -7361,10 +7361,11 @@  create_template_parm_object (tree expr, tsubst_flags_t complain)
 static GTY(()) hash_map<tree, tree> *tparm_obj_values;
 
 /* Find or build an nttp object for (already-validated) EXPR with name
-   NAME.  */
+   NAME.  When CHECK_INIT is false we don't need to process the initialiser,
+   it's already been done.  */
 
 tree
-get_template_parm_object (tree expr, tree name)
+get_template_parm_object (tree expr, tree name, bool check_init/*=true*/)
 {
   tree decl = get_global_binding (name);
   if (decl)
@@ -7390,6 +7391,14 @@  get_template_parm_object (tree expr, tree name)
       hash_map_safe_put<hm_ggc> (tparm_obj_values, decl, copy);
     }
 
+  if (!check_init)
+    {
+      /* The expr is the already processed initializer, set it on the NTTP
+	 object now so that cp_finish_decl doesn't do it again later.  */
+      DECL_INITIAL (decl) = expr;
+      DECL_INITIALIZED_P (decl) = 1;
+    }
+
   pushdecl_top_level_and_finish (decl, expr);
 
   return decl;
diff --git a/gcc/testsuite/g++.dg/modules/tpl-nttp-1_a.C b/gcc/testsuite/g++.dg/modules/tpl-nttp-1_a.C
new file mode 100644
index 00000000000..46f6ecd0c1d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/tpl-nttp-1_a.C
@@ -0,0 +1,8 @@ 
+// PR c++/116382
+// { dg-additional-options "-fmodules-ts -std=c++20" }
+// { dg-module-cmi m:a }
+
+module m:a;
+template <typename> struct X {};
+template <X<int> nttp> struct index {};
+template struct index<{}>;
diff --git a/gcc/testsuite/g++.dg/modules/tpl-nttp-1_b.C b/gcc/testsuite/g++.dg/modules/tpl-nttp-1_b.C
new file mode 100644
index 00000000000..a3241e7bc25
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/tpl-nttp-1_b.C
@@ -0,0 +1,6 @@ 
+// PR c++/116382
+// { dg-additional-options "-fmodules-ts -std=c++20" }
+// { dg-module-cmi m }
+
+export module m;
+import :a;