Fix PR ipa/65432

Hello.

Following patch wraps symtab_node::{asm_}name with xstrdup_for_dump.

Ready for trunk?
Thanks,
Martin

On Wed, Mar 18, 2015 at 2:24 PM, Martin Liška <mliska@suse.cz> wrote:
> Hello.
>
> Following patch wraps symtab_node::{asm_}name with xstrdup_for_dump.
>
> Ready for trunk?

   /* Gets symbol name of the item.  */
   const char *name (void)
   {
-    return node->name ();
+    return xstrdup_for_dump (node->name ());

shouldn't the methods be called dump_name () then?  And why's
node->name () not already dup-ing the string?

That said, I wonder where we use ->name / ->asm_name.  And why
that's different for ICF.

The patch would be more obvious if all fixes were to dumping sites.

Richard.

> Thanks,
> Martin

On Wed, Mar 18, 2015 at 02:24:26PM +0100, Martin Liška wrote:
> >From 06d7667b7e2be23e21b3ea6599ebb2303074b310 Mon Sep 17 00:00:00 2001
> From: mliska <mliska@suse.cz>
> Date: Wed, 18 Mar 2015 13:59:49 +0100
> Subject: [PATCH] Fix PR ipa/65432
> 
> gcc/ChangeLog:
> 
> 2015-03-18  Martin Liska  <mliska@suse.cz>
> 
> 	PR ipa/65432
> 	* ipa-icf.c (sem_item_optimizer::read_section): Wrap symtab_node::name and
> 	symtab_node::asm_name with xstrdup_for_dump.
> 	* ipa-icf.h: Likewise.
> ---
>  gcc/ipa-icf.c | 3 ++-
>  gcc/ipa-icf.h | 4 ++--
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
> index 25b8306..476076d 100644
> --- a/gcc/ipa-icf.c
> +++ b/gcc/ipa-icf.c
> @@ -1995,7 +1995,8 @@ sem_item_optimizer::read_section (lto_file_decl_data *file_data,
>        gcc_assert (node->definition);
>  
>        if (dump_file)
> -	fprintf (dump_file, "Symbol added:%s (tree: %p, uid:%u)\n", node->asm_name (),
> +	fprintf (dump_file, "Symbol added:%s (tree: %p, uid:%u)\n",
> +		 xstrdup_for_dump (node->asm_name ()),
>  		 (void *) node->decl, node->order);

This doesn't make much sense (unless say operator * has side effects or
similar mess I hope will never show up in GCC).  There is just one call
among the arguments, so I fail to see what could clobber it.
xstrdup_for_dump is for the case where you have two or more function calls
in the *printf arguments that can produce transient strings.

It seems cgraph_node::get_create has similar bug though (using
xstrdup_for_dump when it shouldn't).

> --- a/gcc/ipa-icf.h
> +++ b/gcc/ipa-icf.h
> @@ -174,13 +174,13 @@ public:
>    /* Gets symbol name of the item.  */
>    const char *name (void)
>    {
> -    return node->name ();
> +    return xstrdup_for_dump (node->name ());
>    }
>  
>    /* Gets assembler name of the item.  */
>    const char *asm_name (void)
>    {
> -    return node->asm_name ();
> +    return xstrdup_for_dump (node->asm_name ());
>    }
>  
>    /* Fast equality function based on knowledge known in WPA.  */

But for this reason I must say I don't like this change either,
if you use sem_item::name or asm_name just once (happens in various places),
there is no need to preserve it any way.
IMNSHO you should instead stick it in the calls that have
more than one %s format specifiers (and only those that can have more than
one transient strings).
That is probably just the call in sem_function::equals (4x),
sem_variable::equals (ditto), sem_item_optimizer::merge_classes (twice 2x)
and that's it.

	Jakub