Re: [C++ PATCH] Fix cxx_printable_name caching

["Richard Guenther" <richard dot guenther at gmail dot com>]

On 9/24/07, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> On x86_64-linux running ./cc1plus -O0 -fdump-tree-eh (or any other tree dump
> option) shows duplicate function names:
>
> // { dg-do compile }
> // { dg-options "-O0 -fdump-tree-eh" }
>
> static int foo (void);
> void f (void);
> int g (void);
> static int
> foo (void)
> {
>   int result;
>   try
>   {
>     result = g ();
>     f ();
>   }
>   catch (const int &)
>   {
>   }
>   return result;
> }
>
> int
> bar ()
> {
>   int result;
>   for (;;)
>     if (foo ())
>       return 1;
> }
>
> // { dg-final { scan-tree-dump-times ";; Function int foo\\(\\)" 1 "eh" } }
> // { dg-final { scan-tree-dump-times ";; Function int bar\\(\\)" 1 "eh" } }
> // { dg-final { cleanup-tree-dump "eh" } }
>
> grep '^;; Function' printablename1.C.012t.eh
> ;; Function int foo() (_Z3barv)
> ;; Function int foo() (_ZL3foov)
>
> The problem seems to be cxx_printable_name's caching.  If it was asked to
> print some FUNCTION_DECL, which is later on garbage collected and later on
> happens to be ggc_alloced to a different FUNCTION_DECL, which
> cxx_printable_name is called on soon afterwards, then it returns cached
> string for the already garbage collected FUNCTION_DECL.
> The following patch fixes this by also remembering DECL_UID, which is
> increasing monotonically and so does not suffer from this problem.
> I have tried to move decl_ring array to file scope instead and
> mark it with GTY((deletable)), but that didn't work, as the old
> FUNCTION_DECL in this case is ggc_free'd (during duplicate_decls) rather
> than normally garbage collected, so there isn't anything that would set
> it to NULL in between the ggc_free call and next cxx_printable_name.
> As ggc_free is a promise that such a pointer isn't held anywhere, that is
> not a GC bug.
>
> Unfortunately, when adding further options this is no longer reproduceable
> on this testcase, so I have nothing to add into the testsuite.

It looks like you should be able to get rid of decl_ring completely?

Richard.

> 2007-09-24  Jakub Jelinek  <jakub@redhat.com>
>
>         * tree.c (cxx_printable_name): Add uid_ring.  Only consider
>         a match if decl is equal to decl_ring[x] and DECL_UID (decl)
>         is equal to uid_ring[x].
>
> --- gcc/cp/tree.c.jj    2007-09-24 17:14:12.000000000 +0200
> +++ gcc/cp/tree.c       2007-09-24 21:38:09.000000000 +0200
> @@ -1156,6 +1156,7 @@ const char *
>  cxx_printable_name (tree decl, int v)
>  {
>    static tree decl_ring[PRINT_RING_SIZE];
> +  static unsigned int uid_ring[PRINT_RING_SIZE];
>    static char *print_ring[PRINT_RING_SIZE];
>    static int ring_counter;
>    int i;
> @@ -1168,7 +1169,7 @@ cxx_printable_name (tree decl, int v)
>
>    /* See if this print name is lying around.  */
>    for (i = 0; i < PRINT_RING_SIZE; i++)
> -    if (decl_ring[i] == decl)
> +    if (decl_ring[i] == decl && uid_ring[i] == DECL_UID (decl))
>        /* yes, so return it.  */
>        return print_ring[i];
>
> @@ -1177,11 +1178,14 @@ cxx_printable_name (tree decl, int v)
>
>    if (current_function_decl != NULL_TREE)
>      {
> -      if (decl_ring[ring_counter] == current_function_decl)
> +      if (decl_ring[ring_counter] == current_function_decl
> +         && uid_ring[ring_counter] == DECL_UID (current_function_decl))
>         ring_counter += 1;
>        if (ring_counter == PRINT_RING_SIZE)
>         ring_counter = 0;
> -      gcc_assert (decl_ring[ring_counter] != current_function_decl);
> +      gcc_assert (decl_ring[ring_counter] != current_function_decl
> +                 || uid_ring[ring_counter]
> +                    != DECL_UID (current_function_decl));
>      }
>
>    if (print_ring[ring_counter])
> @@ -1189,6 +1193,7 @@ cxx_printable_name (tree decl, int v)
>
>    print_ring[ring_counter] = xstrdup (lang_decl_name (decl, v));
>    decl_ring[ring_counter] = decl;
> +  uid_ring[ring_counter] = DECL_UID (decl);
>    return print_ring[ring_counter];
>  }
>
>
>         Jakub
>