Re: [PATCH v2 2/2] c++/modules: Fix instantiation of imported temploid friends [PR114275]
On Wed, Apr 17, 2024 at 02:02:21PM -0400, Patrick Palka wrote: > On Mon, 15 Apr 2024, Nathaniel Shead wrote: > > > I'm not a huge fan of always streaming 'imported_temploid_friends' for > > all decls, but I don't think it adds much performance cost over adding a > > new flag to categorise decls that might be marked as such. > > IIUC this value is going to be almost always null which is encoded as a > single 0 byte, which should be fast to stream. But I wonder how much > larger gets? Can we get away with streaming this value > only for TEMPLATE_DECLs? Yes, it should either just be a 0 byte or an additional backref somewhere, which will likely also be small. On my system it increases the size by 0.26%, from 31186800 bytes to 31268672. But I've just found that this patch has a bug anyway, in that it doesn't correctly dedup if the friend types are instantiated in two separate modules that are then both imported. I'll see what I need to do to fix this which may influence what we need to stream here. > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? > > > > -- >8 -- > > > > This patch fixes a number of issues with the handling of temploid friend > > declarations. > > > > The primary issue is that instantiations of friend declarations should > > attach the declaration to the same module as the befriending class, by > > [module.unit] p7.1 and [temp.friend] p2; this could be a different > > module from the current TU, and so needs special handling. > > Nice, your approach seems consistent with Nathan's comments in the past > about this issue: > > https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603288.html > https://gcc.gnu.org/pipermail/gcc-patches/2023-February/611215.htmlw > > > > > The other main issue here is that we can't assume that just because name > > lookup didn't find a definition for a hidden template class, it doesn't > > mean that it doesn't exist: it could be a non-exported entity that we've > > nevertheless streamed in from an imported module. We need to ensure > > that when instantiating friend classes that we return the same TYPE_DECL > > that we got from our imports, otherwise we will get later issues with > > 'duplicate_decls' (rightfully) complaining that they're different. > > > > This doesn't appear necessary for functions due to the existing name > > lookup handling already finding these hidden declarations. > > It does seem like a weird inconsistency that tsubst_friend_class needs > this workaround but not tsubst_friend_function. > > I wonder if we can relax duplicate_decls to treat an instantiated > template friend class as a redeclaration instead of complaining, > mirroring its behavior for functions, which in turn would let us get rid > of the name lookup in tsubst_friend_class and eliminate the need for > lookup_imported_hidden_friend? This may be too speculative/risky of > a refactoring at this stage though, and your approach has the nice > advantage of changing only modules code paths. Hm, that's a good idea. I've played around a little bit with trying this but I've gotten a little stuck, might try again later. It also feels a little silly to do a full instantiation of a (potentially) large type only to immediately throw it away when we could just look for it beforehand, but this does feel like a neater solution anyway. > In any case I hope we can fix this issue for GCC 14! LGTM overall. > > > > > PR c++/105320 > > PR c++/114275 > > > > gcc/cp/ChangeLog: > > > > * cp-tree.h (propagate_defining_module): Declare. > > (lookup_imported_hidden_friend): Declare. > > * decl.cc (duplicate_decls): Also check if hidden declarations > > can be redeclared in this module. > > * module.cc (imported_temploid_friends): New map. > > (init_modules): Initialize it. > > (trees_out::decl_value): Write it. > > (trees_in::decl_value): Read it. > > (get_originating_module_decl): Follow the owning decl for an > > imported temploid friend. > > (propagate_defining_module): New function. > > * name-lookup.cc (lookup_imported_hidden_friend): New function. > > * pt.cc (tsubst_friend_function): Propagate defining module for > > new friend functions. > > (tsubst_friend_class): Lookup imported hidden friends. Check > > for valid redeclaration. Propagate defining module for new > > friend classes. > > > > gcc/testsuite/ChangeLog: > > > > * g++.dg/modules/tpl-friend-10_a.C: New test. > > * g++.dg/modules/tpl-friend-10_b.C: New test. > > * g++.dg/modules/tpl-friend-10_c.C: New test. > > * g++.dg/modules/tpl-friend-11_a.C: New test. > > * g++.dg/modules/tpl-friend-11_b.C: New test. > > * g++.dg/modules/tpl-friend-12_a.C: New test. > > * g++.dg/modules/tpl-friend-12_b.C: New test. > > * g++.dg/modules/tpl-friend-12_c.C: New test. > > * g++.dg/modules/tpl-friend-12_d.C: New test. > > * g++.dg/modules/tpl-friend-12_e.C: New test. > > * g++.dg/m
Re: [PATCH v2 2/2] c++/modules: Fix instantiation of imported temploid friends [PR114275]
On Mon, 15 Apr 2024, Nathaniel Shead wrote: > I'm not a huge fan of always streaming 'imported_temploid_friends' for > all decls, but I don't think it adds much performance cost over adding a > new flag to categorise decls that might be marked as such. IIUC this value is going to be almost always null which is encoded as a single 0 byte, which should be fast to stream. But I wonder how much larger gets? Can we get away with streaming this value only for TEMPLATE_DECLs? > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? > > -- >8 -- > > This patch fixes a number of issues with the handling of temploid friend > declarations. > > The primary issue is that instantiations of friend declarations should > attach the declaration to the same module as the befriending class, by > [module.unit] p7.1 and [temp.friend] p2; this could be a different > module from the current TU, and so needs special handling. Nice, your approach seems consistent with Nathan's comments in the past about this issue: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603288.html https://gcc.gnu.org/pipermail/gcc-patches/2023-February/611215.htmlw > > The other main issue here is that we can't assume that just because name > lookup didn't find a definition for a hidden template class, it doesn't > mean that it doesn't exist: it could be a non-exported entity that we've > nevertheless streamed in from an imported module. We need to ensure > that when instantiating friend classes that we return the same TYPE_DECL > that we got from our imports, otherwise we will get later issues with > 'duplicate_decls' (rightfully) complaining that they're different. > > This doesn't appear necessary for functions due to the existing name > lookup handling already finding these hidden declarations. It does seem like a weird inconsistency that tsubst_friend_class needs this workaround but not tsubst_friend_function. I wonder if we can relax duplicate_decls to treat an instantiated template friend class as a redeclaration instead of complaining, mirroring its behavior for functions, which in turn would let us get rid of the name lookup in tsubst_friend_class and eliminate the need for lookup_imported_hidden_friend? This may be too speculative/risky of a refactoring at this stage though, and your approach has the nice advantage of changing only modules code paths. In any case I hope we can fix this issue for GCC 14! LGTM overall. > > PR c++/105320 > PR c++/114275 > > gcc/cp/ChangeLog: > > * cp-tree.h (propagate_defining_module): Declare. > (lookup_imported_hidden_friend): Declare. > * decl.cc (duplicate_decls): Also check if hidden declarations > can be redeclared in this module. > * module.cc (imported_temploid_friends): New map. > (init_modules): Initialize it. > (trees_out::decl_value): Write it. > (trees_in::decl_value): Read it. > (get_originating_module_decl): Follow the owning decl for an > imported temploid friend. > (propagate_defining_module): New function. > * name-lookup.cc (lookup_imported_hidden_friend): New function. > * pt.cc (tsubst_friend_function): Propagate defining module for > new friend functions. > (tsubst_friend_class): Lookup imported hidden friends. Check > for valid redeclaration. Propagate defining module for new > friend classes. > > gcc/testsuite/ChangeLog: > > * g++.dg/modules/tpl-friend-10_a.C: New test. > * g++.dg/modules/tpl-friend-10_b.C: New test. > * g++.dg/modules/tpl-friend-10_c.C: New test. > * g++.dg/modules/tpl-friend-11_a.C: New test. > * g++.dg/modules/tpl-friend-11_b.C: New test. > * g++.dg/modules/tpl-friend-12_a.C: New test. > * g++.dg/modules/tpl-friend-12_b.C: New test. > * g++.dg/modules/tpl-friend-12_c.C: New test. > * g++.dg/modules/tpl-friend-12_d.C: New test. > * g++.dg/modules/tpl-friend-12_e.C: New test. > * g++.dg/modules/tpl-friend-12_f.C: New test. > * g++.dg/modules/tpl-friend-13_a.C: New test. > * g++.dg/modules/tpl-friend-13_b.C: New test. > * g++.dg/modules/tpl-friend-13_c.C: New test. > * g++.dg/modules/tpl-friend-13_d.C: New test. > * g++.dg/modules/tpl-friend-13_e.C: New test. > * g++.dg/modules/tpl-friend-9.C: New test. > > Signed-off-by: Nathaniel Shead > --- > gcc/cp/cp-tree.h | 2 + > gcc/cp/decl.cc| 36 +++-- > gcc/cp/module.cc | 52 +++ > gcc/cp/name-lookup.cc | 42 +++ > gcc/cp/pt.cc | 19 +++ > .../g++.dg/modules/tpl-friend-10_a.C | 15 ++ > .../g++.dg/modules/tpl-friend-10_b.C | 5 ++ > .../g++.dg/modules/tpl-friend-10_c.C | 7 +++ > .../g++.dg/modules/tpl-friend-11_a.C | 14 + > .../g++.dg/mod
[PATCH v2 2/2] c++/modules: Fix instantiation of imported temploid friends [PR114275]
I'm not a huge fan of always streaming 'imported_temploid_friends' for all decls, but I don't think it adds much performance cost over adding a new flag to categorise decls that might be marked as such. Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk? -- >8 -- This patch fixes a number of issues with the handling of temploid friend declarations. The primary issue is that instantiations of friend declarations should attach the declaration to the same module as the befriending class, by [module.unit] p7.1 and [temp.friend] p2; this could be a different module from the current TU, and so needs special handling. The other main issue here is that we can't assume that just because name lookup didn't find a definition for a hidden template class, it doesn't mean that it doesn't exist: it could be a non-exported entity that we've nevertheless streamed in from an imported module. We need to ensure that when instantiating friend classes that we return the same TYPE_DECL that we got from our imports, otherwise we will get later issues with 'duplicate_decls' (rightfully) complaining that they're different. This doesn't appear necessary for functions due to the existing name lookup handling already finding these hidden declarations. PR c++/105320 PR c++/114275 gcc/cp/ChangeLog: * cp-tree.h (propagate_defining_module): Declare. (lookup_imported_hidden_friend): Declare. * decl.cc (duplicate_decls): Also check if hidden declarations can be redeclared in this module. * module.cc (imported_temploid_friends): New map. (init_modules): Initialize it. (trees_out::decl_value): Write it. (trees_in::decl_value): Read it. (get_originating_module_decl): Follow the owning decl for an imported temploid friend. (propagate_defining_module): New function. * name-lookup.cc (lookup_imported_hidden_friend): New function. * pt.cc (tsubst_friend_function): Propagate defining module for new friend functions. (tsubst_friend_class): Lookup imported hidden friends. Check for valid redeclaration. Propagate defining module for new friend classes. gcc/testsuite/ChangeLog: * g++.dg/modules/tpl-friend-10_a.C: New test. * g++.dg/modules/tpl-friend-10_b.C: New test. * g++.dg/modules/tpl-friend-10_c.C: New test. * g++.dg/modules/tpl-friend-11_a.C: New test. * g++.dg/modules/tpl-friend-11_b.C: New test. * g++.dg/modules/tpl-friend-12_a.C: New test. * g++.dg/modules/tpl-friend-12_b.C: New test. * g++.dg/modules/tpl-friend-12_c.C: New test. * g++.dg/modules/tpl-friend-12_d.C: New test. * g++.dg/modules/tpl-friend-12_e.C: New test. * g++.dg/modules/tpl-friend-12_f.C: New test. * g++.dg/modules/tpl-friend-13_a.C: New test. * g++.dg/modules/tpl-friend-13_b.C: New test. * g++.dg/modules/tpl-friend-13_c.C: New test. * g++.dg/modules/tpl-friend-13_d.C: New test. * g++.dg/modules/tpl-friend-13_e.C: New test. * g++.dg/modules/tpl-friend-9.C: New test. Signed-off-by: Nathaniel Shead --- gcc/cp/cp-tree.h | 2 + gcc/cp/decl.cc| 36 +++-- gcc/cp/module.cc | 52 +++ gcc/cp/name-lookup.cc | 42 +++ gcc/cp/pt.cc | 19 +++ .../g++.dg/modules/tpl-friend-10_a.C | 15 ++ .../g++.dg/modules/tpl-friend-10_b.C | 5 ++ .../g++.dg/modules/tpl-friend-10_c.C | 7 +++ .../g++.dg/modules/tpl-friend-11_a.C | 14 + .../g++.dg/modules/tpl-friend-11_b.C | 5 ++ .../g++.dg/modules/tpl-friend-12_a.C | 10 .../g++.dg/modules/tpl-friend-12_b.C | 9 .../g++.dg/modules/tpl-friend-12_c.C | 10 .../g++.dg/modules/tpl-friend-12_d.C | 8 +++ .../g++.dg/modules/tpl-friend-12_e.C | 7 +++ .../g++.dg/modules/tpl-friend-12_f.C | 8 +++ .../g++.dg/modules/tpl-friend-13_a.C | 12 + .../g++.dg/modules/tpl-friend-13_b.C | 9 .../g++.dg/modules/tpl-friend-13_c.C | 11 .../g++.dg/modules/tpl-friend-13_d.C | 7 +++ .../g++.dg/modules/tpl-friend-13_e.C | 14 + gcc/testsuite/g++.dg/modules/tpl-friend-9.C | 13 + 22 files changed, 299 insertions(+), 16 deletions(-) create mode 100644 gcc/testsuite/g++.dg/modules/tpl-friend-10_a.C create mode 100644 gcc/testsuite/g++.dg/modules/tpl-friend-10_b.C create mode 100644 gcc/testsuite/g++.dg/modules/tpl-friend-10_c.C create mode 100644 gcc/testsuite/g++.dg/modules/tpl-friend-11_a.C create mode 100644 gcc/testsuite/g++.dg/modules/tpl-friend-11_b.C create mode 100644 gcc/testsuite/g++.dg/modules/tpl-friend-12_a.C create mode 100644 gcc/testsuite/g++.dg/modules/tpl-friend-
