c++/modules: Fix dangling pointer with imported_temploid_friends

Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk (and
later 14.2)?  I don't think making it a GTY root is necessary but I felt
perhaps better to be safe than sorry.

Potentially another approach would be to use DECL_UID instead like how
entity_map does; would that be preferable?

-- >8 --

I got notified by Linaro CI and by checking testresults that there seems
to be some occasional failures in tpl-friend-4_b.C on some architectures
and standards modes since r15-59-gb5f6a56940e708.  I haven't been able
to reproduce but looking at the backtrace I suspect the issue is that
we're adding to the 'imported_temploid_friend' map a decl that is
ultimately discarded, which then has its address reused by a later decl
causing a failure in the assert in 'set_originating_module'.

This patch attempts to fix the issue in two ways: by ensuring that we
only store the decl if we know it's a new decl (and hence won't be
discarded), and by making the imported_temploid_friends map a GTY root
so that even if the decl does get discarded later the address isn't
reused.

gcc/cp/ChangeLog:

	* module.cc (imported_temploid_friends): Mark GTY, and...
	(init_modules): ...allocate from GGC.
	(trees_in::decl_value): Only write to imported_temploid_friends
	for new decls.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/module.cc | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

On Wed, 1 May 2024, Nathaniel Shead wrote:

> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk (and
> later 14.2)?  I don't think making it a GTY root is necessary but I felt
> perhaps better to be safe than sorry.
> 
> Potentially another approach would be to use DECL_UID instead like how
> entity_map does; would that be preferable?
> 
> -- >8 --
> 
> I got notified by Linaro CI and by checking testresults that there seems
> to be some occasional failures in tpl-friend-4_b.C on some architectures
> and standards modes since r15-59-gb5f6a56940e708.  I haven't been able
> to reproduce but looking at the backtrace I suspect the issue is that
> we're adding to the 'imported_temploid_friend' map a decl that is
> ultimately discarded, which then has its address reused by a later decl
> causing a failure in the assert in 'set_originating_module'.
> 
> This patch attempts to fix the issue in two ways: by ensuring that we
> only store the decl if we know it's a new decl (and hence won't be
> discarded), and by making the imported_temploid_friends map a GTY root
> so that even if the decl does get discarded later the address isn't
> reused.
> 
> gcc/cp/ChangeLog:
> 
> 	* module.cc (imported_temploid_friends): Mark GTY, and...
> 	(init_modules): ...allocate from GGC.
> 	(trees_in::decl_value): Only write to imported_temploid_friends
> 	for new decls.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>  gcc/cp/module.cc | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index 5b8ff5bc483..37d38bb9654 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -2731,7 +2731,7 @@ static keyed_map_t *keyed_table;
>     need to be attached to the same module as the temploid.  This maps
>     these decls to the temploid they are instantiated them, as there is
>     no other easy way to get this information.  */
> -static hash_map<tree, tree> *imported_temploid_friends;
> +static GTY(()) hash_map<tree, tree> *imported_temploid_friends;
>  
>  /********************************************************************/
>  /* Tree streaming.   The tree streaming is very specific to the tree
> @@ -8327,7 +8327,8 @@ trees_in::decl_value ()
>    if (TREE_CODE (inner) == FUNCTION_DECL
>        || TREE_CODE (inner) == TYPE_DECL)
>      if (tree owner = tree_node ())
> -      imported_temploid_friends->put (decl, owner);
> +      if (is_new)
> +	imported_temploid_friends->put (decl, owner);

Hmm, I'm not seeing this code path getting reached for tpl-friend-4_b.C.
It seems we're instead adding to imported_temploid_friends from
propagate_defining_module, during tsubst_friend_function.

What seems to be happening is that we we first tsubst_friend_function
'foo' from TPL<int>, and then we tsubst_friend_function 'foo' from DEF<int>,
which ends up calling duplicate_decls, which ggc_frees this 'foo'
redeclaration that is still present in the imported_temploid_friends map.

So I don't think marking imported_temploid_friends as a GC root would
help with this situation.  If we want to keep imported_temploid_friends
as a tree -> tree map, I think we just need to ensure that a decl
is removed from the map upon getting ggc_free'd from e.g.  duplicate_decls.

But it seems simpler to use DECL_UID as the key instead, since those
never get reused even after the decl gets ggc_free'd IIUC.

>  
>    /* Regular typedefs will have a NULL TREE_TYPE at this point.  */
>    unsigned tdef_flags = 0;
> @@ -20523,7 +20524,7 @@ init_modules (cpp_reader *reader)
>        entity_map = new entity_map_t (EXPERIMENT (1, 400));
>        vec_safe_reserve (entity_ary, EXPERIMENT (1, 400));
>        imported_temploid_friends
> -	= new hash_map<tree,tree> (EXPERIMENT (1, 400));
> +	= hash_map<tree,tree>::create_ggc (EXPERIMENT (1, 400));
>      }
>  
>  #if CHECKING_P
> -- 
> 2.43.2
> 
>

On Wed, May 01, 2024 at 09:57:38AM -0400, Patrick Palka wrote:
> 
> On Wed, 1 May 2024, Nathaniel Shead wrote:
> 
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk (and
> > later 14.2)?  I don't think making it a GTY root is necessary but I felt
> > perhaps better to be safe than sorry.
> > 
> > Potentially another approach would be to use DECL_UID instead like how
> > entity_map does; would that be preferable?
> > 
> > -- >8 --
> > 
> > I got notified by Linaro CI and by checking testresults that there seems
> > to be some occasional failures in tpl-friend-4_b.C on some architectures
> > and standards modes since r15-59-gb5f6a56940e708.  I haven't been able
> > to reproduce but looking at the backtrace I suspect the issue is that
> > we're adding to the 'imported_temploid_friend' map a decl that is
> > ultimately discarded, which then has its address reused by a later decl
> > causing a failure in the assert in 'set_originating_module'.
> > 
> > This patch attempts to fix the issue in two ways: by ensuring that we
> > only store the decl if we know it's a new decl (and hence won't be
> > discarded), and by making the imported_temploid_friends map a GTY root
> > so that even if the decl does get discarded later the address isn't
> > reused.
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* module.cc (imported_temploid_friends): Mark GTY, and...
> > 	(init_modules): ...allocate from GGC.
> > 	(trees_in::decl_value): Only write to imported_temploid_friends
> > 	for new decls.
> > 
> > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> > ---
> >  gcc/cp/module.cc | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> > index 5b8ff5bc483..37d38bb9654 100644
> > --- a/gcc/cp/module.cc
> > +++ b/gcc/cp/module.cc
> > @@ -2731,7 +2731,7 @@ static keyed_map_t *keyed_table;
> >     need to be attached to the same module as the temploid.  This maps
> >     these decls to the temploid they are instantiated them, as there is
> >     no other easy way to get this information.  */
> > -static hash_map<tree, tree> *imported_temploid_friends;
> > +static GTY(()) hash_map<tree, tree> *imported_temploid_friends;
> >  
> >  /********************************************************************/
> >  /* Tree streaming.   The tree streaming is very specific to the tree
> > @@ -8327,7 +8327,8 @@ trees_in::decl_value ()
> >    if (TREE_CODE (inner) == FUNCTION_DECL
> >        || TREE_CODE (inner) == TYPE_DECL)
> >      if (tree owner = tree_node ())
> > -      imported_temploid_friends->put (decl, owner);
> > +      if (is_new)
> > +	imported_temploid_friends->put (decl, owner);
> 
> Hmm, I'm not seeing this code path getting reached for tpl-friend-4_b.C.
> It seems we're instead adding to imported_temploid_friends from
> propagate_defining_module, during tsubst_friend_function.
> 
> What seems to be happening is that we we first tsubst_friend_function
> 'foo' from TPL<int>, and then we tsubst_friend_function 'foo' from DEF<int>,
> which ends up calling duplicate_decls, which ggc_frees this 'foo'
> redeclaration that is still present in the imported_temploid_friends map.
> 
> So I don't think marking imported_temploid_friends as a GC root would
> help with this situation.  If we want to keep imported_temploid_friends
> as a tree -> tree map, I think we just need to ensure that a decl
> is removed from the map upon getting ggc_free'd from e.g.  duplicate_decls.
> 
> But it seems simpler to use DECL_UID as the key instead, since those
> never get reused even after the decl gets ggc_free'd IIUC.
> 

Ah right, thanks for digging into that further.  Yup OK, I think
probably the DECL_UID route feels safer to me then in case there are
other places where a decl might be explicitly freed.

Looking at tree.cc it looks like the relevant function is
'allocate_decl_uid' which shouldn't reuse UIDs until 2^32 decls have
been created, so we should be safe on the reuse front.

I'll draft and test a patch for that tomorrow morning.

> >  
> >    /* Regular typedefs will have a NULL TREE_TYPE at this point.  */
> >    unsigned tdef_flags = 0;
> > @@ -20523,7 +20524,7 @@ init_modules (cpp_reader *reader)
> >        entity_map = new entity_map_t (EXPERIMENT (1, 400));
> >        vec_safe_reserve (entity_ary, EXPERIMENT (1, 400));
> >        imported_temploid_friends
> > -	= new hash_map<tree,tree> (EXPERIMENT (1, 400));
> > +	= hash_map<tree,tree>::create_ggc (EXPERIMENT (1, 400));
> >      }
> >  
> >  #if CHECKING_P
> > -- 
> > 2.43.2
> > 
> > 
>