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

[Patrick Palka <ppalka@redhat.com>]

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

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?

-- >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