Re: [PATCH][14 backport] c++: Fix instantiation of imported temploid friends [PR114275]
On 5/24/24 10:40, Iain Sandoe wrote:
On 24 May 2024, at 14:54, Jason Merrill wrote:
On 5/24/24 04:06, Nathaniel Shead wrote:
On Thu, May 23, 2024 at 06:41:06PM -0400, Jason Merrill wrote:
On 5/13/24 07:56, Nathaniel Shead wrote:
@@ -11751,9 +11767,16 @@ tsubst_friend_class (tree friend_tmpl, tree args)
if (tmpl != error_mark_node)
{
/* The new TMPL is not an instantiation of anything, so we
-forget its origins. We don't reset CLASSTYPE_TI_TEMPLATE
+forget its origins. It is also not a specialization of
+anything. We don't reset CLASSTYPE_TI_TEMPLATE
for the new type because that is supposed to be the
corresponding template decl, i.e., TMPL. */
+ spec_entry elt;
+ elt.tmpl = friend_tmpl;
+ elt.args = CLASSTYPE_TI_ARGS (TREE_TYPE (tmpl));
+ elt.spec = TREE_TYPE (tmpl);
+ type_specializations->remove_elt ();
For GCC 14.2 let's guard this with if (modules_p ()); for GCC 15 it can be
unconditional. OK.
I'm looking to backport this patch to GCC 14 now that it's been on trunk
some time. Here's the patch I'm aiming to add (squashed with the
changes from r15-220-gec2365e07537e8) after cherrypicking the
prerequisite commit r15-58-g2faf040335f9b4; is this OK?
Or should I keep it as two separate commits to make the cherrypicking
more obvious? Not entirely sure on the etiquette around this.
It's OK to squash them, but it's typical to use -x (directly or via git
gcc-backport) to mention where a branch change was cherry-picked from, and
in this case it would make sense to edit in the second commit so it's clear
the backport includes both. OK that way.
Sorry, still a bit confused :) Do you mean to merge the two commits
together such that there are two "cherry picked from commit ..."s in the
commit message? Or just list second commit, and mention that it
includes both in the commit message?
I was thinking
(cherry picked from commit a and b)
For the record, I do not think the git hooks with allow that exactly (or, at
least, if they
do I did not find the right syntax); what I’ve done in similar cases is to
keep the main
“cherry picked from” line for the main patch and then add text to the intro
section
saying that and are also included.
Looks like the git_commit.py regex "cherry picked from commit
(?P\w+)" will ignore everything after the first commit-id, so it
should be fine. You just can't pluralize "commit" to "commits". :)
Jason
Re: [PATCH][14 backport] c++: Fix instantiation of imported temploid friends [PR114275]
> On 24 May 2024, at 14:54, Jason Merrill wrote:
>
> On 5/24/24 04:06, Nathaniel Shead wrote:
>> On Thu, May 23, 2024 at 06:41:06PM -0400, Jason Merrill wrote:
>>> On 5/13/24 07:56, Nathaniel Shead wrote:
>> @@ -11751,9 +11767,16 @@ tsubst_friend_class (tree friend_tmpl, tree
>> args)
>> if (tmpl != error_mark_node)
>> {
>>/* The new TMPL is not an instantiation of anything, so we
>> - forget its origins. We don't reset CLASSTYPE_TI_TEMPLATE
>> + forget its origins. It is also not a specialization of
>> + anything. We don't reset CLASSTYPE_TI_TEMPLATE
>> for the new type because that is supposed to be the
>> corresponding template decl, i.e., TMPL. */
>> + spec_entry elt;
>> + elt.tmpl = friend_tmpl;
>> + elt.args = CLASSTYPE_TI_ARGS (TREE_TYPE (tmpl));
>> + elt.spec = TREE_TYPE (tmpl);
>> + type_specializations->remove_elt ();
>
> For GCC 14.2 let's guard this with if (modules_p ()); for GCC 15 it can be
> unconditional. OK.
I'm looking to backport this patch to GCC 14 now that it's been on trunk
some time. Here's the patch I'm aiming to add (squashed with the
changes from r15-220-gec2365e07537e8) after cherrypicking the
prerequisite commit r15-58-g2faf040335f9b4; is this OK?
Or should I keep it as two separate commits to make the cherrypicking
more obvious? Not entirely sure on the etiquette around this.
>>>
>>> It's OK to squash them, but it's typical to use -x (directly or via git
>>> gcc-backport) to mention where a branch change was cherry-picked from, and
>>> in this case it would make sense to edit in the second commit so it's clear
>>> the backport includes both. OK that way.
>> Sorry, still a bit confused :) Do you mean to merge the two commits
>> together such that there are two "cherry picked from commit ..."s in the
>> commit message? Or just list second commit, and mention that it
>> includes both in the commit message?
>
> I was thinking
>
> (cherry picked from commit a and b)
For the record, I do not think the git hooks with allow that exactly (or, at
least, if they
do I did not find the right syntax); what I’ve done in similar cases is to
keep the main
“cherry picked from” line for the main patch and then add text to the intro
section
saying that and are also included.
Iain
>
> but the exact format doesn't matter, just looking for a mention of both
> commits.
>
> Jason
Re: [PATCH][14 backport] c++: Fix instantiation of imported temploid friends [PR114275]
On Fri, May 24, 2024 at 09:54:31AM -0400, Jason Merrill wrote:
> On 5/24/24 04:06, Nathaniel Shead wrote:
> > On Thu, May 23, 2024 at 06:41:06PM -0400, Jason Merrill wrote:
> > > On 5/13/24 07:56, Nathaniel Shead wrote:
> > > > > > @@ -11751,9 +11767,16 @@ tsubst_friend_class (tree friend_tmpl,
> > > > > > tree args)
> > > > > > if (tmpl != error_mark_node)
> > > > > > {
> > > > > > /* The new TMPL is not an instantiation of anything,
> > > > > > so we
> > > > > > -forget its origins. We don't reset CLASSTYPE_TI_TEMPLATE
> > > > > > +forget its origins. It is also not a specialization of
> > > > > > +anything. We don't reset CLASSTYPE_TI_TEMPLATE
> > > > > > for the new type because that is supposed to be the
> > > > > > corresponding template decl, i.e., TMPL. */
> > > > > > + spec_entry elt;
> > > > > > + elt.tmpl = friend_tmpl;
> > > > > > + elt.args = CLASSTYPE_TI_ARGS (TREE_TYPE (tmpl));
> > > > > > + elt.spec = TREE_TYPE (tmpl);
> > > > > > + type_specializations->remove_elt ();
> > > > >
> > > > > For GCC 14.2 let's guard this with if (modules_p ()); for GCC 15 it
> > > > > can be
> > > > > unconditional. OK.
> > > >
> > > > I'm looking to backport this patch to GCC 14 now that it's been on trunk
> > > > some time. Here's the patch I'm aiming to add (squashed with the
> > > > changes from r15-220-gec2365e07537e8) after cherrypicking the
> > > > prerequisite commit r15-58-g2faf040335f9b4; is this OK?
> > > >
> > > > Or should I keep it as two separate commits to make the cherrypicking
> > > > more obvious? Not entirely sure on the etiquette around this.
> > >
> > > It's OK to squash them, but it's typical to use -x (directly or via git
> > > gcc-backport) to mention where a branch change was cherry-picked from, and
> > > in this case it would make sense to edit in the second commit so it's
> > > clear
> > > the backport includes both. OK that way.
> >
> > Sorry, still a bit confused :) Do you mean to merge the two commits
> > together such that there are two "cherry picked from commit ..."s in the
> > commit message? Or just list second commit, and mention that it
> > includes both in the commit message?
>
> I was thinking
>
> (cherry picked from commit a and b)
>
> but the exact format doesn't matter, just looking for a mention of both
> commits.
>
> Jason
>
Thanks, pushed as r14-10240-gfd6fd88b1a93f4fb38f095688255ab5c00122810.
Re: [PATCH][14 backport] c++: Fix instantiation of imported temploid friends [PR114275]
On 5/24/24 04:06, Nathaniel Shead wrote:
On Thu, May 23, 2024 at 06:41:06PM -0400, Jason Merrill wrote:
On 5/13/24 07:56, Nathaniel Shead wrote:
@@ -11751,9 +11767,16 @@ tsubst_friend_class (tree friend_tmpl, tree args)
if (tmpl != error_mark_node)
{
/* The new TMPL is not an instantiation of anything, so we
-forget its origins. We don't reset CLASSTYPE_TI_TEMPLATE
+forget its origins. It is also not a specialization of
+anything. We don't reset CLASSTYPE_TI_TEMPLATE
for the new type because that is supposed to be the
corresponding template decl, i.e., TMPL. */
+ spec_entry elt;
+ elt.tmpl = friend_tmpl;
+ elt.args = CLASSTYPE_TI_ARGS (TREE_TYPE (tmpl));
+ elt.spec = TREE_TYPE (tmpl);
+ type_specializations->remove_elt ();
For GCC 14.2 let's guard this with if (modules_p ()); for GCC 15 it can be
unconditional. OK.
I'm looking to backport this patch to GCC 14 now that it's been on trunk
some time. Here's the patch I'm aiming to add (squashed with the
changes from r15-220-gec2365e07537e8) after cherrypicking the
prerequisite commit r15-58-g2faf040335f9b4; is this OK?
Or should I keep it as two separate commits to make the cherrypicking
more obvious? Not entirely sure on the etiquette around this.
It's OK to squash them, but it's typical to use -x (directly or via git
gcc-backport) to mention where a branch change was cherry-picked from, and
in this case it would make sense to edit in the second commit so it's clear
the backport includes both. OK that way.
Sorry, still a bit confused :) Do you mean to merge the two commits
together such that there are two "cherry picked from commit ..."s in the
commit message? Or just list second commit, and mention that it
includes both in the commit message?
I was thinking
(cherry picked from commit a and b)
but the exact format doesn't matter, just looking for a mention of both
commits.
Jason
Re: [PATCH][14 backport] c++: Fix instantiation of imported temploid friends [PR114275]
On Thu, May 23, 2024 at 06:41:06PM -0400, Jason Merrill wrote:
> On 5/13/24 07:56, Nathaniel Shead wrote:
> > > > @@ -11751,9 +11767,16 @@ tsubst_friend_class (tree friend_tmpl, tree
> > > > args)
> > > > if (tmpl != error_mark_node)
> > > > {
> > > > /* The new TMPL is not an instantiation of anything, so we
> > > > -forget its origins. We don't reset CLASSTYPE_TI_TEMPLATE
> > > > +forget its origins. It is also not a specialization of
> > > > +anything. We don't reset CLASSTYPE_TI_TEMPLATE
> > > > for the new type because that is supposed to be the
> > > > corresponding template decl, i.e., TMPL. */
> > > > + spec_entry elt;
> > > > + elt.tmpl = friend_tmpl;
> > > > + elt.args = CLASSTYPE_TI_ARGS (TREE_TYPE (tmpl));
> > > > + elt.spec = TREE_TYPE (tmpl);
> > > > + type_specializations->remove_elt ();
> > >
> > > For GCC 14.2 let's guard this with if (modules_p ()); for GCC 15 it can be
> > > unconditional. OK.
> >
> > I'm looking to backport this patch to GCC 14 now that it's been on trunk
> > some time. Here's the patch I'm aiming to add (squashed with the
> > changes from r15-220-gec2365e07537e8) after cherrypicking the
> > prerequisite commit r15-58-g2faf040335f9b4; is this OK?
> >
> > Or should I keep it as two separate commits to make the cherrypicking
> > more obvious? Not entirely sure on the etiquette around this.
>
> It's OK to squash them, but it's typical to use -x (directly or via git
> gcc-backport) to mention where a branch change was cherry-picked from, and
> in this case it would make sense to edit in the second commit so it's clear
> the backport includes both. OK that way.
>
> Jason
>
Sorry, still a bit confused :) Do you mean to merge the two commits
together such that there are two "cherry picked from commit ..."s in the
commit message? Or just list second commit, and mention that it
includes both in the commit message?
Nathaniel
Re: [PATCH][14 backport] c++: Fix instantiation of imported temploid friends [PR114275]
On 5/13/24 07:56, Nathaniel Shead wrote:
@@ -11751,9 +11767,16 @@ tsubst_friend_class (tree friend_tmpl, tree args)
if (tmpl != error_mark_node)
{
/* The new TMPL is not an instantiation of anything, so we
-forget its origins. We don't reset CLASSTYPE_TI_TEMPLATE
+forget its origins. It is also not a specialization of
+anything. We don't reset CLASSTYPE_TI_TEMPLATE
for the new type because that is supposed to be the
corresponding template decl, i.e., TMPL. */
+ spec_entry elt;
+ elt.tmpl = friend_tmpl;
+ elt.args = CLASSTYPE_TI_ARGS (TREE_TYPE (tmpl));
+ elt.spec = TREE_TYPE (tmpl);
+ type_specializations->remove_elt ();
For GCC 14.2 let's guard this with if (modules_p ()); for GCC 15 it can be
unconditional. OK.
I'm looking to backport this patch to GCC 14 now that it's been on trunk
some time. Here's the patch I'm aiming to add (squashed with the
changes from r15-220-gec2365e07537e8) after cherrypicking the
prerequisite commit r15-58-g2faf040335f9b4; is this OK?
Or should I keep it as two separate commits to make the cherrypicking
more obvious? Not entirely sure on the etiquette around this.
It's OK to squash them, but it's typical to use -x (directly or via git
gcc-backport) to mention where a branch change was cherry-picked from,
and in this case it would make sense to edit in the second commit so
it's clear the backport includes both. OK that way.
Jason
Re: [PATCH][14 backport] c++: Fix instantiation of imported temploid friends [PR114275]
On Mon, 13 May 2024, Nathaniel Shead wrote:
> > > @@ -11751,9 +11767,16 @@ tsubst_friend_class (tree friend_tmpl, tree args)
> > > if (tmpl != error_mark_node)
> > > {
> > > /* The new TMPL is not an instantiation of anything, so we
> > > - forget its origins. We don't reset CLASSTYPE_TI_TEMPLATE
> > > + forget its origins. It is also not a specialization of
> > > + anything. We don't reset CLASSTYPE_TI_TEMPLATE
> > >for the new type because that is supposed to be the
> > >corresponding template decl, i.e., TMPL. */
> > > + spec_entry elt;
> > > + elt.tmpl = friend_tmpl;
> > > + elt.args = CLASSTYPE_TI_ARGS (TREE_TYPE (tmpl));
> > > + elt.spec = TREE_TYPE (tmpl);
> > > + type_specializations->remove_elt ();
> >
> > For GCC 14.2 let's guard this with if (modules_p ()); for GCC 15 it can be
> > unconditional. OK.
> >
> > Jason
> >
>
> I'm looking to backport this patch to GCC 14 now that it's been on trunk
> some time. Here's the patch I'm aiming to add (squashed with the
> changes from r15-220-gec2365e07537e8) after cherrypicking the
> prerequisite commit r15-58-g2faf040335f9b4; is this OK?
>
> Or should I keep it as two separate commits to make the cherrypicking
> more obvious? Not entirely sure on the etiquette around this.
Since the first patch "only" causes sporadic testsuite failures (and
doesn't e.g. break bootstrap or anything serious like that), I reckon
it'd be fine to keep them as separate commits? Not sure either.
>
> Bootstrapped and regtested on x86_64-pc-linux-gnu on top of the
> releases/gcc-14 branch.
>
> -- >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 class template, that it
> doesn't exist at all: it could be a non-exported entity that we've
> nevertheless streamed in from an imported module. We need to ensure
> that when instantiating template friend classes that we return the same
> TEMPLATE_DECL that we got from our imports, otherwise we will get later
> issues with 'duplicate_decls' (rightfully) complaining that they're
> different when trying to merge.
>
> This doesn't appear necessary for function templates 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.
> (remove_defining_module): Declare.
> (lookup_imported_hidden_friend): Declare.
> * decl.cc (duplicate_decls): Also check if hidden decls can be
> redeclared in this module. Call remove_defining_module on
> to-be-freed newdecl.
> * module.cc (imported_temploid_friends): New.
> (init_modules): Initialize it.
> (trees_out::decl_value): Write it; don't consider imported
> temploid friends as attached to a module.
> (trees_in::decl_value): Read it for non-discarded decls.
> (get_originating_module_decl): Follow the owning decl for an
> imported temploid friend.
> (propagate_defining_module): New.
> (remove_defining_module): New.
> * name-lookup.cc (get_mergeable_namespace_binding): New.
> (lookup_imported_hidden_friend): New.
> * pt.cc (tsubst_friend_function): Propagate defining module for
> new friend functions.
> (tsubst_friend_class): Lookup imported hidden friends. Check
> for valid module attachment of existing names. Propagate
> defining module for new 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-10_d.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-13_f.C: New test.
> *
[PATCH][14 backport] c++: Fix instantiation of imported temploid friends [PR114275]
> > @@ -11751,9 +11767,16 @@ tsubst_friend_class (tree friend_tmpl, tree args)
> > if (tmpl != error_mark_node)
> > {
> > /* The new TMPL is not an instantiation of anything, so we
> > -forget its origins. We don't reset CLASSTYPE_TI_TEMPLATE
> > +forget its origins. It is also not a specialization of
> > +anything. We don't reset CLASSTYPE_TI_TEMPLATE
> > for the new type because that is supposed to be the
> > corresponding template decl, i.e., TMPL. */
> > + spec_entry elt;
> > + elt.tmpl = friend_tmpl;
> > + elt.args = CLASSTYPE_TI_ARGS (TREE_TYPE (tmpl));
> > + elt.spec = TREE_TYPE (tmpl);
> > + type_specializations->remove_elt ();
>
> For GCC 14.2 let's guard this with if (modules_p ()); for GCC 15 it can be
> unconditional. OK.
>
> Jason
>
I'm looking to backport this patch to GCC 14 now that it's been on trunk
some time. Here's the patch I'm aiming to add (squashed with the
changes from r15-220-gec2365e07537e8) after cherrypicking the
prerequisite commit r15-58-g2faf040335f9b4; is this OK?
Or should I keep it as two separate commits to make the cherrypicking
more obvious? Not entirely sure on the etiquette around this.
Bootstrapped and regtested on x86_64-pc-linux-gnu on top of the
releases/gcc-14 branch.
-- >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 class template, that it
doesn't exist at all: it could be a non-exported entity that we've
nevertheless streamed in from an imported module. We need to ensure
that when instantiating template friend classes that we return the same
TEMPLATE_DECL that we got from our imports, otherwise we will get later
issues with 'duplicate_decls' (rightfully) complaining that they're
different when trying to merge.
This doesn't appear necessary for function templates 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.
(remove_defining_module): Declare.
(lookup_imported_hidden_friend): Declare.
* decl.cc (duplicate_decls): Also check if hidden decls can be
redeclared in this module. Call remove_defining_module on
to-be-freed newdecl.
* module.cc (imported_temploid_friends): New.
(init_modules): Initialize it.
(trees_out::decl_value): Write it; don't consider imported
temploid friends as attached to a module.
(trees_in::decl_value): Read it for non-discarded decls.
(get_originating_module_decl): Follow the owning decl for an
imported temploid friend.
(propagate_defining_module): New.
(remove_defining_module): New.
* name-lookup.cc (get_mergeable_namespace_binding): New.
(lookup_imported_hidden_friend): New.
* pt.cc (tsubst_friend_function): Propagate defining module for
new friend functions.
(tsubst_friend_class): Lookup imported hidden friends. Check
for valid module attachment of existing names. Propagate
defining module for new 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-10_d.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-13_f.C: New test.
* g++.dg/modules/tpl-friend-13_g.C: New test.
* g++.dg/modules/tpl-friend-14_a.C: New test.
* g++.dg/modules/tpl-friend-14_b.C: New test.
* g++.dg/modules/tpl-friend-14_c.C: New test.
* g++.dg/modules/tpl-friend-14_d.C: New test.
* g++.dg/modules/tpl-friend-9.C: New test.
Signed-off-by: Nathaniel Shead
Reviewed-by: Jason Merrill
Reviewed-by: Patrick Palka
---
