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)
/* 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
