Re: [PATCH] c++ modules: ICE with class NTTP argument [PR100616]

[Patrick Palka <ppalka@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 12637 bytes --]

On Mon, 26 Sep 2022, Patrick Palka wrote:

> On Mon, 26 Sep 2022, Nathan Sidwell wrote:
> 
> > On 9/26/22 10:08, Nathan Sidwell wrote:
> > > On 9/23/22 09:32, Patrick Palka wrote:
> > > 
> > > > Judging by the two commits that introduced/modified this part of
> > > > maybe_register_incomplete_var, r196852 and r214333, ISTM the code
> > > > is really only concerned with constexpr static data members (whose
> > > > initializer may contain a pointer-to-member for a currently open class).
> > > > So maybe we ought to restrict the branch like so, which effectively
> > > > disables this part of maybe_register_incomplete_var during stream-in, and
> > > > guarantees that outermost_open_class doesn't return NULL if the branch is
> > > > taken?
> > > 
> > > I think the problem is that we're streaming these VAR_DECLs as regular
> > > VAR_DECLS, when we should be handling them as a new kind of object fished
> > > out from the template they're instantiating. (I'm guessing that'll just be a
> > > new tag, a type and an initializer?)
> > > 
> > > Then on stream-in we can handle them in the same way as a non-modules
> > > compilation handles such redeclarations.  I.e. how does:
> > > 
> > > template<auto> struct C { };
> > > struct A { };
> > > C<A{}> c1; // #1
> > > C<A{}> c2; // #2
> > > 
> > > work.  Presumably at some point #2's A{} gets unified such that we find the
> > > instantation that occurred at #1?
> 
> This works because the lookup in get_template_parm_object for #2's A{}
> finds and reuses the VAR_DECL created for #1's A{}.
> 
> But IIUC this lookup (performed via get_global_binding) isn't
> import-aware, which I suppose explains why we don't find the VAR_DECL
> from another TU.
> 
> > > 
> > > I notice the template arg for C<A{}> is a var decl mangled as _ZTAXtl1AEE,
> > > which is a 'template paramete object for A{}'.  I see that's a special
> > > mangler 'mangle_template_parm_object', called from
> > > get_template_parm_object.  Perhaps these VAR_DECLs need an additional
> > > in-tree flag that the streamer can check for?
> > 
> > I wonder if we're setting the module attachment for these variables sanely?
> > They should be attached to the global module.  My guess is the
> > pushdecl_top_level_and_finish call in get_templatE_parm_object is not doing
> > what is needed (as well as the other issues).
> 
> This is a bit of a shot in the dark, but the following seems to work:
> when pushing the VAR_DECL, we need to call set_originating_module to
> attach it to the global module, and when looking it up, we need to do so
> in an import-aware way.  Hopefully something like this is sufficient
> to properly handle these VAR_DECLs and we don't need to stream them
> specially?

Err, rather than changing the behavior of get_namespace_binding (which
has many unrelated callers), I guess we could just use the already
import-aware lookup_qualified_name instead where appropriate.  WDYT of
the following? (testing in progress)

-- >8 --

Subject: [PATCH] c++ modules: ICE with class NTTP argument [PR100616]

	PR c++/100616
	PR c++/102576

gcc/cp/ChangeLog:

	* pt.cc (get_template_parm_object): Use lookup_qualified_name
	instead of get_global_binding.  Call set_originating_module before
	pushing the VAR_DECL.
	(listify): Use lookup_qualified_name instead of get_global_binding.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/pr100616_a.C: New test.
	* g++.dg/modules/pr100616_b.C: New test.
	* g++.dg/modules/pr102576_a.H: New test.
	* g++.dg/modules/pr102576_b.C: New test.
---
 gcc/cp/pt.cc                              | 10 ++++++----
 gcc/testsuite/g++.dg/modules/pr100616_a.C |  8 ++++++++
 gcc/testsuite/g++.dg/modules/pr100616_b.C |  8 ++++++++
 gcc/testsuite/g++.dg/modules/pr102576_a.H |  5 +++++
 gcc/testsuite/g++.dg/modules/pr102576_b.C |  9 +++++++++
 5 files changed, 36 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/pr100616_a.C
 create mode 100644 gcc/testsuite/g++.dg/modules/pr100616_b.C
 create mode 100644 gcc/testsuite/g++.dg/modules/pr102576_a.H
 create mode 100644 gcc/testsuite/g++.dg/modules/pr102576_b.C

diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index 1f088fe281e..e030d9db2f6 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -7284,8 +7284,8 @@ get_template_parm_object (tree expr, tsubst_flags_t complain)
   gcc_assert (!TREE_HAS_CONSTRUCTOR (expr));
 
   tree name = mangle_template_parm_object (expr);
-  tree decl = get_global_binding (name);
-  if (decl)
+  tree decl = lookup_qualified_name (global_namespace, name);
+  if (decl != error_mark_node)
     return decl;
 
   tree type = cp_build_qualified_type (TREE_TYPE (expr), TYPE_QUAL_CONST);
@@ -7307,6 +7307,7 @@ get_template_parm_object (tree expr, tsubst_flags_t complain)
       hash_map_safe_put<hm_ggc> (tparm_obj_values, decl, copy);
     }
 
+  set_originating_module (decl);
   pushdecl_top_level_and_finish (decl, expr);
 
   return decl;
@@ -29150,9 +29151,10 @@ finish_concept_definition (cp_expr id, tree init)
 static tree
 listify (tree arg)
 {
-  tree std_init_list = get_namespace_binding (std_node, init_list_identifier);
+  tree std_init_list = lookup_qualified_name (std_node, init_list_identifier);
 
-  if (!std_init_list || !DECL_CLASS_TEMPLATE_P (std_init_list))
+  if (std_init_list == error_mark_node
+      || !DECL_CLASS_TEMPLATE_P (std_init_list))
     {
       gcc_rich_location richloc (input_location);
       maybe_add_include_fixit (&richloc, "<initializer_list>", false);
diff --git a/gcc/testsuite/g++.dg/modules/pr100616_a.C b/gcc/testsuite/g++.dg/modules/pr100616_a.C
new file mode 100644
index 00000000000..788af2eb533
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/pr100616_a.C
@@ -0,0 +1,8 @@
+// PR c++/100616
+// { dg-additional-options "-std=c++20 -fmodules-ts" }
+// { dg-module-cmi pr100616 }
+export module pr100616;
+
+template<auto> struct C { };
+struct A { };
+C<A{}> c1;
diff --git a/gcc/testsuite/g++.dg/modules/pr100616_b.C b/gcc/testsuite/g++.dg/modules/pr100616_b.C
new file mode 100644
index 00000000000..fc89cd08ac5
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/pr100616_b.C
@@ -0,0 +1,8 @@
+// PR c++/100616
+// { dg-additional-options "-std=c++20 -fmodules-ts" }
+module pr100616;
+
+C<A{}> c2;
+
+using type = decltype(c1);
+using type = decltype(c2);
diff --git a/gcc/testsuite/g++.dg/modules/pr102576_a.H b/gcc/testsuite/g++.dg/modules/pr102576_a.H
new file mode 100644
index 00000000000..87ba9b52031
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/pr102576_a.H
@@ -0,0 +1,5 @@
+// PR c++/102576
+// { dg-additional-options -fmodule-header }
+// { dg-module-cmi {} }
+
+#include <initializer_list>
diff --git a/gcc/testsuite/g++.dg/modules/pr102576_b.C b/gcc/testsuite/g++.dg/modules/pr102576_b.C
new file mode 100644
index 00000000000..10251ed5304
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/pr102576_b.C
@@ -0,0 +1,9 @@
+// PR c++/102576
+// { dg-additional-options -fmodules-ts }
+
+import "pr102576_a.H";
+
+int main() {
+  for (int i : {1, 2, 3})
+    ;
+}
-- 
2.38.0.rc1.2.g4b79ee4b0c



> 
> -- >8 --
> 
> Subject: [PATCH] c++ modules: ICE with class NTTP argument [PR100616]
> 
> The function get_template_parm_object returns an artifical mangled
> VAR_DECL that's unique to the given class NTTP argument.  To enforce
> this uniqueness, we first look up the mangled name of the VAR_DECL from
> the global scope via get_global_binding, and only create/push a VAR_DECL
> if this lookup fails.
> 
> But with modules, we need to do more to enforce uniqueness: the VAR_DECL
> needs to be attached to the global module, and the lookup needs to be
> import-aware, which get_global_binding currently isn't.
> 
> So this patch makes us call set_originating_module from
> get_template_parm_object before pushing, and makes get_namespace_binding
> use name_lookup::search_qualified which does look into imports.
> 
> It turns out this change to get_namespace_binding also fixes PR102576
> where we were failing to find an imported std::initializer_list due to
> the function not being import-aware.
> 
> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> trunk?
> 
> 	PR c++/100616
> 	PR c++/102576
> 
> gcc/cp/ChangeLog:
> 
> 	* name-lookup.cc (get_namespace_binding): Rewrite in terms of
> 	name_lookup::search_unqualified.
> 	* pt.cc (get_template_parm_object): Call set_originating_module
> 	before pushing the VAR_DECL.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/pr100616_a.C: New test.
> 	* g++.dg/modules/pr100616_b.C: New test.
> 	* g++.dg/modules/pr102576_a.H: New test.
> 	* g++.dg/modules/pr102576_b.C: New test.
> ---
>  gcc/cp/name-lookup.cc                     | 21 ++++++---------------
>  gcc/cp/pt.cc                              |  1 +
>  gcc/testsuite/g++.dg/modules/pr100616_a.C |  8 ++++++++
>  gcc/testsuite/g++.dg/modules/pr100616_b.C |  8 ++++++++
>  gcc/testsuite/g++.dg/modules/pr102576_a.H |  5 +++++
>  gcc/testsuite/g++.dg/modules/pr102576_b.C |  9 +++++++++
>  6 files changed, 37 insertions(+), 15 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/modules/pr100616_a.C
>  create mode 100644 gcc/testsuite/g++.dg/modules/pr100616_b.C
>  create mode 100644 gcc/testsuite/g++.dg/modules/pr102576_a.H
>  create mode 100644 gcc/testsuite/g++.dg/modules/pr102576_b.C
> 
> diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
> index 69d555ddf1f..6cd73276cf5 100644
> --- a/gcc/cp/name-lookup.cc
> +++ b/gcc/cp/name-lookup.cc
> @@ -5772,9 +5772,7 @@ do_class_using_decl (tree scope, tree name)
>  
>  \f
>  /* Return the binding for NAME in NS in the current TU.  If NS is
> -   NULL, look in global_namespace.  We will not find declarations
> -   from imports.  Users of this who, having found nothing, push a new
> -   decl must be prepared for that pushing to match an existing decl.  */
> +   NULL, look in global_namespace.  .*/
>  
>  tree
>  get_namespace_binding (tree ns, tree name)
> @@ -5783,19 +5781,12 @@ get_namespace_binding (tree ns, tree name)
>    if (!ns)
>      ns = global_namespace;
>    gcc_checking_assert (!DECL_NAMESPACE_ALIAS (ns));
> -  tree ret = NULL_TREE;
>  
> -  if (tree *b = find_namespace_slot (ns, name))
> -    {
> -      ret = *b;
> -
> -      if (TREE_CODE (ret) == BINDING_VECTOR)
> -	ret = BINDING_VECTOR_CLUSTER (ret, 0).slots[0];
> -      if (ret)
> -	ret = MAYBE_STAT_DECL (ret);
> -    }
> -
> -  return ret;
> +  name_lookup lookup (name);
> +  if (lookup.search_qualified (ns))
> +    return lookup.value;
> +  else
> +    return NULL_TREE;
>  }
>  
>  /* Push internal DECL into the global namespace.  Does not do the
> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index 1f088fe281e..95cfc5b70a3 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -7307,6 +7307,7 @@ get_template_parm_object (tree expr, tsubst_flags_t complain)
>        hash_map_safe_put<hm_ggc> (tparm_obj_values, decl, copy);
>      }
>  
> +  set_originating_module (decl);
>    pushdecl_top_level_and_finish (decl, expr);
>  
>    return decl;
> diff --git a/gcc/testsuite/g++.dg/modules/pr100616_a.C b/gcc/testsuite/g++.dg/modules/pr100616_a.C
> new file mode 100644
> index 00000000000..788af2eb533
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/pr100616_a.C
> @@ -0,0 +1,8 @@
> +// PR c++/100616
> +// { dg-additional-options "-std=c++20 -fmodules-ts" }
> +// { dg-module-cmi pr100616 }
> +export module pr100616;
> +
> +template<auto> struct C { };
> +struct A { };
> +C<A{}> c1;
> diff --git a/gcc/testsuite/g++.dg/modules/pr100616_b.C b/gcc/testsuite/g++.dg/modules/pr100616_b.C
> new file mode 100644
> index 00000000000..fc89cd08ac5
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/pr100616_b.C
> @@ -0,0 +1,8 @@
> +// PR c++/100616
> +// { dg-additional-options "-std=c++20 -fmodules-ts" }
> +module pr100616;
> +
> +C<A{}> c2;
> +
> +using type = decltype(c1);
> +using type = decltype(c2);
> diff --git a/gcc/testsuite/g++.dg/modules/pr102576_a.H b/gcc/testsuite/g++.dg/modules/pr102576_a.H
> new file mode 100644
> index 00000000000..87ba9b52031
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/pr102576_a.H
> @@ -0,0 +1,5 @@
> +// PR c++/102576
> +// { dg-additional-options -fmodule-header }
> +// { dg-module-cmi {} }
> +
> +#include <initializer_list>
> diff --git a/gcc/testsuite/g++.dg/modules/pr102576_b.C b/gcc/testsuite/g++.dg/modules/pr102576_b.C
> new file mode 100644
> index 00000000000..10251ed5304
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/pr102576_b.C
> @@ -0,0 +1,9 @@
> +// PR c++/102576
> +// { dg-additional-options -fmodules-ts }
> +
> +import "pr102576_a.H";
> +
> +int main() {
> +  for (int i : {1, 2, 3})
> +    ;
> +}
> -- 
> 2.38.0.rc1.2.g4b79ee4b0c
>