| Message ID | 20110302100730.GA4958@kam.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
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; > } >
> > @@ -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? Uh, yes. Bugfix, but I will commit it independently. > > 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? Well, cgraph was originally designed with cgraph_node being the way to get from decl to callgraph node. Since callgraph node no longer is just an vertex of graph, the fact that cgraph_node lazilly allocates the nodes on demand no longer makes sense and I will change that for 4.7 requiring explicit cgraph_create_node calls. (to avoid funny bugs like this one). At the moment I am not quite sure if using cgraph_get_node at random places makes more sense, but I will update the patch and re-test. (I am not even sure if the main function is already in cgraph at time frontend adds thunks to it). Honza
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); + } } /* 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)); 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)); 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])) { 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); 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, 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); 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); 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; }