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

  When streaming in the artificial VAR_DECL synthesized for a class NTTP
argument, we end up crashing from complete_vars because the call to
maybe_register_incomplete_var from add_module_namespace_decl for this
VAR_DECL pushes an unexpected NULL_TREE type onto the incomplete_vars
vector.

This patch fixes this by checking for NULL_TREE before pushing onto
the vector.  This avoids the crash, but I noticed we still appear to
mishandle these artificial VAR_DECLs across translation units: the lookup
from get_template_parm_object for an existing VAR_DECL for the given
class NTTP argument fails to find the streamed-in VAR_DECL from the
other translation unit, so we end up creating a second VAR_DECL, but
that causes specialization equivalency issues in the XFAIL'd part of the
below test.  I'm afraid I don't understand why the lookup fails here
despite having done add_module_namespace_decl during stream-in, but
fixing the ICE seems like a safe and useful step towards enabling class
NTTP arguments used in modules.

Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
trunk?

	PR c++/100616

gcc/cp/ChangeLog:

	* decl.cc (maybe_register_incomplete_var): Check result of
	outermost_open_class.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/pr100616_a.C: New test.
	* g++.dg/modules/pr100616_b.C: New test.
---
 gcc/cp/decl.cc                            |  8 +++++---
 gcc/testsuite/g++.dg/modules/pr100616_a.C |  8 ++++++++
 gcc/testsuite/g++.dg/modules/pr100616_b.C | 10 ++++++++++
 3 files changed, 23 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/pr100616_a.C
 create mode 100644 gcc/testsuite/g++.dg/modules/pr100616_b.C
  

On 9/22/22 14:25, Patrick Palka wrote:

> index 80467c19254..722b64793ed 100644
> --- a/gcc/cp/decl.cc
> +++ b/gcc/cp/decl.cc
> @@ -18235,9 +18235,11 @@ maybe_register_incomplete_var (tree var)
>   	{
>   	  /* When the outermost open class is complete we can resolve any
>   	     pointers-to-members.  */
> -	  tree context = outermost_open_class ();
> -	  incomplete_var iv = {var, context};
> -	  vec_safe_push (incomplete_vars, iv);
> +	  if (tree context = outermost_open_class ())
> +	    {
> +	      incomplete_var iv = {var, context};
> +	      vec_safe_push (incomplete_vars, iv);
> +	    }

My immediate thought here is eek!  during stream in, the 
outermost_open_class could be anything -- to do with the context that 
wanted to lookup of the thing being streamed in, right?  So, the above 
change is I think just papering over a problem in this case.

not sure how to approach this.

nathan
  

On Thu, 22 Sep 2022, Nathan Sidwell wrote:

> On 9/22/22 14:25, Patrick Palka wrote:
> 
> > index 80467c19254..722b64793ed 100644
> > --- a/gcc/cp/decl.cc
> > +++ b/gcc/cp/decl.cc
> > @@ -18235,9 +18235,11 @@ maybe_register_incomplete_var (tree var)
> >   	{
> >   	  /* When the outermost open class is complete we can resolve any
> >   	     pointers-to-members.  */
> > -	  tree context = outermost_open_class ();
> > -	  incomplete_var iv = {var, context};
> > -	  vec_safe_push (incomplete_vars, iv);
> > +	  if (tree context = outermost_open_class ())
> > +	    {
> > +	      incomplete_var iv = {var, context};
> > +	      vec_safe_push (incomplete_vars, iv);
> > +	    }
> 
> My immediate thought here is eek!  during stream in, the outermost_open_class
> could be anything -- to do with the context that wanted to lookup of the thing
> being streamed in, right?  So, the above change is I think just papering over
> a problem in this case.

D'oh, makes sense.  I suppose this second branch of
maybe_register_incomplete_var shouldn't ever be taken during stream-in.

> 
> not sure how to approach this.

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?

Bootstrapped and regtested on x86_64-pc-linux-gnu.


-- >8 --

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

	PR c++/100616

gcc/cp/ChangeLog:

	* decl.cc (maybe_register_incomplete_var): Restrict second branch
	to static data members from a currently open class.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/pr100616_a.C: New test.
	* g++.dg/modules/pr100616_b.C: New test.
---
 gcc/cp/decl.cc                            |  2 ++
 gcc/testsuite/g++.dg/modules/pr100616_a.C |  8 ++++++++
 gcc/testsuite/g++.dg/modules/pr100616_b.C | 10 ++++++++++
 3 files changed, 20 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/modules/pr100616_a.C
 create mode 100644 gcc/testsuite/g++.dg/modules/pr100616_b.C

diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
index 80467c19254..ea616f0e686 100644
--- a/gcc/cp/decl.cc
+++ b/gcc/cp/decl.cc
@@ -18230,6 +18230,8 @@ maybe_register_incomplete_var (tree var)
 	  vec_safe_push (incomplete_vars, iv);
 	}
       else if (!(DECL_LANG_SPECIFIC (var) && DECL_TEMPLATE_INFO (var))
+	       && DECL_CLASS_SCOPE_P (var)
+	       && currently_open_class (DECL_CONTEXT (var))
 	       && decl_constant_var_p (var)
 	       && (TYPE_PTRMEM_P (inner_type) || CLASS_TYPE_P (inner_type)))
 	{
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..a37eb08131b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/pr100616_b.C
@@ -0,0 +1,10 @@
+// PR c++/100616
+// { dg-additional-options "-std=c++20 -fmodules-ts" }
+module pr100616;
+
+C<A{}> c2;
+
+// FIXME: We don't reuse the artificial VAR_DECL for the class NTTP argument A{}
+// from the other translation unit, which causes these types to be different.
+using type = decltype(c1);
+using type = decltype(c2); // { dg-bogus "conflicting" "" { xfail *-*-* } }
  

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?

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?

nathan
  

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?
> 
> 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).
  

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})
+    ;
+}
  

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})
+    ;
+}
  

On 9/26/22 15:05, Patrick Palka wrote:
> 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)

I'm going to have to think further.  Morally these VAR_DECLs are like 
the typeinfo objects -- which we have to handle specially.  Reminding 
myself, I see rtti.cc does the pushdecl_top_level stuff creating them -- 
so they go into the slot for the current TU.  But the streaming code 
writes tt_tinfo_var/tt_tinfo_typedef records, and recreates the typeinfo 
on stream in, using the same above pushdec_top_level path. So even 
though the tinfo decls might seem attached to the current module, that 
doesn;t confuse the streaming, nor create collisions on read back.  Nor 
do we stream out tinfo decls that are not reachable through the streamed 
AST (if we treated then as normal decls, we'd stream them cos they're 
inthe current TU in the symbol table.  I have the same fear for these 
NTTPs.)

It looks like TREE_LANG_FLAG_5 can be used to note these VAR_DECLs are 
NTTPs, and then the streaming can deal with them.  Let me look further.

> @@ -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);

this is wrong.  You're attaching the decl to the current module. which 
will mean conflicts when reading in such VAR_DECLs for the same NTTP 
from different modules.  Your test case might be hiding that as you have 
an interface and implementation unit from the same module (but you 
should be getting some kind of multiple definition error anyway?)


>   
>     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})
> +    ;
> +}
  

On Tue, 27 Sep 2022, Nathan Sidwell wrote:

> On 9/26/22 15:05, Patrick Palka wrote:
> > 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)
> 
> I'm going to have to think further.  Morally these VAR_DECLs are like the
> typeinfo objects -- which we have to handle specially.  Reminding myself, I
> see rtti.cc does the pushdecl_top_level stuff creating them -- so they go into
> the slot for the current TU.  But the streaming code writes
> tt_tinfo_var/tt_tinfo_typedef records, and recreates the typeinfo on stream
> in, using the same above pushdec_top_level path. So even though the tinfo
> decls might seem attached to the current module, that doesn;t confuse the
> streaming, nor create collisions on read back.  Nor do we stream out tinfo
> decls that are not reachable through the streamed AST (if we treated then as
> normal decls, we'd stream them cos they're inthe current TU in the symbol
> table.  I have the same fear for these NTTPs.)
> 
> It looks like TREE_LANG_FLAG_5 can be used to note these VAR_DECLs are NTTPs,
> and then the streaming can deal with them.  Let me look further.

I see, thanks very much for the enlightening explanation.

> 
> > @@ -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);
> 
> this is wrong.  You're attaching the decl to the current module. which will
> mean conflicts when reading in such VAR_DECLs for the same NTTP from different
> modules.  Your test case might be hiding that as you have an interface and
> implementation unit from the same module (but you should be getting some kind
> of multiple definition error anyway?)

Makes sense.  Indeed this approach falls apart for the following testcase
which uses the same NTTP argument in two different modules, for the
reasons you mentioned:

$ cat 100616_a.H
template<auto> struct C { };
struct A { };

$ cat 100616_b.C
export module pr100616_b;
import "100616_a.H";
export C<A{}> c1;

$ cat 100616_c.C
export module pr100616_c;
import "100616_a.H";
export C<A{}> c2;

$ cat 100616_d.C
import pr100616_b;
import pr100616_c;
using type = decltype(c1);
using type = decltype(c2); // bogus error: types of c1 and c2 don't match

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

What do you think about this independent change to use
lookup_qualified_name instead of get_namespace_binding in listify so
that the lookup for std::initializer_list is import-aware, which seems
to fix PR102576?

> > 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})
> > +    ;
> > +}
> 
> -- 
> Nathan Sidwell
> 
>
  

On 9/28/22 10:42, Patrick Palka wrote:
> On Tue, 27 Sep 2022, Nathan Sidwell wrote:
> 
>> On 9/26/22 15:05, Patrick Palka wrote:
>>> On Mon, 26 Sep 2022, Patrick Palka wrote:
>>>
>>>> On Mon, 26 Sep 2022, Nathan Sidwell wrote:

>>
>>
>>>        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);
> 
> What do you think about this independent change to use
> lookup_qualified_name instead of get_namespace_binding in listify so
> that the lookup for std::initializer_list is import-aware, which seems
> to fix PR102576?

Yes, that looks right to me, thanks!  (I think it'll also fix a 
potential future problem if we ever have:

namespace std {
inline namespace v2 {
template <stuff> class initializer_list {...};
}}

> 
>>> 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})
>>> +    ;
>>> +}
>>
>> -- 
>> Nathan Sidwell
>>