| Message ID | 20240207172911.4173062-1-ppalka@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | libstdc++: Work around modules issue causing hello-1 ICE [PR113710] | expand |
On Wed, 7 Feb 2024 at 17:29, Patrick Palka <ppalka@redhat.com> wrote: > Tested on x86_64-pc-linux-gnu, does this look OK for trunk? > OK. Do we have a reduced testcase to track the modules bug that will still exist in the FE? > > -- >8 -- > > The forward declarations of std::get in <bits/stl_pair.h> added in > r14-8710-g65b4cba9d6a9ff are causing an ICE in the test modules/hello-1 > due to what seems to be a declaration merging issue in modules. > > What seems to be happening is that in hello-1_b.C we first include > <string_view>, which indirectly includes <bits/stl_pair.h> which forms > the dependent specialization tuple_element<__i, tuple<_Elements...>> > (appearing in some of the std::get overloads) and adds it to the > specializations table. > > We then import hello which indirectly includes <tuple> (in the GMF), > within which we define a partial specialization of tuple_element with > that same template-id. So importing hello in turn streams in this > partial specialization, but we don't notice it matches the previously > created dependent specialization, and we end up with two equivalent > types for this template-id with different TYPE_CANONICAL. > > This patch works around this issue by adding a forward declaration of > the tuple_element partial specialization from <tuple> to <bits/stl_pair.h> > so that it appears alongside the dependent specialization of the same > template-id. So when including <bits/stl_pair.h> we immediately register > the template-id as a partial specialization, and if we later stream in the > partial specialization the MK_partial case of trees_in::key_mergeable will > match them up. (So perhaps a proper modules fix for this might be to make > key_mergeable try to match up a streamed in partial specialization with an > existing specialization from the table via match_mergeable_specialization.) > > PR c++/113710 > > libstdc++-v3/ChangeLog: > > * include/bits/stl_pair.h (tuple_element): Add forward > declaration of the partial specialization for tuple. > --- > libstdc++-v3/include/bits/stl_pair.h | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/libstdc++-v3/include/bits/stl_pair.h > b/libstdc++-v3/include/bits/stl_pair.h > index 00ec53ebc33..8c71b1350e5 100644 > --- a/libstdc++-v3/include/bits/stl_pair.h > +++ b/libstdc++-v3/include/bits/stl_pair.h > @@ -1153,6 +1153,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > struct tuple_element<1, pair<_Tp1, _Tp2>> > { typedef _Tp2 type; }; > > + // Forward declare the partial specialization for std::tuple > + // to work around modules bug PR c++/113710. > + template<size_t __i, typename... _Types> > + struct tuple_element<__i, tuple<_Types...>>; > + > #if __cplusplus >= 201703L > template<typename _Tp1, typename _Tp2> > inline constexpr size_t tuple_size_v<pair<_Tp1, _Tp2>> = 2; > -- > 2.43.0.561.g235986be82 > >
On Wed, 7 Feb 2024, Jonathan Wakely wrote: > > > On Wed, 7 Feb 2024 at 17:29, Patrick Palka <ppalka@redhat.com> wrote: > Tested on x86_64-pc-linux-gnu, does this look OK for trunk? > > > OK. > > Do we have a reduced testcase to track the modules bug that will still exist in the FE? https://gcc.gnu.org/PR113814 > > > > -- >8 -- > > The forward declarations of std::get in <bits/stl_pair.h> added in > r14-8710-g65b4cba9d6a9ff are causing an ICE in the test modules/hello-1 > due to what seems to be a declaration merging issue in modules. > > What seems to be happening is that in hello-1_b.C we first include > <string_view>, which indirectly includes <bits/stl_pair.h> which forms > the dependent specialization tuple_element<__i, tuple<_Elements...>> > (appearing in some of the std::get overloads) and adds it to the > specializations table. > > We then import hello which indirectly includes <tuple> (in the GMF), > within which we define a partial specialization of tuple_element with > that same template-id. So importing hello in turn streams in this > partial specialization, but we don't notice it matches the previously > created dependent specialization, and we end up with two equivalent > types for this template-id with different TYPE_CANONICAL. > > This patch works around this issue by adding a forward declaration of > the tuple_element partial specialization from <tuple> to <bits/stl_pair.h> > so that it appears alongside the dependent specialization of the same > template-id. So when including <bits/stl_pair.h> we immediately register > the template-id as a partial specialization, and if we later stream in the > partial specialization the MK_partial case of trees_in::key_mergeable will > match them up. (So perhaps a proper modules fix for this might be to make > key_mergeable try to match up a streamed in partial specialization with an > existing specialization from the table via match_mergeable_specialization.) > > PR c++/113710 > > libstdc++-v3/ChangeLog: > > * include/bits/stl_pair.h (tuple_element): Add forward > declaration of the partial specialization for tuple. > --- > libstdc++-v3/include/bits/stl_pair.h | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/libstdc++-v3/include/bits/stl_pair.h b/libstdc++-v3/include/bits/stl_pair.h > index 00ec53ebc33..8c71b1350e5 100644 > --- a/libstdc++-v3/include/bits/stl_pair.h > +++ b/libstdc++-v3/include/bits/stl_pair.h > @@ -1153,6 +1153,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > struct tuple_element<1, pair<_Tp1, _Tp2>> > { typedef _Tp2 type; }; > > + // Forward declare the partial specialization for std::tuple > + // to work around modules bug PR c++/113710. > + template<size_t __i, typename... _Types> > + struct tuple_element<__i, tuple<_Types...>>; > + > #if __cplusplus >= 201703L > template<typename _Tp1, typename _Tp2> > inline constexpr size_t tuple_size_v<pair<_Tp1, _Tp2>> = 2; > -- > 2.43.0.561.g235986be82 > > >
On Wed, 7 Feb 2024, 19:20 Patrick Palka, <ppalka@redhat.com> wrote: > On Wed, 7 Feb 2024, Jonathan Wakely wrote: > > > > > > > On Wed, 7 Feb 2024 at 17:29, Patrick Palka <ppalka@redhat.com> wrote: > > Tested on x86_64-pc-linux-gnu, does this look OK for trunk? > > > > > > OK. > > > > Do we have a reduced testcase to track the modules bug that will still > exist in the FE? > > https://gcc.gnu.org/PR113814 Excellent, thanks! > > > > > > > > > -- >8 -- > > > > The forward declarations of std::get in <bits/stl_pair.h> added in > > r14-8710-g65b4cba9d6a9ff are causing an ICE in the test > modules/hello-1 > > due to what seems to be a declaration merging issue in modules. > > > > What seems to be happening is that in hello-1_b.C we first include > > <string_view>, which indirectly includes <bits/stl_pair.h> which > forms > > the dependent specialization tuple_element<__i, > tuple<_Elements...>> > > (appearing in some of the std::get overloads) and adds it to the > > specializations table. > > > > We then import hello which indirectly includes <tuple> (in the > GMF), > > within which we define a partial specialization of tuple_element > with > > that same template-id. So importing hello in turn streams in this > > partial specialization, but we don't notice it matches the > previously > > created dependent specialization, and we end up with two equivalent > > types for this template-id with different TYPE_CANONICAL. > > > > This patch works around this issue by adding a forward declaration > of > > the tuple_element partial specialization from <tuple> to > <bits/stl_pair.h> > > so that it appears alongside the dependent specialization of the > same > > template-id. So when including <bits/stl_pair.h> we immediately > register > > the template-id as a partial specialization, and if we later > stream in the > > partial specialization the MK_partial case of > trees_in::key_mergeable will > > match them up. (So perhaps a proper modules fix for this might be > to make > > key_mergeable try to match up a streamed in partial specialization > with an > > existing specialization from the table via > match_mergeable_specialization.) > > > > PR c++/113710 > > > > libstdc++-v3/ChangeLog: > > > > * include/bits/stl_pair.h (tuple_element): Add forward > > declaration of the partial specialization for tuple. > > --- > > libstdc++-v3/include/bits/stl_pair.h | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/libstdc++-v3/include/bits/stl_pair.h > b/libstdc++-v3/include/bits/stl_pair.h > > index 00ec53ebc33..8c71b1350e5 100644 > > --- a/libstdc++-v3/include/bits/stl_pair.h > > +++ b/libstdc++-v3/include/bits/stl_pair.h > > @@ -1153,6 +1153,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > struct tuple_element<1, pair<_Tp1, _Tp2>> > > { typedef _Tp2 type; }; > > > > + // Forward declare the partial specialization for std::tuple > > + // to work around modules bug PR c++/113710. > > + template<size_t __i, typename... _Types> > > + struct tuple_element<__i, tuple<_Types...>>; > > + > > #if __cplusplus >= 201703L > > template<typename _Tp1, typename _Tp2> > > inline constexpr size_t tuple_size_v<pair<_Tp1, _Tp2>> = 2; > > -- > > 2.43.0.561.g235986be82 > > > > > >
diff --git a/libstdc++-v3/include/bits/stl_pair.h b/libstdc++-v3/include/bits/stl_pair.h index 00ec53ebc33..8c71b1350e5 100644 --- a/libstdc++-v3/include/bits/stl_pair.h +++ b/libstdc++-v3/include/bits/stl_pair.h @@ -1153,6 +1153,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION struct tuple_element<1, pair<_Tp1, _Tp2>> { typedef _Tp2 type; }; + // Forward declare the partial specialization for std::tuple + // to work around modules bug PR c++/113710. + template<size_t __i, typename... _Types> + struct tuple_element<__i, tuple<_Types...>>; + #if __cplusplus >= 201703L template<typename _Tp1, typename _Tp2> inline constexpr size_t tuple_size_v<pair<_Tp1, _Tp2>> = 2;