Re: [PATCH] c++/modules: Fix dangling pointer with imported_temploid_friends

[Patrick Palka <ppalka@redhat.com>]

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
> 
>