Message ID | 20240209215001.1196422-2-ppalka@redhat.com |
---|---|
State | New |
Headers | show |
Series | [1/2] c++/modules: reduce lazy loading recursion | expand |
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 > >
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
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);
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 --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)