On Wed, May 1, 2024 at 9:35 PM Nathaniel Shead
<nathanielosh...@gmail.com> wrote:
>
> On Thu, May 02, 2024 at 12:15:44AM +1000, Nathaniel Shead wrote:
> > 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 <nathanielosh...@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.
> >
>
> Here's that patch. Bootstrapped and regtested on x86_64-pc-linux-gnu, OK
> for trunk/14.2?
LGTM
>
> -- >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 fixes the problem by using DECL_UID as the map key instead of
> the tree directly, much like with entity_map, since even if a
> declaration gets deallocated the DECL_UID should not be reused by a
> later declaration.
>
> gcc/cp/ChangeLog:
>
> * module.cc (imported_temploid_friends): Map from DECL_UID
> instead of tree.
> (get_originating_module_decl): Likewise.
> (propagate_defining_module): Likewise.
> (trees_out::decl_value): Likewise.
> (trees_in::decl_value): Likewise. And only enter into the map
> for new declarations.
> (init_modules): Update type of imported_temploid_friends.
>
> Signed-off-by: Nathaniel Shead <nathanielosh...@gmail.com>
> ---
> gcc/cp/module.cc | 23 ++++++++++++++---------
> 1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index fac0301d80e..ceb383ba509 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -2730,8 +2730,13 @@ static keyed_map_t *keyed_table;
> /* Instantiations of temploid friends imported from another module
> 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;
> + no other easy way to get this information. We map the DECL_UID
> + instead of the tree directly to handle keys that get freed and
> + reused. */
> +typedef hash_map<unsigned, tree,
> + simple_hashmap_traits<int_hash<unsigned, 0>, tree>
> + > temploid_map_t;
> +static temploid_map_t *imported_temploid_friends;
>
> /********************************************************************/
> /* Tree streaming. The tree streaming is very specific to the tree
> @@ -7829,7 +7834,7 @@ trees_out::decl_value (tree decl, depset *dep)
> /* But don't consider imported temploid friends as attached,
> since importers will need to merge this decl even if it was
> attached to a different module. */
> - if (imported_temploid_friends->get (decl))
> + if (imported_temploid_friends->get (DECL_UID (decl)))
> is_attached = false;
>
> bits.b (is_attached);
> @@ -8014,7 +8019,7 @@ trees_out::decl_value (tree decl, depset *dep)
> {
> /* Write imported temploid friends so that importers can reconstruct
> this information on stream-in. */
> - tree* slot = imported_temploid_friends->get (decl);
> + tree* slot = imported_temploid_friends->get (DECL_UID (decl));
> tree_node (slot ? *slot : NULL_TREE);
> }
>
> @@ -8327,7 +8332,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_UID (decl), owner);
>
> /* Regular typedefs will have a NULL TREE_TYPE at this point. */
> unsigned tdef_flags = 0;
> @@ -18976,7 +18982,7 @@ get_originating_module_decl (tree decl)
> /* An imported temploid friend is attached to the same module the
> befriending class was. */
> if (imported_temploid_friends)
> - if (tree *slot = imported_temploid_friends->get (decl))
> + if (tree *slot = imported_temploid_friends->get (DECL_UID (decl)))
> decl = *slot;
>
> int use;
> @@ -19306,7 +19312,7 @@ propagate_defining_module (tree decl, tree orig)
>
> if (DECL_LANG_SPECIFIC (not_tmpl) && DECL_MODULE_IMPORT_P (not_tmpl))
> {
> - bool exists = imported_temploid_friends->put (decl, orig);
> + bool exists = imported_temploid_friends->put (DECL_UID (decl), orig);
>
> /* We should only be called if lookup for an existing decl
> failed, in which case there shouldn't already be an entry
> @@ -20528,8 +20534,7 @@ init_modules (cpp_reader *reader)
> pending_table = new pending_map_t (EXPERIMENT (1, 400));
> 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));
> + imported_temploid_friends = new temploid_map_t (EXPERIMENT (1, 400));
> }
>
> #if CHECKING_P
> --
> 2.43.2
>
