Re: PR lto/47497 (undefined symbol when building soplex)

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

On Wed, Mar 2, 2011 at 11:07 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>
> Hi,
> the problem is with thunks referring thunks or aliases.
>
> lto-symtab is wrong here and when moving thunks&aliases associated with one
> node to the other node, it overwrites thunk.alias by the destination node. It
> consequently turns thunk referring another thunk into thunk referring the
> functoin itself.
>
> I fixed this by adding extra loop setting alias decl to prevailing decl. Richi,
> perhaps there is better place to put this? ?I don't like it being in the loop
> redirecting thunks&aliases from one node to another because
>
> ?1) I think that loop is not quite correct. ?When one function is prevailed by
> another, local static thunk from the first function should not be redirected to
> another. ?That happens correctly and is harmless (we end up with dead thunk)
> ?2) It may happen that thunks get prevailed other way than nodes they are
> aliased with. ?We use comdat groups that prevents this from happening, but I
> would rather not 100% rely on this on all targets since not all targets
> implements comdat groups. ?So I think it is more robust to simply merge the
> decls in alias field like we merge other decls.
>
> Fixing this problem cause different problem with streaming. ?When we have alias
> A referring function F and alias B referring alias A and we are unlucky with
> prevailing and other things, we might end up streaming alias B before alias A.
> This leads us to call cgraph_same_body_alias on decl of A before A is added to
> cgraph as an alias. ?Consequently cgraph_same_body_alias does nothing later
> when we try to create alias A itself, because the node already exists.
>
> This patch fixes it by adding node pointer into the cgraph_same_body_alias and
> cgraph_add_thunk so the thunks&aliases can be added in random order w/o
> problems as long as the function they are associated with is already in cgraph.
>
> Bootstraped/regtested x86_64-linux, also tested with Mozilla and qt build.

Comments inline

> Honza
> ? ? ? ?PR lto/47497
> ? ? ? ?* lto-symtab.c (lto_cgraph_replace_node): Do not set thunk.alias.
> ? ? ? ?(lto_symtab_merge_cgraph_nodes_1): Update thunk.alias pointers here.
> ? ? ? ?* cgraph.h (cgraph_same_body_alias, cgraph_add_thunk): Add node pointers.
> ? ? ? ?* cgraph.c (cgraph_same_body_alias_1, cgraph_same_body_alias,
> ? ? ? ?cgraph_add_thunk): Add node pointers.
> ? ? ? ?* lto-cgraph.c (lto_output_node): Verify that thunks&aliases are
> ? ? ? ?associated to right node.
> ? ? ? ?(input_node): Update use of cgraph_same_body_alias
> ? ? ? ?and cgraph_add_thunk.
>
> ? ? ? ?* optimize.c (maybe_clone_body): Update call of cgraph_same_body_alias
> ? ? ? ?and cgraph_add_thunk.
> ? ? ? ?* method.c (make_alias_for_thunk, use_thunk): Likewise.
> ? ? ? ?* mangle.c (mangle_decl): Likewise.
> Index: lto-symtab.c
> ===================================================================
> --- lto-symtab.c ? ? ? ?(revision 170364)
> +++ lto-symtab.c ? ? ? ?(working copy)
> @@ -273,7 +273,6 @@ lto_cgraph_replace_node (struct cgraph_n
> ? ? ? ? ?last = alias;
> ? ? ? ? ?gcc_assert (alias->same_body_alias);
> ? ? ? ? ?alias->same_body = prevailing_node;
> - ? ? ? ? alias->thunk.alias = prevailing_node->decl;
> ? ? ? ?}
> ? ? ? last->next = prevailing_node->same_body;
> ? ? ? /* Node with aliases is prevailed by alias.
> @@ -828,8 +827,16 @@ lto_symtab_merge_cgraph_nodes_1 (void **
> ?void
> ?lto_symtab_merge_cgraph_nodes (void)
> ?{
> + ?struct cgraph_node *node, *alias, *next;
> ? lto_symtab_maybe_init_hash_table ();
> ? htab_traverse (lto_symtab_identifiers, lto_symtab_merge_cgraph_nodes_1, NULL);
> +
> + ?for (node = cgraph_nodes; node; node = node->next)
> + ? ?for (alias = node->same_body; alias; alias = next)
> + ? ? ?{
> + ? ? ? next = alias->next;
> + ? ? ? alias->thunk.alias = lto_symtab_prevailing_decl (alias->thunk.alias);
> + ? ? ?}

Not a very nice place but I guess ok.

> ?}
>
> ?/* Given the decl DECL, return the prevailing decl with the same name. */
> Index: cgraph.h
> ===================================================================
> --- cgraph.h ? ?(revision 170364)
> +++ cgraph.h ? ?(working copy)
> @@ -559,8 +559,8 @@ struct cgraph_indirect_call_info *cgraph
> ?struct cgraph_node * cgraph_get_node (const_tree);
> ?struct cgraph_node * cgraph_get_node_or_alias (const_tree);
> ?struct cgraph_node * cgraph_node (tree);
> -struct cgraph_node * cgraph_same_body_alias (tree, tree);
> -struct cgraph_node * cgraph_add_thunk (tree, tree, bool, HOST_WIDE_INT,
> +struct cgraph_node * cgraph_same_body_alias (struct cgraph_node *, tree, tree);
> +struct cgraph_node * cgraph_add_thunk (struct cgraph_node *, tree, tree, bool, HOST_WIDE_INT,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? HOST_WIDE_INT, tree, tree);
> ?void cgraph_remove_same_body_alias (struct cgraph_node *);
> ?struct cgraph_node *cgraph_node_for_asm (tree);
> Index: cgraph.c
> ===================================================================
> --- cgraph.c ? ?(revision 170364)
> +++ cgraph.c ? ?(working copy)
> @@ -536,16 +536,16 @@ cgraph_node (tree decl)
> ? return node;
> ?}
>
> -/* Mark ALIAS as an alias to DECL. ?*/
> +/* Mark ALIAS as an alias to DECL. ?DECL_NODE is cgraph node representing
> + ? the function body is associated with (not neccesarily cgraph_node (DECL). ?*/
>
> ?static struct cgraph_node *
> -cgraph_same_body_alias_1 (tree alias, tree decl)
> +cgraph_same_body_alias_1 (struct cgraph_node *decl_node, tree alias, tree decl)
> ?{
> - ?struct cgraph_node key, *alias_node, *decl_node, **slot;
> + ?struct cgraph_node key, *alias_node, **slot;
>
> ? gcc_assert (TREE_CODE (decl) == FUNCTION_DECL);
> ? gcc_assert (TREE_CODE (alias) == FUNCTION_DECL);
> - ?decl_node = cgraph_node (decl);
>
> ? key.decl = alias;
>
> @@ -575,7 +575,7 @@ cgraph_same_body_alias_1 (tree alias, tr
> ? ?and cgraph_node (ALIAS) transparently returns cgraph_node (DECL). ? */
>
> ?struct cgraph_node *
> -cgraph_same_body_alias (tree alias, tree decl)
> +cgraph_same_body_alias (struct cgraph_node *decl_node, tree alias, tree decl)
> ?{
> ?#ifndef ASM_OUTPUT_DEF
> ? /* If aliases aren't supported by the assembler, fail. ?*/
> @@ -584,7 +584,7 @@ cgraph_same_body_alias (tree alias, tree
>
> ? /*gcc_assert (!assembler_name_hash);*/
>
> - ?return cgraph_same_body_alias_1 (alias, decl);
> + ?return cgraph_same_body_alias_1 (decl_node, alias, decl);
> ?}
>
> ?/* Add thunk alias into callgraph. ?The alias declaration is ALIAS and it
> @@ -592,7 +592,8 @@ cgraph_same_body_alias (tree alias, tree
> ? ?See comments in thunk_adjust for detail on the parameters. ?*/
>
> ?struct cgraph_node *
> -cgraph_add_thunk (tree alias, tree decl, bool this_adjusting,
> +cgraph_add_thunk (struct cgraph_node *decl_node, tree alias, tree decl,
> + ? ? ? ? ? ? ? ? bool this_adjusting,
> ? ? ? ? ? ? ? ? ?HOST_WIDE_INT fixed_offset, HOST_WIDE_INT virtual_value,
> ? ? ? ? ? ? ? ? ?tree virtual_offset,
> ? ? ? ? ? ? ? ? ?tree real_alias)
> @@ -606,7 +607,7 @@ cgraph_add_thunk (tree alias, tree decl,
> ? ? ? cgraph_remove_node (node);
> ? ? }
>
> - ?node = cgraph_same_body_alias_1 (alias, decl);
> + ?node = cgraph_same_body_alias_1 (decl_node, alias, decl);
> ? gcc_assert (node);
> ? gcc_checking_assert (!virtual_offset
> ? ? ? ? ? ? ? ? ? ? ? || tree_int_cst_equal (virtual_offset,
> @@ -2722,7 +2723,7 @@ cgraph_propagate_frequency (struct cgrap
> ? ? ? ?case NODE_FREQUENCY_EXECUTED_ONCE:
> ? ? ? ? ?if (dump_file && (dump_flags & TDF_DETAILS))
> ? ? ? ? ? ?fprintf (dump_file, " ?Called by %s that is executed once\n",
> - ? ? ? ? ? ? ? ? ? ?cgraph_node_name (node));
> + ? ? ? ? ? ? ? ? ? ?cgraph_node_name (edge->caller));

unrelated change?

> ? ? ? ? ?maybe_unlikely_executed = false;
> ? ? ? ? ?if (edge->loop_nest)
> ? ? ? ? ? ?{
> @@ -2735,7 +2736,7 @@ cgraph_propagate_frequency (struct cgrap
> ? ? ? ?case NODE_FREQUENCY_NORMAL:
> ? ? ? ? ?if (dump_file && (dump_flags & TDF_DETAILS))
> ? ? ? ? ? ?fprintf (dump_file, " ?Called by %s that is normal or hot\n",
> - ? ? ? ? ? ? ? ? ? ?cgraph_node_name (node));
> + ? ? ? ? ? ? ? ? ? ?cgraph_node_name (edge->caller));

likewise.

> ? ? ? ? ?maybe_unlikely_executed = false;
> ? ? ? ? ?maybe_executed_once = false;
> ? ? ? ? ?break;
> Index: cp/optimize.c
> ===================================================================
> --- cp/optimize.c ? ? ? (revision 170364)
> +++ cp/optimize.c ? ? ? (working copy)
> @@ -309,7 +309,7 @@ maybe_clone_body (tree fn)
> ? ? ? ? ?&& (!DECL_ONE_ONLY (fns[0])
> ? ? ? ? ? ? ?|| (HAVE_COMDAT_GROUP
> ? ? ? ? ? ? ? ? ?&& DECL_WEAK (fns[0])))
> - ? ? ? ? && cgraph_same_body_alias (clone, fns[0]))
> + ? ? ? ? && cgraph_same_body_alias (cgraph_node (fns[0]), clone, fns[0]))

Not cgraph_get_node?

> ? ? ? ?{
> ? ? ? ? ?alias = true;
> ? ? ? ? ?if (DECL_ONE_ONLY (fns[0]))
> Index: cp/method.c
> ===================================================================
> --- cp/method.c (revision 170364)
> +++ cp/method.c (working copy)
> @@ -259,7 +259,8 @@ make_alias_for_thunk (tree function)
>
> ? if (!flag_syntax_only)
> ? ? {
> - ? ? ?struct cgraph_node *aliasn = cgraph_same_body_alias (alias, function);
> + ? ? ?struct cgraph_node *aliasn = cgraph_same_body_alias (cgraph_node (function),
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?alias, function);

Likewise.

> ? ? ? DECL_ASSEMBLER_NAME (function);
> ? ? ? gcc_assert (aliasn != NULL);
> ? ? }
> @@ -376,7 +377,7 @@ use_thunk (tree thunk_fndecl, bool emit_
> ? a = nreverse (t);
> ? DECL_ARGUMENTS (thunk_fndecl) = a;
> ? TREE_ASM_WRITTEN (thunk_fndecl) = 1;
> - ?cgraph_add_thunk (thunk_fndecl, function,
> + ?cgraph_add_thunk (cgraph_node (function), thunk_fndecl, function,

Likewise.

> ? ? ? ? ? ? ? ? ? ?this_adjusting, fixed_offset, virtual_value,
> ? ? ? ? ? ? ? ? ? ?virtual_offset, alias);
>
> Index: cp/mangle.c
> ===================================================================
> --- cp/mangle.c (revision 170364)
> +++ cp/mangle.c (working copy)
> @@ -3109,7 +3109,7 @@ mangle_decl (const tree decl)
> ? ? ? if (vague_linkage_p (decl))
> ? ? ? ?DECL_WEAK (alias) = 1;
> ? ? ? if (TREE_CODE (decl) == FUNCTION_DECL)
> - ? ? ? cgraph_same_body_alias (alias, decl);
> + ? ? ? cgraph_same_body_alias (cgraph_node (decl), alias, decl);

Likewise.

> ? ? ? else
> ? ? ? ?varpool_extra_name_alias (alias, decl);
> ?#endif
> Index: lto-cgraph.c
> ===================================================================
> --- lto-cgraph.c ? ? ? ?(revision 170364)
> +++ lto-cgraph.c ? ? ? ?(working copy)
> @@ -551,6 +551,7 @@ lto_output_node (struct lto_simple_outpu
> ? ? ? ? ? ? ?lto_output_fn_decl_index (ob->decl_state, ob->main_stream,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?alias->thunk.alias);
> ? ? ? ? ? ?}
> + ? ? ? ? gcc_assert (cgraph_node (alias->thunk.alias) == node);

Likewise.

The patch looks ok with cgraph_node exchanged for cgraph_get_node
(and re-testing).

Thanks,
Richard.


> ? ? ? ? ?lto_output_uleb128_stream (ob->main_stream, alias->resolution);
> ? ? ? ? ?alias = alias->previous;
> ? ? ? ?}
> @@ -1094,7 +1095,7 @@ input_node (struct lto_file_decl_data *f
> ? ? ? ? ?tree real_alias;
> ? ? ? ? ?decl_index = lto_input_uleb128 (ib);
> ? ? ? ? ?real_alias = lto_file_decl_data_get_fn_decl (file_data, decl_index);
> - ? ? ? ? alias = cgraph_same_body_alias (alias_decl, real_alias);
> + ? ? ? ? alias = cgraph_same_body_alias (node, alias_decl, real_alias);
> ? ? ? ?}
> ? ? ? else
> ? ? ? ? {
> @@ -1103,12 +1104,13 @@ input_node (struct lto_file_decl_data *f
> ? ? ? ? ?tree real_alias;
> ? ? ? ? ?decl_index = lto_input_uleb128 (ib);
> ? ? ? ? ?real_alias = lto_file_decl_data_get_fn_decl (file_data, decl_index);
> - ? ? ? ? alias = cgraph_add_thunk (alias_decl, fn_decl, type & 2, fixed_offset,
> + ? ? ? ? alias = cgraph_add_thunk (node, alias_decl, fn_decl, type & 2, fixed_offset,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?virtual_value,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?(type & 4) ? size_int (virtual_value) : NULL_TREE,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?real_alias);
> ? ? ? ?}
> - ? ? ? alias->resolution = (enum ld_plugin_symbol_resolution)lto_input_uleb128 (ib);
> + ? ? ?gcc_assert (alias);
> + ? ? ?alias->resolution = (enum ld_plugin_symbol_resolution)lto_input_uleb128 (ib);
> ? ? }
> ? return node;
> ?}
>