[PATCH 1/2] c++/modules: reduce lazy loading recursion

[PATCH 1/2] c++/modules: reduce lazy loading recursion

[Patrick Palka]

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

-- >8 --

It turns out that with modules we can call mangle_decl recursively,
which is a problem because the global mangling state isn't recursion
aware.  The recursion happens from write_closure_type_name, which calls
lambda_function, which performs name lookup, which can trigger lazy
loading, which can call maybe_clone_body for a newly loaded cdtor, which
can inspect DECL_ASSEMBLER_NAME, which enters mangling.  This was observed
when using fmtlib as a module with trunk and it leads to a bogus
"mangling conflicts with a previous mangle error" followed by an ICE
from cdtor_comdat_group due to a mangling mismatch.

This patch fixes this by sidestepping lazy loading when performing the
op() lookup in lambda_function, so that we don't accidentally end up
entering mangling recursively.  This should be safe since the lazy load
should still get triggered by some other name lookup.

In passing it was noticed that lazy loading can get quite recursive
ultimately due to the name lookups from check_local_shadow, which may
perform lazy loading, which cause us to instantiate/clone things, which
end up calling check_local_shadow.  This patch fixes this by
implementating an optimization suggested by Jason:

> I think we shouldn't bother with check_local_shadow in a clone or
> instantiation, only when actually parsing.

This reduces the maximum depth of lazy loading recursion for a simple
modular Hello World from ~115 to ~12.

gcc/cp/ChangeLog:

	* lambda.cc (lambda_function): Call get_class_binding_direct
	instead of lookup_member to sidestep lazy loading.
	* name-lookup.cc (check_local_shadow): Punt if we're in a
	function context that's definitely not actual parsing.
---
 gcc/cp/lambda.cc      |  4 +---
 gcc/cp/name-lookup.cc | 17 +++++++++++++++++
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/gcc/cp/lambda.cc b/gcc/cp/lambda.cc
index 1d37e5a52b9..4b1f9391fee 100644
--- a/gcc/cp/lambda.cc
+++ b/gcc/cp/lambda.cc
@@ -175,9 +175,7 @@ lambda_function (tree lambda)
   if (CLASSTYPE_TEMPLATE_INSTANTIATION (type)
       && !COMPLETE_OR_OPEN_TYPE_P (type))
     return NULL_TREE;
-  lambda = lookup_member (type, call_op_identifier,
-			  /*protect=*/0, /*want_type=*/false,
-			  tf_warning_or_error);
+  lambda = get_class_binding_direct (type, call_op_identifier);
   if (lambda)
     lambda = STRIP_TEMPLATE (get_first_fn (lambda));
   return lambda;
diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
index e58f3b5cb4d..ca5ba87bc76 100644
--- a/gcc/cp/name-lookup.cc
+++ b/gcc/cp/name-lookup.cc
@@ -3275,6 +3275,23 @@ check_local_shadow (tree decl)
   if (TREE_CODE (decl) == PARM_DECL && !DECL_CONTEXT (decl))
     return NULL_TREE;
 
+  if (DECL_FUNCTION_SCOPE_P (decl))
+    {
+      tree ctx = DECL_CONTEXT (decl);
+      if (DECL_CLONED_FUNCTION_P (ctx)
+	  || DECL_TEMPLATE_INSTANTIATED (ctx)
+	  || (DECL_LANG_SPECIFIC (ctx)
+	      && DECL_DEFAULTED_FN (ctx))
+	  || (LAMBDA_FUNCTION_P (ctx)
+	      && LAMBDA_EXPR_REGEN_INFO (CLASSTYPE_LAMBDA_EXPR
+					 (DECL_CONTEXT (ctx)))))
+	/* It suffices to check shadowing only when actually parsing.
+	   So punt for clones, instantiations, defaulted functions and
+	   regenerated lambdas.  This optimization helps lazy loading
+	   cascades with modules.  */
+	return NULL_TREE;
+    }
+
   tree old = NULL_TREE;
   cp_binding_level *old_scope = NULL;
   if (cxx_binding *binding = outer_binding (DECL_NAME (decl), NULL, true))
-- 
2.43.0.561.g235986be82

[PATCH 2/2] c++/modules: ICE with modular fmtlib

[Patrick Palka]

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

I'll try to reduce and add testcases overnight for these separate bugs
before pushing.

-- >8 --

Building modular fmtlib triggered two small modules bugs in C++23 and
C++26 mode respectively (due to libstdc++ header differences).

The first is that a TEMPLATE_DECL having DECL_LANG_SPECIFIC doesn't
necessarily imply that its DECL_TEMPLATE_RESULT has DECL_LANG_SPECIFIC.
So we need to use STRIP_TEMPLATE consistently; this is a follow-up to
r12-7187-gdb84f382ae3dc2.

The second is that get_originating_module_decl was ICEing on class-scope
enumerators injected via using-enum.

gcc/cp/ChangeLog:

	* module.cc (depset::hash::add_specializations): Use
	STRIP_TEMPLATE consistently.
	(get_originating_module_decl): Handle class-scope CONST_DECL.
---
 gcc/cp/module.cc | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index 3c2fef0e3f4..659fa03dae1 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -13248,7 +13248,7 @@ depset::hash::add_specializations (bool decl_p)
       if (use_tpl == 1)
 	/* Implicit instantiations only walked if we reach them.  */
 	needs_reaching = true;
-      else if (!DECL_LANG_SPECIFIC (spec)
+      else if (!DECL_LANG_SPECIFIC (STRIP_TEMPLATE (spec))
 	       || !DECL_MODULE_PURVIEW_P (STRIP_TEMPLATE (spec)))
 	/* Likewise, GMF explicit or partial specializations.  */
 	needs_reaching = true;
@@ -18708,7 +18708,8 @@ get_originating_module_decl (tree decl)
       && (TREE_CODE (DECL_CONTEXT (decl)) == ENUMERAL_TYPE))
     decl = TYPE_NAME (DECL_CONTEXT (decl));
   else if (TREE_CODE (decl) == FIELD_DECL
-	   || TREE_CODE (decl) == USING_DECL)
+	   || TREE_CODE (decl) == USING_DECL
+	   || TREE_CODE (decl) == CONST_DECL)
     {
       decl = DECL_CONTEXT (decl);
       if (TREE_CODE (decl) != FUNCTION_DECL)
-- 
2.43.0.561.g235986be82

Re: [PATCH 2/2] c++/modules: ICE with modular fmtlib

[Patrick Palka]

On Fri, 9 Feb 2024, Patrick Palka wrote:

> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look
> OK for trunk?
> 
> I'll try to reduce and add testcases overnight for these separate bugs
> before pushing.
> 
> -- >8 --
> 
> Building modular fmtlib triggered two small modules bugs in C++23 and
> C++26 mode respectively (due to libstdc++ header differences).
> 
> The first is that a TEMPLATE_DECL having DECL_LANG_SPECIFIC doesn't
> necessarily imply that its DECL_TEMPLATE_RESULT has DECL_LANG_SPECIFIC.
> So we need to use STRIP_TEMPLATE consistently; this is a follow-up to
> r12-7187-gdb84f382ae3dc2.
> 
> The second is that get_originating_module_decl was ICEing on class-scope
> enumerators injected via using-enum.
> 
> gcc/cp/ChangeLog:
> 
> 	* module.cc (depset::hash::add_specializations): Use
> 	STRIP_TEMPLATE consistently.
> 	(get_originating_module_decl): Handle class-scope CONST_DECL.
> ---
>  gcc/cp/module.cc | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index 3c2fef0e3f4..659fa03dae1 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -13248,7 +13248,7 @@ depset::hash::add_specializations (bool decl_p)
>        if (use_tpl == 1)
>  	/* Implicit instantiations only walked if we reach them.  */
>  	needs_reaching = true;
> -      else if (!DECL_LANG_SPECIFIC (spec)
> +      else if (!DECL_LANG_SPECIFIC (STRIP_TEMPLATE (spec))
>  	       || !DECL_MODULE_PURVIEW_P (STRIP_TEMPLATE (spec)))
>  	/* Likewise, GMF explicit or partial specializations.  */
>  	needs_reaching = true;
> @@ -18708,7 +18708,8 @@ get_originating_module_decl (tree decl)
>        && (TREE_CODE (DECL_CONTEXT (decl)) == ENUMERAL_TYPE))
>      decl = TYPE_NAME (DECL_CONTEXT (decl));
>    else if (TREE_CODE (decl) == FIELD_DECL
> -	   || TREE_CODE (decl) == USING_DECL)
> +	   || TREE_CODE (decl) == USING_DECL
> +	   || TREE_CODE (decl) == CONST_DECL)

On second thought maybe we should test for CONST_DECL_USING_P (decl)
specifically.  In other contexts a CONST_DECL could be a template
parameter, so using CONST_DECL_USING_P makes this code more readable
arguably.

>      {
>        decl = DECL_CONTEXT (decl);
>        if (TREE_CODE (decl) != FUNCTION_DECL)
> -- 
> 2.43.0.561.g235986be82
> 
> 

Re: [PATCH 1/2] c++/modules: reduce lazy loading recursion

[Jason Merrill]

On 2/9/24 16:50, Patrick Palka wrote:
> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look
> OK for trunk
> 
> -- >8 --
> 
> It turns out that with modules we can call mangle_decl recursively,
> which is a problem because the global mangling state isn't recursion
> aware.  The recursion happens from write_closure_type_name, which calls
> lambda_function, which performs name lookup, which can trigger lazy
> loading, which can call maybe_clone_body for a newly loaded cdtor, which
> can inspect DECL_ASSEMBLER_NAME, which enters mangling.  This was observed
> when using fmtlib as a module with trunk and it leads to a bogus
> "mangling conflicts with a previous mangle error" followed by an ICE
> from cdtor_comdat_group due to a mangling mismatch.
> 
> This patch fixes this by sidestepping lazy loading when performing the
> op() lookup in lambda_function, so that we don't accidentally end up
> entering mangling recursively.  This should be safe since the lazy load
> should still get triggered by some other name lookup.
> 
> In passing it was noticed that lazy loading can get quite recursive
> ultimately due to the name lookups from check_local_shadow, which may
> perform lazy loading, which cause us to instantiate/clone things, which
> end up calling check_local_shadow.  This patch fixes this by
> implementating an optimization suggested by Jason:
> 
>> I think we shouldn't bother with check_local_shadow in a clone or
>> instantiation, only when actually parsing.
> 
> This reduces the maximum depth of lazy loading recursion for a simple
> modular Hello World from ~115 to ~12.
> 
> gcc/cp/ChangeLog:
> 
> 	* lambda.cc (lambda_function): Call get_class_binding_direct
> 	instead of lookup_member to sidestep lazy loading.
> 	* name-lookup.cc (check_local_shadow): Punt if we're in a
> 	function context that's definitely not actual parsing.
> ---
>   gcc/cp/lambda.cc      |  4 +---
>   gcc/cp/name-lookup.cc | 17 +++++++++++++++++
>   2 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/gcc/cp/lambda.cc b/gcc/cp/lambda.cc
> index 1d37e5a52b9..4b1f9391fee 100644
> --- a/gcc/cp/lambda.cc
> +++ b/gcc/cp/lambda.cc
> @@ -175,9 +175,7 @@ lambda_function (tree lambda)
>     if (CLASSTYPE_TEMPLATE_INSTANTIATION (type)
>         && !COMPLETE_OR_OPEN_TYPE_P (type))
>       return NULL_TREE;
> -  lambda = lookup_member (type, call_op_identifier,
> -			  /*protect=*/0, /*want_type=*/false,
> -			  tf_warning_or_error);
> +  lambda = get_class_binding_direct (type, call_op_identifier);
>     if (lambda)
>       lambda = STRIP_TEMPLATE (get_first_fn (lambda));
>     return lambda;
> diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
> index e58f3b5cb4d..ca5ba87bc76 100644
> --- a/gcc/cp/name-lookup.cc
> +++ b/gcc/cp/name-lookup.cc
> @@ -3275,6 +3275,23 @@ check_local_shadow (tree decl)
>     if (TREE_CODE (decl) == PARM_DECL && !DECL_CONTEXT (decl))
>       return NULL_TREE;
>   
> +  if (DECL_FUNCTION_SCOPE_P (decl))
> +    {
> +      tree ctx = DECL_CONTEXT (decl);
> +      if (DECL_CLONED_FUNCTION_P (ctx)
> +	  || DECL_TEMPLATE_INSTANTIATED (ctx)
> +	  || (DECL_LANG_SPECIFIC (ctx)
> +	      && DECL_DEFAULTED_FN (ctx))
> +	  || (LAMBDA_FUNCTION_P (ctx)
> +	      && LAMBDA_EXPR_REGEN_INFO (CLASSTYPE_LAMBDA_EXPR
> +					 (DECL_CONTEXT (ctx)))))

Maybe these tests should be factored out in case other places want to 
check the same condition?  OK either way.

> +	/* It suffices to check shadowing only when actually parsing.
> +	   So punt for clones, instantiations, defaulted functions and
> +	   regenerated lambdas.  This optimization helps lazy loading
> +	   cascades with modules.  */
> +	return NULL_TREE;
> +    }
> +
>     tree old = NULL_TREE;
>     cp_binding_level *old_scope = NULL;
>     if (cxx_binding *binding = outer_binding (DECL_NAME (decl), NULL, true))

Re: [PATCH 2/2] c++/modules: ICE with modular fmtlib

[Jason Merrill]

On 2/9/24 17:11, Patrick Palka wrote:
> On Fri, 9 Feb 2024, Patrick Palka wrote:
> 
>> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look
>> OK for trunk?
>>
>> I'll try to reduce and add testcases overnight for these separate bugs
>> before pushing.
>>
>> -- >8 --
>>
>> Building modular fmtlib triggered two small modules bugs in C++23 and
>> C++26 mode respectively (due to libstdc++ header differences).
>>
>> The first is that a TEMPLATE_DECL having DECL_LANG_SPECIFIC doesn't
>> necessarily imply that its DECL_TEMPLATE_RESULT has DECL_LANG_SPECIFIC.
>> So we need to use STRIP_TEMPLATE consistently; this is a follow-up to
>> r12-7187-gdb84f382ae3dc2.
>>
>> The second is that get_originating_module_decl was ICEing on class-scope
>> enumerators injected via using-enum.
>>
>> gcc/cp/ChangeLog:
>>
>> 	* module.cc (depset::hash::add_specializations): Use
>> 	STRIP_TEMPLATE consistently.
>> 	(get_originating_module_decl): Handle class-scope CONST_DECL.
>> ---
>>   gcc/cp/module.cc | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
>> index 3c2fef0e3f4..659fa03dae1 100644
>> --- a/gcc/cp/module.cc
>> +++ b/gcc/cp/module.cc
>> @@ -13248,7 +13248,7 @@ depset::hash::add_specializations (bool decl_p)
>>         if (use_tpl == 1)
>>   	/* Implicit instantiations only walked if we reach them.  */
>>   	needs_reaching = true;
>> -      else if (!DECL_LANG_SPECIFIC (spec)
>> +      else if (!DECL_LANG_SPECIFIC (STRIP_TEMPLATE (spec))
>>   	       || !DECL_MODULE_PURVIEW_P (STRIP_TEMPLATE (spec)))
>>   	/* Likewise, GMF explicit or partial specializations.  */
>>   	needs_reaching = true;
>> @@ -18708,7 +18708,8 @@ get_originating_module_decl (tree decl)
>>         && (TREE_CODE (DECL_CONTEXT (decl)) == ENUMERAL_TYPE))
>>       decl = TYPE_NAME (DECL_CONTEXT (decl));
>>     else if (TREE_CODE (decl) == FIELD_DECL
>> -	   || TREE_CODE (decl) == USING_DECL)
>> +	   || TREE_CODE (decl) == USING_DECL
>> +	   || TREE_CODE (decl) == CONST_DECL)
> 
> On second thought maybe we should test for CONST_DECL_USING_P (decl)
> specifically.  In other contexts a CONST_DECL could be a template
> parameter, so using CONST_DECL_USING_P makes this code more readable
> arguably.

That makes sense.

Jason

Re: [PATCH 2/2] c++/modules: ICE with modular fmtlib

[Patrick Palka]

On Sat, 10 Feb 2024, Jason Merrill wrote:

> On 2/9/24 17:11, Patrick Palka wrote:
> > On Fri, 9 Feb 2024, Patrick Palka wrote:
> > 
> > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look
> > > OK for trunk?
> > > 
> > > I'll try to reduce and add testcases overnight for these separate bugs
> > > before pushing.
> > > 
> > > -- >8 --
> > > 
> > > Building modular fmtlib triggered two small modules bugs in C++23 and
> > > C++26 mode respectively (due to libstdc++ header differences).
> > > 
> > > The first is that a TEMPLATE_DECL having DECL_LANG_SPECIFIC doesn't
> > > necessarily imply that its DECL_TEMPLATE_RESULT has DECL_LANG_SPECIFIC.
> > > So we need to use STRIP_TEMPLATE consistently; this is a follow-up to
> > > r12-7187-gdb84f382ae3dc2.
> > > 
> > > The second is that get_originating_module_decl was ICEing on class-scope
> > > enumerators injected via using-enum.
> > > 
> > > gcc/cp/ChangeLog:
> > > 
> > > 	* module.cc (depset::hash::add_specializations): Use
> > > 	STRIP_TEMPLATE consistently.
> > > 	(get_originating_module_decl): Handle class-scope CONST_DECL.
> > > ---
> > >   gcc/cp/module.cc | 5 +++--
> > >   1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> > > index 3c2fef0e3f4..659fa03dae1 100644
> > > --- a/gcc/cp/module.cc
> > > +++ b/gcc/cp/module.cc
> > > @@ -13248,7 +13248,7 @@ depset::hash::add_specializations (bool decl_p)
> > >         if (use_tpl == 1)
> > >   	/* Implicit instantiations only walked if we reach them.  */
> > >   	needs_reaching = true;
> > > -      else if (!DECL_LANG_SPECIFIC (spec)
> > > +      else if (!DECL_LANG_SPECIFIC (STRIP_TEMPLATE (spec))
> > >   	       || !DECL_MODULE_PURVIEW_P (STRIP_TEMPLATE (spec)))
> > >   	/* Likewise, GMF explicit or partial specializations.  */
> > >   	needs_reaching = true;
> > > @@ -18708,7 +18708,8 @@ get_originating_module_decl (tree decl)
> > >         && (TREE_CODE (DECL_CONTEXT (decl)) == ENUMERAL_TYPE))
> > >       decl = TYPE_NAME (DECL_CONTEXT (decl));
> > >     else if (TREE_CODE (decl) == FIELD_DECL
> > > -	   || TREE_CODE (decl) == USING_DECL)
> > > +	   || TREE_CODE (decl) == USING_DECL
> > > +	   || TREE_CODE (decl) == CONST_DECL)
> > 
> > On second thought maybe we should test for CONST_DECL_USING_P (decl)
> > specifically.  In other contexts a CONST_DECL could be a template
> > parameter, so using CONST_DECL_USING_P makes this code more readable
> > arguably.
> 
> That makes sense.

Like so?  Now with reduced testcases:

-- >8 --

Subject: [PATCH] c++/modules: ICEs with modular fmtlib

gcc/cp/ChangeLog:

	* module.cc (depset::hash::add_specializations): Use
	STRIP_TEMPLATE consistently.
	(get_originating_module_decl): Handle class-scope CONST_DECL.

gcc/testsuite/ChangeLog:

	* gcc/testsuite/g++.dg/modules/friend-6_a.C: New test.
	* gcc/testsuite/g++.dg/modules/using-enum-3_a.C: New test.
	* gcc/testsuite/g++.dg/modules/using-enum-3_b.C: New test.
---
 gcc/cp/module.cc                              |  5 +++--
 gcc/testsuite/g++.dg/modules/friend-6_a.C     | 11 +++++++++++
 gcc/testsuite/g++.dg/modules/using-enum-3_a.C | 10 ++++++++++
 gcc/testsuite/g++.dg/modules/using-enum-3_b.C |  5 +++++
 4 files changed, 29 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/friend-6_a.C
 create mode 100644 gcc/testsuite/g++.dg/modules/using-enum-3_a.C
 create mode 100644 gcc/testsuite/g++.dg/modules/using-enum-3_b.C

diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index 3c2fef0e3f4..86e43aee542 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -13248,7 +13248,7 @@ depset::hash::add_specializations (bool decl_p)
       if (use_tpl == 1)
 	/* Implicit instantiations only walked if we reach them.  */
 	needs_reaching = true;
-      else if (!DECL_LANG_SPECIFIC (spec)
+      else if (!DECL_LANG_SPECIFIC (STRIP_TEMPLATE (spec))
 	       || !DECL_MODULE_PURVIEW_P (STRIP_TEMPLATE (spec)))
 	/* Likewise, GMF explicit or partial specializations.  */
 	needs_reaching = true;
@@ -18708,7 +18708,8 @@ get_originating_module_decl (tree decl)
       && (TREE_CODE (DECL_CONTEXT (decl)) == ENUMERAL_TYPE))
     decl = TYPE_NAME (DECL_CONTEXT (decl));
   else if (TREE_CODE (decl) == FIELD_DECL
-	   || TREE_CODE (decl) == USING_DECL)
+	   || TREE_CODE (decl) == USING_DECL
+	   || CONST_DECL_USING_P (decl))
     {
       decl = DECL_CONTEXT (decl);
       if (TREE_CODE (decl) != FUNCTION_DECL)
diff --git a/gcc/testsuite/g++.dg/modules/friend-6_a.C b/gcc/testsuite/g++.dg/modules/friend-6_a.C
new file mode 100644
index 00000000000..3b3d714b3f3
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/friend-6_a.C
@@ -0,0 +1,11 @@
+// { dg-additional-options "-fmodules-ts -Wno-pedantic" }
+// { dg-module-cmi friend_6 }
+
+module;
+# 1 "" 1
+template<class> struct Trans_NS___cxx11_basic_string {
+  template<class> friend class basic_stringbuf;
+};
+template struct Trans_NS___cxx11_basic_string<char>;
+# 6 "" 2
+export module friend_6;
diff --git a/gcc/testsuite/g++.dg/modules/using-enum-3_a.C b/gcc/testsuite/g++.dg/modules/using-enum-3_a.C
new file mode 100644
index 00000000000..e2651f36462
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/using-enum-3_a.C
@@ -0,0 +1,10 @@
+// { dg-do compile { target c++20 } }
+// { dg-additional-options "-fmodules-ts" }
+// { dg-module-cmi using_enum_3 }
+
+export module using_enum_3;
+export
+struct text_encoding {
+  enum class id { CP50220 };
+  using enum id;
+};
diff --git a/gcc/testsuite/g++.dg/modules/using-enum-3_b.C b/gcc/testsuite/g++.dg/modules/using-enum-3_b.C
new file mode 100644
index 00000000000..719dc0f2b84
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/using-enum-3_b.C
@@ -0,0 +1,5 @@
+// { dg-do compile { target c++20 } }
+// { dg-additional-options "-fmodules-ts" }
+import using_enum_3;
+
+static_assert(text_encoding::id::CP50220 == text_encoding::CP50220);
-- 
2.43.0.561.g235986be82

Re: [PATCH 2/2] c++/modules: ICE with modular fmtlib

[Jason Merrill]

On 2/10/24 13:37, Patrick Palka wrote:
> On Sat, 10 Feb 2024, Jason Merrill wrote:
> 
>> On 2/9/24 17:11, Patrick Palka wrote:
>>> On Fri, 9 Feb 2024, Patrick Palka wrote:
>>>
>>>> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look
>>>> OK for trunk?
>>>>
>>>> I'll try to reduce and add testcases overnight for these separate bugs
>>>> before pushing.
>>>>
>>>> -- >8 --
>>>>
>>>> Building modular fmtlib triggered two small modules bugs in C++23 and
>>>> C++26 mode respectively (due to libstdc++ header differences).
>>>>
>>>> The first is that a TEMPLATE_DECL having DECL_LANG_SPECIFIC doesn't
>>>> necessarily imply that its DECL_TEMPLATE_RESULT has DECL_LANG_SPECIFIC.
>>>> So we need to use STRIP_TEMPLATE consistently; this is a follow-up to
>>>> r12-7187-gdb84f382ae3dc2.
>>>>
>>>> The second is that get_originating_module_decl was ICEing on class-scope
>>>> enumerators injected via using-enum.
>>>>
>>>> gcc/cp/ChangeLog:
>>>>
>>>> 	* module.cc (depset::hash::add_specializations): Use
>>>> 	STRIP_TEMPLATE consistently.
>>>> 	(get_originating_module_decl): Handle class-scope CONST_DECL.
>>>> ---
>>>>    gcc/cp/module.cc | 5 +++--
>>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
>>>> index 3c2fef0e3f4..659fa03dae1 100644
>>>> --- a/gcc/cp/module.cc
>>>> +++ b/gcc/cp/module.cc
>>>> @@ -13248,7 +13248,7 @@ depset::hash::add_specializations (bool decl_p)
>>>>          if (use_tpl == 1)
>>>>    	/* Implicit instantiations only walked if we reach them.  */
>>>>    	needs_reaching = true;
>>>> -      else if (!DECL_LANG_SPECIFIC (spec)
>>>> +      else if (!DECL_LANG_SPECIFIC (STRIP_TEMPLATE (spec))
>>>>    	       || !DECL_MODULE_PURVIEW_P (STRIP_TEMPLATE (spec)))
>>>>    	/* Likewise, GMF explicit or partial specializations.  */
>>>>    	needs_reaching = true;
>>>> @@ -18708,7 +18708,8 @@ get_originating_module_decl (tree decl)
>>>>          && (TREE_CODE (DECL_CONTEXT (decl)) == ENUMERAL_TYPE))
>>>>        decl = TYPE_NAME (DECL_CONTEXT (decl));
>>>>      else if (TREE_CODE (decl) == FIELD_DECL
>>>> -	   || TREE_CODE (decl) == USING_DECL)
>>>> +	   || TREE_CODE (decl) == USING_DECL
>>>> +	   || TREE_CODE (decl) == CONST_DECL)
>>>
>>> On second thought maybe we should test for CONST_DECL_USING_P (decl)
>>> specifically.  In other contexts a CONST_DECL could be a template
>>> parameter, so using CONST_DECL_USING_P makes this code more readable
>>> arguably.
>>
>> That makes sense.
> 
> Like so?  Now with reduced testcases:

OK.

> -- >8 --
> 
> Subject: [PATCH] c++/modules: ICEs with modular fmtlib
> 
> gcc/cp/ChangeLog:
> 
> 	* module.cc (depset::hash::add_specializations): Use
> 	STRIP_TEMPLATE consistently.
> 	(get_originating_module_decl): Handle class-scope CONST_DECL.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc/testsuite/g++.dg/modules/friend-6_a.C: New test.
> 	* gcc/testsuite/g++.dg/modules/using-enum-3_a.C: New test.
> 	* gcc/testsuite/g++.dg/modules/using-enum-3_b.C: New test.
> ---
>   gcc/cp/module.cc                              |  5 +++--
>   gcc/testsuite/g++.dg/modules/friend-6_a.C     | 11 +++++++++++
>   gcc/testsuite/g++.dg/modules/using-enum-3_a.C | 10 ++++++++++
>   gcc/testsuite/g++.dg/modules/using-enum-3_b.C |  5 +++++
>   4 files changed, 29 insertions(+), 2 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/modules/friend-6_a.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/using-enum-3_a.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/using-enum-3_b.C
> 
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index 3c2fef0e3f4..86e43aee542 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -13248,7 +13248,7 @@ depset::hash::add_specializations (bool decl_p)
>         if (use_tpl == 1)
>   	/* Implicit instantiations only walked if we reach them.  */
>   	needs_reaching = true;
> -      else if (!DECL_LANG_SPECIFIC (spec)
> +      else if (!DECL_LANG_SPECIFIC (STRIP_TEMPLATE (spec))
>   	       || !DECL_MODULE_PURVIEW_P (STRIP_TEMPLATE (spec)))
>   	/* Likewise, GMF explicit or partial specializations.  */
>   	needs_reaching = true;
> @@ -18708,7 +18708,8 @@ get_originating_module_decl (tree decl)
>         && (TREE_CODE (DECL_CONTEXT (decl)) == ENUMERAL_TYPE))
>       decl = TYPE_NAME (DECL_CONTEXT (decl));
>     else if (TREE_CODE (decl) == FIELD_DECL
> -	   || TREE_CODE (decl) == USING_DECL)
> +	   || TREE_CODE (decl) == USING_DECL
> +	   || CONST_DECL_USING_P (decl))
>       {
>         decl = DECL_CONTEXT (decl);
>         if (TREE_CODE (decl) != FUNCTION_DECL)
> diff --git a/gcc/testsuite/g++.dg/modules/friend-6_a.C b/gcc/testsuite/g++.dg/modules/friend-6_a.C
> new file mode 100644
> index 00000000000..3b3d714b3f3
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/friend-6_a.C
> @@ -0,0 +1,11 @@
> +// { dg-additional-options "-fmodules-ts -Wno-pedantic" }
> +// { dg-module-cmi friend_6 }
> +
> +module;
> +# 1 "" 1
> +template<class> struct Trans_NS___cxx11_basic_string {
> +  template<class> friend class basic_stringbuf;
> +};
> +template struct Trans_NS___cxx11_basic_string<char>;
> +# 6 "" 2
> +export module friend_6;
> diff --git a/gcc/testsuite/g++.dg/modules/using-enum-3_a.C b/gcc/testsuite/g++.dg/modules/using-enum-3_a.C
> new file mode 100644
> index 00000000000..e2651f36462
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/using-enum-3_a.C
> @@ -0,0 +1,10 @@
> +// { dg-do compile { target c++20 } }
> +// { dg-additional-options "-fmodules-ts" }
> +// { dg-module-cmi using_enum_3 }
> +
> +export module using_enum_3;
> +export
> +struct text_encoding {
> +  enum class id { CP50220 };
> +  using enum id;
> +};
> diff --git a/gcc/testsuite/g++.dg/modules/using-enum-3_b.C b/gcc/testsuite/g++.dg/modules/using-enum-3_b.C
> new file mode 100644
> index 00000000000..719dc0f2b84
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/using-enum-3_b.C
> @@ -0,0 +1,5 @@
> +// { dg-do compile { target c++20 } }
> +// { dg-additional-options "-fmodules-ts" }
> +import using_enum_3;
> +
> +static_assert(text_encoding::id::CP50220 == text_encoding::CP50220);