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 <[email protected]>
> Date: Wed, 18 Mar 2015 13:59:49 +0100
> Subject: [PATCH] Fix PR ipa/65432
>
> gcc/ChangeLog:
>
> 2015-03-18 Martin Liska <[email protected]>
>
> 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
