| Message ID | 20231218213135.2720773-1-jason@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] c++/modules: __class_type_info and modules | expand |
On 12/18/23 16:31, Jason Merrill wrote: > Tested x86_64-pc-linux-gnu. Does this make sense? Did you have another theory > about how to merge these? Why isn't push_abi_namespace doing the right setup here? (and I think get_global_binding might be similarly problematic?) nathan > > -- 8< -- > > Doing a dynamic_cast in both TUs broke because we were declaring a new > __class_type_info in _b that conflicted with the one imported in the global > module from _a. lookup_elaborated_type has a comment that we probably don't > want to find such imports in general, but in this case it seems necessary to > make the artificial lazy declarations of RTTI types work. > > gcc/cp/ChangeLog: > > * name-lookup.cc (lookup_elaborated_type): Look for bindings > in the global namespace in the ABI namespace. > > gcc/testsuite/ChangeLog: > > * g++.dg/modules/pr106304_b.C: Add dynamic_cast. > --- > gcc/cp/name-lookup.cc | 10 ++++++++++ > gcc/testsuite/g++.dg/modules/pr106304_b.C | 1 + > 2 files changed, 11 insertions(+) > > diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc > index 09dc6ef3e5a..f15b338025d 100644 > --- a/gcc/cp/name-lookup.cc > +++ b/gcc/cp/name-lookup.cc > @@ -8092,6 +8092,16 @@ lookup_elaborated_type (tree name, TAG_how how) > // FIXME: This isn't quite right, if we find something > // here, from the language PoV we're not supposed to > // know it? > + // We at least need to do this in __cxxabiv1 to unify lazy > + // declarations of __class_type_info in build_dynamic_cast_1. > + if (current_namespace == abi_node) > + { > + tree g = (BINDING_VECTOR_CLUSTER (*slot, 0) > + .slots[BINDING_SLOT_GLOBAL]); > + for (ovl_iterator iter (g); iter; ++iter) > + if (qualify_lookup (*iter, LOOK_want::TYPE)) > + return *iter; > + } > } > } > } > diff --git a/gcc/testsuite/g++.dg/modules/pr106304_b.C b/gcc/testsuite/g++.dg/modules/pr106304_b.C > index e8333909c8d..0d1da086176 100644 > --- a/gcc/testsuite/g++.dg/modules/pr106304_b.C > +++ b/gcc/testsuite/g++.dg/modules/pr106304_b.C > @@ -5,4 +5,5 @@ module pr106304; > > void f(A& a) { > as_b(a); > + dynamic_cast<B*>(&a); > } > > base-commit: 5347263b347d02e875879ca40ca6e289ac178919 > prerequisite-patch-id: 66735c0c7beb22586ed4b632d10ec9094bb9920c
On 12/18/23 16:57, Nathan Sidwell wrote: > On 12/18/23 16:31, Jason Merrill wrote: >> Tested x86_64-pc-linux-gnu. Does this make sense? Did you have >> another theory >> about how to merge these? > > Why isn't push_abi_namespace doing the right setup here? (and I think > get_global_binding might be similarly problematic?) What would the right setup be? It pushes into the global module, but before this change lookup doesn't find things imported into the global module, and so we get two independent (and so non-equivalent) declarations. The comment for get_namespace_binding says "Users of this who, having found nothing, push a new decl must be prepared for that pushing to match an existing decl." But if lookup_elaborated_type fails, so we pushtag a new type, check_module_override doesn't try to merge them because TREE_PUBLIC isn't set on the TYPE_DECL yet at that point, and they coexist until we complain about redeclaring __dynamic_cast with non-matching parameter types. I tried setting TREE_PUBLIC on the TYPE_DECL, and then check_module_override called duplicate_decls, and rejected the redeclaration as a different type. >> -- 8< -- >> >> Doing a dynamic_cast in both TUs broke because we were declaring a new >> __class_type_info in _b that conflicted with the one imported in the >> global >> module from _a. lookup_elaborated_type has a comment that we probably >> don't >> want to find such imports in general, but in this case it seems >> necessary to >> make the artificial lazy declarations of RTTI types work. >> >> gcc/cp/ChangeLog: >> >> * name-lookup.cc (lookup_elaborated_type): Look for bindings >> in the global namespace in the ABI namespace. >> >> gcc/testsuite/ChangeLog: >> >> * g++.dg/modules/pr106304_b.C: Add dynamic_cast. >> --- >> gcc/cp/name-lookup.cc | 10 ++++++++++ >> gcc/testsuite/g++.dg/modules/pr106304_b.C | 1 + >> 2 files changed, 11 insertions(+) >> >> diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc >> index 09dc6ef3e5a..f15b338025d 100644 >> --- a/gcc/cp/name-lookup.cc >> +++ b/gcc/cp/name-lookup.cc >> @@ -8092,6 +8092,16 @@ lookup_elaborated_type (tree name, TAG_how how) >> // FIXME: This isn't quite right, if we find something >> // here, from the language PoV we're not supposed to >> // know it? >> + // We at least need to do this in __cxxabiv1 to unify lazy >> + // declarations of __class_type_info in build_dynamic_cast_1. >> + if (current_namespace == abi_node) >> + { >> + tree g = (BINDING_VECTOR_CLUSTER (*slot, 0) >> + .slots[BINDING_SLOT_GLOBAL]); >> + for (ovl_iterator iter (g); iter; ++iter) >> + if (qualify_lookup (*iter, LOOK_want::TYPE)) >> + return *iter; >> + } >> } >> } >> } >> diff --git a/gcc/testsuite/g++.dg/modules/pr106304_b.C >> b/gcc/testsuite/g++.dg/modules/pr106304_b.C >> index e8333909c8d..0d1da086176 100644 >> --- a/gcc/testsuite/g++.dg/modules/pr106304_b.C >> +++ b/gcc/testsuite/g++.dg/modules/pr106304_b.C >> @@ -5,4 +5,5 @@ module pr106304; >> void f(A& a) { >> as_b(a); >> + dynamic_cast<B*>(&a); >> } >> >> base-commit: 5347263b347d02e875879ca40ca6e289ac178919 >> prerequisite-patch-id: 66735c0c7beb22586ed4b632d10ec9094bb9920c >
On 12/18/23 17:10, Jason Merrill wrote: > On 12/18/23 16:57, Nathan Sidwell wrote: >> On 12/18/23 16:31, Jason Merrill wrote: >>> Tested x86_64-pc-linux-gnu. Does this make sense? Did you have another theory >>> about how to merge these? >> >> Why isn't push_abi_namespace doing the right setup here? (and I think >> get_global_binding might be similarly problematic?) > > What would the right setup be? It pushes into the global module, but before > this change lookup doesn't find things imported into the global module, and so > we get two independent (and so non-equivalent) declarations. > > The comment for get_namespace_binding says "Users of this who, having found > nothing, push a new decl must be prepared for that pushing to match an existing > decl." But if lookup_elaborated_type fails, so we pushtag a new type, > check_module_override doesn't try to merge them because TREE_PUBLIC isn't set on > the TYPE_DECL yet at that point, and they coexist until we complain about > redeclaring __dynamic_cast with non-matching parameter types. > > I tried setting TREE_PUBLIC on the TYPE_DECL, and then check_module_override > called duplicate_decls, and rejected the redeclaration as a different type. sigh, it seems that doesn't work as intended, I guess your approace is a pragmatic workaround, much as I dislike special-casing particular identifier. Perhaps comment with an appropriate FIXME? I've realized there's problems with completeness here -- the 'invisible' type may be complete, but the current TU only foreward-declares it. Our AST can't represent that right now. And I'm not sure if there are template instantiation issues -- is the type complete or not in any particular instantiaton? nathan > >>> -- 8< -- >>> >>> Doing a dynamic_cast in both TUs broke because we were declaring a new >>> __class_type_info in _b that conflicted with the one imported in the global >>> module from _a. lookup_elaborated_type has a comment that we probably don't >>> want to find such imports in general, but in this case it seems necessary to >>> make the artificial lazy declarations of RTTI types work. >>> >>> gcc/cp/ChangeLog: >>> >>> * name-lookup.cc (lookup_elaborated_type): Look for bindings >>> in the global namespace in the ABI namespace. >>> >>> gcc/testsuite/ChangeLog: >>> >>> * g++.dg/modules/pr106304_b.C: Add dynamic_cast. >>> --- >>> gcc/cp/name-lookup.cc | 10 ++++++++++ >>> gcc/testsuite/g++.dg/modules/pr106304_b.C | 1 + >>> 2 files changed, 11 insertions(+) >>> >>> diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc >>> index 09dc6ef3e5a..f15b338025d 100644 >>> --- a/gcc/cp/name-lookup.cc >>> +++ b/gcc/cp/name-lookup.cc >>> @@ -8092,6 +8092,16 @@ lookup_elaborated_type (tree name, TAG_how how) >>> // FIXME: This isn't quite right, if we find something >>> // here, from the language PoV we're not supposed to >>> // know it? >>> + // We at least need to do this in __cxxabiv1 to unify lazy >>> + // declarations of __class_type_info in build_dynamic_cast_1. >>> + if (current_namespace == abi_node) >>> + { >>> + tree g = (BINDING_VECTOR_CLUSTER (*slot, 0) >>> + .slots[BINDING_SLOT_GLOBAL]); >>> + for (ovl_iterator iter (g); iter; ++iter) >>> + if (qualify_lookup (*iter, LOOK_want::TYPE)) >>> + return *iter; >>> + } >>> } >>> } >>> } >>> diff --git a/gcc/testsuite/g++.dg/modules/pr106304_b.C >>> b/gcc/testsuite/g++.dg/modules/pr106304_b.C >>> index e8333909c8d..0d1da086176 100644 >>> --- a/gcc/testsuite/g++.dg/modules/pr106304_b.C >>> +++ b/gcc/testsuite/g++.dg/modules/pr106304_b.C >>> @@ -5,4 +5,5 @@ module pr106304; >>> void f(A& a) { >>> as_b(a); >>> + dynamic_cast<B*>(&a); >>> } >>> >>> base-commit: 5347263b347d02e875879ca40ca6e289ac178919 >>> prerequisite-patch-id: 66735c0c7beb22586ed4b632d10ec9094bb9920c >> >
On 12/23/23 14:46, Nathan Sidwell wrote: > On 12/18/23 17:10, Jason Merrill wrote: >> On 12/18/23 16:57, Nathan Sidwell wrote: >>> On 12/18/23 16:31, Jason Merrill wrote: >>>> Tested x86_64-pc-linux-gnu. Does this make sense? Did you have >>>> another theory >>>> about how to merge these? >>> >>> Why isn't push_abi_namespace doing the right setup here? (and I think >>> get_global_binding might be similarly problematic?) >> >> What would the right setup be? It pushes into the global module, but >> before this change lookup doesn't find things imported into the global >> module, and so we get two independent (and so non-equivalent) >> declarations. >> >> The comment for get_namespace_binding says "Users of this who, having >> found nothing, push a new decl must be prepared for that pushing to >> match an existing decl." But if lookup_elaborated_type fails, so we >> pushtag a new type, check_module_override doesn't try to merge them >> because TREE_PUBLIC isn't set on the TYPE_DECL yet at that point, and >> they coexist until we complain about redeclaring __dynamic_cast with >> non-matching parameter types. >> >> I tried setting TREE_PUBLIC on the TYPE_DECL, and then >> check_module_override called duplicate_decls, and rejected the >> redeclaration as a different type. > > sigh, it seems that doesn't work as intended, I guess your approace is a > pragmatic workaround, much as I dislike special-casing particular > identifier. Perhaps comment with an appropriate FIXME? > > I've realized there's problems with completeness here -- the 'invisible' > type may be complete, but the current TU only forward-declares it. Our > AST can't represent that right now. And I'm not sure if there are > template instantiation issues -- is the type complete or not in any > particular instantiaton? My understanding of https://eel.is/c++draft/module#reach-4 is that this doesn't matter: if there is a reachable definition of the class, the class is complete, even if the current TU only forward-declares it. Here's an alternate approach that handles this merging in check_module_override; this makes P1811 include-after-import a bit worse, but it's already not well supported, so perhaps that's OK for now. But I'm inclined to go with my earlier patch for GCC 14. What do you think? Jason
On 1/12/24 09:09, Jason Merrill wrote: > On 12/23/23 14:46, Nathan Sidwell wrote: >> On 12/18/23 17:10, Jason Merrill wrote: >>> On 12/18/23 16:57, Nathan Sidwell wrote: >>>> On 12/18/23 16:31, Jason Merrill wrote: >>>>> Tested x86_64-pc-linux-gnu. Does this make sense? Did you have >>>>> another theory >>>>> about how to merge these? >>>> >>>> Why isn't push_abi_namespace doing the right setup here? (and I >>>> think get_global_binding might be similarly problematic?) >>> >>> What would the right setup be? It pushes into the global module, but >>> before this change lookup doesn't find things imported into the >>> global module, and so we get two independent (and so non-equivalent) >>> declarations. >>> >>> The comment for get_namespace_binding says "Users of this who, having >>> found nothing, push a new decl must be prepared for that pushing to >>> match an existing decl." But if lookup_elaborated_type fails, so we >>> pushtag a new type, check_module_override doesn't try to merge them >>> because TREE_PUBLIC isn't set on the TYPE_DECL yet at that point, and >>> they coexist until we complain about redeclaring __dynamic_cast with >>> non-matching parameter types. >>> >>> I tried setting TREE_PUBLIC on the TYPE_DECL, and then >>> check_module_override called duplicate_decls, and rejected the >>> redeclaration as a different type. >> >> sigh, it seems that doesn't work as intended, I guess your approace is >> a pragmatic workaround, much as I dislike special-casing particular >> identifier. Perhaps comment with an appropriate FIXME? >> >> I've realized there's problems with completeness here -- the >> 'invisible' type may be complete, but the current TU only >> forward-declares it. Our AST can't represent that right now. And I'm >> not sure if there are template instantiation issues -- is the type >> complete or not in any particular instantiaton? > > My understanding of https://eel.is/c++draft/module#reach-4 is that this > doesn't matter: if there is a reachable definition of the class, the > class is complete, even if the current TU only forward-declares it. > > Here's an alternate approach that handles this merging in > check_module_override; this makes P1811 include-after-import a bit > worse, but it's already not well supported, so perhaps that's OK for > now. But I'm inclined to go with my earlier patch for GCC 14. What do > you think? I'm going to go ahead and push this revision of my earlier patch for now, we can adjust as needed.
diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc index 09dc6ef3e5a..f15b338025d 100644 --- a/gcc/cp/name-lookup.cc +++ b/gcc/cp/name-lookup.cc @@ -8092,6 +8092,16 @@ lookup_elaborated_type (tree name, TAG_how how) // FIXME: This isn't quite right, if we find something // here, from the language PoV we're not supposed to // know it? + // We at least need to do this in __cxxabiv1 to unify lazy + // declarations of __class_type_info in build_dynamic_cast_1. + if (current_namespace == abi_node) + { + tree g = (BINDING_VECTOR_CLUSTER (*slot, 0) + .slots[BINDING_SLOT_GLOBAL]); + for (ovl_iterator iter (g); iter; ++iter) + if (qualify_lookup (*iter, LOOK_want::TYPE)) + return *iter; + } } } } diff --git a/gcc/testsuite/g++.dg/modules/pr106304_b.C b/gcc/testsuite/g++.dg/modules/pr106304_b.C index e8333909c8d..0d1da086176 100644 --- a/gcc/testsuite/g++.dg/modules/pr106304_b.C +++ b/gcc/testsuite/g++.dg/modules/pr106304_b.C @@ -5,4 +5,5 @@ module pr106304; void f(A& a) { as_b(a); + dynamic_cast<B*>(&a); }