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

[Patrick Palka <ppalka@redhat.com>]

On Wed, May 1, 2024 at 9:35 PM Nathaniel Shead
<nathanieloshead@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 <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.
> >
>
> 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 <nathanieloshead@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
>