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 struct C { }; > > struct A { }; > > C c1; // #1 > > C 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 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 (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 struct C { }; +struct A { }; +C 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 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 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