diff mbox series

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

Message ID 20240209215001.1196422-2-ppalka@redhat.com
State New
Headers show
Series [1/2] c++/modules: reduce lazy loading recursion | expand

Commit Message

Patrick Palka Feb. 9, 2024, 9:50 p.m. UTC 
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(-)

Comments

Patrick Palka Feb. 9, 2024, 10:11 p.m. UTC | #1 
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
> 
>
Jason Merrill Feb. 10, 2024, 12:11 p.m. UTC | #2 
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
Patrick Palka Feb. 10, 2024, 6:37 p.m. UTC | #3 
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);
Jason Merrill Feb. 13, 2024, 12:15 a.m. UTC | #4 
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);
diff mbox series

Patch

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)