| Message ID | 20150414091414.GA50171@msticlxl57.ims.intel.com |
|---|---|
| State | New |
| Headers | show |
Ping 2015-04-14 12:14 GMT+03:00 Ilya Enkovich <enkovich.gnu@gmail.com>: > On 10 Apr 03:15, Jan Hubicka wrote: >> > >> > References are not streamed out for nodes which are referenced in a >> > partition but don't belong to it ('continue' condition in output_refs >> > loop). >> >> Yeah, but it already special cases aliases (because we now always preserve IPA_REF_ALIAS link >> in the boundary, so I think making IPA_REF_CHKP to be rpeserved should go the same path) >> so we can special case the instrumentation thunks too? > > Any cgraph_node having instrumented clone should have it, not only instrumentation thunks. Surely we can make an exception for IPA_REF_CHKP. > >> > >> > > >> > > I suppose we still need to >> > > 1) write_symbol and lto-symtab should know that transparent aliases are not real >> > > symbols and ignore them. We have predicate symtab_node::real_symbol_p, >> > > I suppose it should return true. >> > > >> > > I think cgraph_merge and varpool_merge in lto-symtab needs to know what to do >> > > when merging two symbols with transparent aliases. >> > > 2) At some point we need to populate transparent alias links as discussed in the >> > > other thread. >> > > 3) For safety, I guess we want symbol_table::change_decl_assembler_name to either >> > > sanity check there are no trasparent alias links pointing to old assembler >> > > names or update it. >> > >> > Wouldn't search for possible referring via transparent aliases nodes >> > be too expensive? >> >> Changing of decl_assembler_name is not very common and if we set up the links >> only after renaming of statics, i think we are safe. But it would be better to >> keep verify it at some point. >> >> I suppose verify_node check that there is no transparent alias link on symbols >> were it is not supposed to be and that there is link when it is supposed to be (i.e. >> symtab_state is >=EXPANSION and symbol is either weakref on tragets w/o native weakrefs >> or instrumentation cone. >> >> Would you please mind adding the test and making sure it triggers when things are >> out of sync? >> > > OK, but I suppose it should be a part of transparent alias links rework discussed in another thread. BTW are you going to intoroduce transparent aliases in cgraph soon? > >> > >> > > 4) I think we want verify_node to check that transparent alias links and chkp points >> > > as intended and only on those symbols where they need to be >> > > >> > > There is also logic in lto-partition to always insert alias target >> > >> > OK, so you have the chkp references that links to instrumented_version. >> > >> > You do not stream them for boundary symbols but for some reason they are needed, >> > >> > but when you can end up with wrong CHKP link? >> > >> >> > >> Wrong links may appear when cgraph nodes are merged. >> > > >> > > I see, I think you really want to fix these at merging time as sugested in 1). >> > > In this case even the node->instrumented_version pointer may be out of date and >> > > point to a node that lost in merging? >> > >> > node->instrumented_version is redirected and never points to lost >> > symbol. But during merge we may have situations when >> > (node->instrumented_version->instrumented_version != node) because of >> > multiple not merged (yet) symbols referencing the same merged target. >> >> OK, which should not be a problem if we stream the links instead of rebuilding them, right? > > IPA_REF_CHKP refs don't affect instrumented_version merging. And I actually don't see here any problems, instrumented_version links always come into consistent state when symbol merging finishes. > > > Here is a new patch version. It has no chkp_maybe_fix_chkp_ref function, IPA_REF_CHKP references are streamed out instead. Bootstrapped and tested on x86_64-unknown-linux-gnu. OK for trunk? I also want to port it to GCC 5 branch after release. > > Thanks, > Ilya > -- > gcc/ > > 2015-04-14 Ilya Enkovich <ilya.enkovich@intel.com> > > * ipa.c (symbol_table::remove_unreachable_nodes): Don't > remove instumentation thunks calling reachable functions. > * lto-cgraph.c (output_refs): Always output IPA_REF_CHKP. > * lto/lto-partition.c (privatize_symbol_name_1): New. > (privatize_symbol_name): Privatize both decl and orig_decl > names for instrumented functions. > > gcc/testsuite/ > > 2015-04-14 Ilya Enkovich <ilya.enkovich@intel.com> > > * gcc.dg/lto/chkp-privatize-1_0.c: New. > * gcc.dg/lto/chkp-privatize-1_1.c: New. > * gcc.dg/lto/chkp-privatize-2_0.c: New. > * gcc.dg/lto/chkp-privatize-2_1.c: New. > > > diff --git a/gcc/ipa.c b/gcc/ipa.c > index b3752de..3054afe 100644 > --- a/gcc/ipa.c > +++ b/gcc/ipa.c > @@ -492,7 +492,22 @@ symbol_table::remove_unreachable_nodes (FILE *file) > } > else if (cnode->thunk.thunk_p) > enqueue_node (cnode->callees->callee, &first, &reachable); > - > + > + /* For instrumentation clones we always need original > + function node for proper LTO privatization. */ > + if (cnode->instrumentation_clone > + && reachable.contains (cnode) > + && cnode->definition) > + { > + gcc_assert (cnode->instrumented_version || in_lto_p); > + if (cnode->instrumented_version) > + { > + enqueue_node (cnode->instrumented_version, &first, > + &reachable); > + reachable.add (cnode->instrumented_version); > + } > + } > + > /* If any reachable function has simd clones, mark them as > reachable as well. */ > if (cnode->simd_clones) > diff --git a/gcc/lto-cgraph.c b/gcc/lto-cgraph.c > index ac50e4b..ea352f1 100644 > --- a/gcc/lto-cgraph.c > +++ b/gcc/lto-cgraph.c > @@ -805,8 +805,33 @@ output_refs (lto_symtab_encoder_t encoder) > { > symtab_node *node = lto_symtab_encoder_deref (encoder, i); > > + /* IPA_REF_ALIAS and IPA_REF_CHKP references are always preserved > + in the boundary. Alias node can't have other references and > + can be always handled as if it's not in the boundary. */ > if (!node->alias && !lto_symtab_encoder_in_partition_p (encoder, node)) > - continue; > + { > + cgraph_node *cnode = dyn_cast <cgraph_node *> (node); > + /* Output IPA_REF_CHKP reference. */ > + if (cnode > + && cnode->instrumented_version > + && !cnode->instrumentation_clone) > + { > + for (int i = 0; node->iterate_reference (i, ref); i++) > + if (ref->use == IPA_REF_CHKP) > + { > + if (lto_symtab_encoder_lookup (encoder, ref->referred) > + != LCC_NOT_FOUND) > + { > + int nref = lto_symtab_encoder_lookup (encoder, node); > + streamer_write_gcov_count_stream (ob->main_stream, 1); > + streamer_write_uhwi_stream (ob->main_stream, nref); > + lto_output_ref (ob, ref, encoder); > + } > + break; > + } > + } > + continue; > + } > > count = node->ref_list.nreferences (); > if (count) > diff --git a/gcc/lto/lto-partition.c b/gcc/lto/lto-partition.c > index 235b735..7d117e9 100644 > --- a/gcc/lto/lto-partition.c > +++ b/gcc/lto/lto-partition.c > @@ -877,27 +877,13 @@ validize_symbol_for_target (symtab_node *node) > } > } > > -/* Mangle NODE symbol name into a local name. > - This is necessary to do > - 1) if two or more static vars of same assembler name > - are merged into single ltrans unit. > - 2) if previously static var was promoted hidden to avoid possible conflict > - with symbols defined out of the LTO world. */ > +/* Helper for privatize_symbol_name. Mangle NODE symbol name > + represented by DECL. */ > > static bool > -privatize_symbol_name (symtab_node *node) > +privatize_symbol_name_1 (symtab_node *node, tree decl) > { > - tree decl = node->decl; > - const char *name; > - cgraph_node *cnode = dyn_cast <cgraph_node *> (node); > - > - /* If we want to privatize instrumentation clone > - then we need to change original function name > - which is used via transparent alias chain. */ > - if (cnode && cnode->instrumentation_clone) > - decl = cnode->orig_decl; > - > - name = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl)); > + const char *name = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl)); > > if (must_not_rename (node, name)) > return false; > @@ -906,31 +892,57 @@ privatize_symbol_name (symtab_node *node) > symtab->change_decl_assembler_name (decl, > clone_function_name_1 (name, > "lto_priv")); > + > if (node->lto_file_data) > lto_record_renamed_decl (node->lto_file_data, name, > IDENTIFIER_POINTER > (DECL_ASSEMBLER_NAME (decl))); > + > + if (symtab->dump_file) > + fprintf (symtab->dump_file, > + "Privatizing symbol name: %s -> %s\n", > + name, IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl))); > + > + return true; > +} > + > +/* Mangle NODE symbol name into a local name. > + This is necessary to do > + 1) if two or more static vars of same assembler name > + are merged into single ltrans unit. > + 2) if previously static var was promoted hidden to avoid possible conflict > + with symbols defined out of the LTO world. */ > + > +static bool > +privatize_symbol_name (symtab_node *node) > +{ > + if (!privatize_symbol_name_1 (node, node->decl)) > + return false; > + > /* We could change name which is a target of transparent alias > chain of instrumented function name. Fix alias chain if so .*/ > - if (cnode) > + if (cgraph_node *cnode = dyn_cast <cgraph_node *> (node)) > { > tree iname = NULL_TREE; > if (cnode->instrumentation_clone) > - iname = DECL_ASSEMBLER_NAME (cnode->decl); > + { > + /* If we want to privatize instrumentation clone > + then we also need to privatize original function. */ > + if (cnode->instrumented_version) > + privatize_symbol_name (cnode->instrumented_version); > + else > + privatize_symbol_name_1 (cnode, cnode->orig_decl); > + iname = DECL_ASSEMBLER_NAME (cnode->decl); > + TREE_CHAIN (iname) = DECL_ASSEMBLER_NAME (cnode->orig_decl); > + } > else if (cnode->instrumented_version > - && cnode->instrumented_version->orig_decl == decl) > - iname = DECL_ASSEMBLER_NAME (cnode->instrumented_version->decl); > - > - if (iname) > + && cnode->instrumented_version->orig_decl == cnode->decl) > { > - gcc_assert (IDENTIFIER_TRANSPARENT_ALIAS (iname)); > - TREE_CHAIN (iname) = DECL_ASSEMBLER_NAME (decl); > + iname = DECL_ASSEMBLER_NAME (cnode->instrumented_version->decl); > + TREE_CHAIN (iname) = DECL_ASSEMBLER_NAME (cnode->decl); > } > } > - if (symtab->dump_file) > - fprintf (symtab->dump_file, > - "Privatizing symbol name: %s -> %s\n", > - name, IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl))); > + > return true; > } > > diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_0.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_0.c > new file mode 100644 > index 0000000..2054aa15 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_0.c > @@ -0,0 +1,17 @@ > +/* { dg-lto-do link } */ > +/* { dg-require-effective-target mpx } */ > +/* { dg-lto-options { { -Ofast -flto -fcheck-pointer-bounds -mmpx } } } */ > + > +extern int __attribute__((noinline)) f1 (int i); > + > +static int __attribute__((noinline)) > +f2 (int i) > +{ > + return i + 6; > +} > + > +int > +main (int argc, char **argv) > +{ > + return f1 (argc) + f2 (argc); > +} > diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_1.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_1.c > new file mode 100644 > index 0000000..4fa8656 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_1.c > @@ -0,0 +1,11 @@ > +static int __attribute__((noinline)) > +f2 (int i) > +{ > + return 2 * i; > +} > + > +int __attribute__((noinline)) > +f1 (int i) > +{ > + return f2 (i) + 10; > +} > diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_0.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_0.c > new file mode 100644 > index 0000000..be7f601 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_0.c > @@ -0,0 +1,18 @@ > +/* { dg-lto-do link } */ > +/* { dg-require-effective-target mpx } */ > +/* { dg-lto-options { { -Ofast -flto -fcheck-pointer-bounds -mmpx } } } */ > + > +static int > +__attribute__ ((noinline)) > +func1 (int i) > +{ > + return i + 2; > +} > + > +extern int func2 (int i); > + > +int > +main (int argc, char **argv) > +{ > + return func1 (argc) + func2 (argc) + 1; > +} > diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_1.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_1.c > new file mode 100644 > index 0000000..db39e7f > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_1.c > @@ -0,0 +1,8 @@ > +int func1 = 10; > + > +int > +func2 (int i) > +{ > + func1++; > + return i + func1; > +}
Ping 2015-05-05 11:06 GMT+03:00 Ilya Enkovich <enkovich.gnu@gmail.com>: > Ping > > 2015-04-14 12:14 GMT+03:00 Ilya Enkovich <enkovich.gnu@gmail.com>: >> On 10 Apr 03:15, Jan Hubicka wrote: >>> > >>> > References are not streamed out for nodes which are referenced in a >>> > partition but don't belong to it ('continue' condition in output_refs >>> > loop). >>> >>> Yeah, but it already special cases aliases (because we now always preserve IPA_REF_ALIAS link >>> in the boundary, so I think making IPA_REF_CHKP to be rpeserved should go the same path) >>> so we can special case the instrumentation thunks too? >> >> Any cgraph_node having instrumented clone should have it, not only instrumentation thunks. Surely we can make an exception for IPA_REF_CHKP. >> >>> > >>> > > >>> > > I suppose we still need to >>> > > 1) write_symbol and lto-symtab should know that transparent aliases are not real >>> > > symbols and ignore them. We have predicate symtab_node::real_symbol_p, >>> > > I suppose it should return true. >>> > > >>> > > I think cgraph_merge and varpool_merge in lto-symtab needs to know what to do >>> > > when merging two symbols with transparent aliases. >>> > > 2) At some point we need to populate transparent alias links as discussed in the >>> > > other thread. >>> > > 3) For safety, I guess we want symbol_table::change_decl_assembler_name to either >>> > > sanity check there are no trasparent alias links pointing to old assembler >>> > > names or update it. >>> > >>> > Wouldn't search for possible referring via transparent aliases nodes >>> > be too expensive? >>> >>> Changing of decl_assembler_name is not very common and if we set up the links >>> only after renaming of statics, i think we are safe. But it would be better to >>> keep verify it at some point. >>> >>> I suppose verify_node check that there is no transparent alias link on symbols >>> were it is not supposed to be and that there is link when it is supposed to be (i.e. >>> symtab_state is >=EXPANSION and symbol is either weakref on tragets w/o native weakrefs >>> or instrumentation cone. >>> >>> Would you please mind adding the test and making sure it triggers when things are >>> out of sync? >>> >> >> OK, but I suppose it should be a part of transparent alias links rework discussed in another thread. BTW are you going to intoroduce transparent aliases in cgraph soon? >> >>> > >>> > > 4) I think we want verify_node to check that transparent alias links and chkp points >>> > > as intended and only on those symbols where they need to be >>> > > >>> > > There is also logic in lto-partition to always insert alias target >>> > >> > OK, so you have the chkp references that links to instrumented_version. >>> > >> > You do not stream them for boundary symbols but for some reason they are needed, >>> > >> > but when you can end up with wrong CHKP link? >>> > >> >>> > >> Wrong links may appear when cgraph nodes are merged. >>> > > >>> > > I see, I think you really want to fix these at merging time as sugested in 1). >>> > > In this case even the node->instrumented_version pointer may be out of date and >>> > > point to a node that lost in merging? >>> > >>> > node->instrumented_version is redirected and never points to lost >>> > symbol. But during merge we may have situations when >>> > (node->instrumented_version->instrumented_version != node) because of >>> > multiple not merged (yet) symbols referencing the same merged target. >>> >>> OK, which should not be a problem if we stream the links instead of rebuilding them, right? >> >> IPA_REF_CHKP refs don't affect instrumented_version merging. And I actually don't see here any problems, instrumented_version links always come into consistent state when symbol merging finishes. >> >> >> Here is a new patch version. It has no chkp_maybe_fix_chkp_ref function, IPA_REF_CHKP references are streamed out instead. Bootstrapped and tested on x86_64-unknown-linux-gnu. OK for trunk? I also want to port it to GCC 5 branch after release. >> >> Thanks, >> Ilya >> -- >> gcc/ >> >> 2015-04-14 Ilya Enkovich <ilya.enkovich@intel.com> >> >> * ipa.c (symbol_table::remove_unreachable_nodes): Don't >> remove instumentation thunks calling reachable functions. >> * lto-cgraph.c (output_refs): Always output IPA_REF_CHKP. >> * lto/lto-partition.c (privatize_symbol_name_1): New. >> (privatize_symbol_name): Privatize both decl and orig_decl >> names for instrumented functions. >> >> gcc/testsuite/ >> >> 2015-04-14 Ilya Enkovich <ilya.enkovich@intel.com> >> >> * gcc.dg/lto/chkp-privatize-1_0.c: New. >> * gcc.dg/lto/chkp-privatize-1_1.c: New. >> * gcc.dg/lto/chkp-privatize-2_0.c: New. >> * gcc.dg/lto/chkp-privatize-2_1.c: New. >> >> >> diff --git a/gcc/ipa.c b/gcc/ipa.c >> index b3752de..3054afe 100644 >> --- a/gcc/ipa.c >> +++ b/gcc/ipa.c >> @@ -492,7 +492,22 @@ symbol_table::remove_unreachable_nodes (FILE *file) >> } >> else if (cnode->thunk.thunk_p) >> enqueue_node (cnode->callees->callee, &first, &reachable); >> - >> + >> + /* For instrumentation clones we always need original >> + function node for proper LTO privatization. */ >> + if (cnode->instrumentation_clone >> + && reachable.contains (cnode) >> + && cnode->definition) >> + { >> + gcc_assert (cnode->instrumented_version || in_lto_p); >> + if (cnode->instrumented_version) >> + { >> + enqueue_node (cnode->instrumented_version, &first, >> + &reachable); >> + reachable.add (cnode->instrumented_version); >> + } >> + } >> + >> /* If any reachable function has simd clones, mark them as >> reachable as well. */ >> if (cnode->simd_clones) >> diff --git a/gcc/lto-cgraph.c b/gcc/lto-cgraph.c >> index ac50e4b..ea352f1 100644 >> --- a/gcc/lto-cgraph.c >> +++ b/gcc/lto-cgraph.c >> @@ -805,8 +805,33 @@ output_refs (lto_symtab_encoder_t encoder) >> { >> symtab_node *node = lto_symtab_encoder_deref (encoder, i); >> >> + /* IPA_REF_ALIAS and IPA_REF_CHKP references are always preserved >> + in the boundary. Alias node can't have other references and >> + can be always handled as if it's not in the boundary. */ >> if (!node->alias && !lto_symtab_encoder_in_partition_p (encoder, node)) >> - continue; >> + { >> + cgraph_node *cnode = dyn_cast <cgraph_node *> (node); >> + /* Output IPA_REF_CHKP reference. */ >> + if (cnode >> + && cnode->instrumented_version >> + && !cnode->instrumentation_clone) >> + { >> + for (int i = 0; node->iterate_reference (i, ref); i++) >> + if (ref->use == IPA_REF_CHKP) >> + { >> + if (lto_symtab_encoder_lookup (encoder, ref->referred) >> + != LCC_NOT_FOUND) >> + { >> + int nref = lto_symtab_encoder_lookup (encoder, node); >> + streamer_write_gcov_count_stream (ob->main_stream, 1); >> + streamer_write_uhwi_stream (ob->main_stream, nref); >> + lto_output_ref (ob, ref, encoder); >> + } >> + break; >> + } >> + } >> + continue; >> + } >> >> count = node->ref_list.nreferences (); >> if (count) >> diff --git a/gcc/lto/lto-partition.c b/gcc/lto/lto-partition.c >> index 235b735..7d117e9 100644 >> --- a/gcc/lto/lto-partition.c >> +++ b/gcc/lto/lto-partition.c >> @@ -877,27 +877,13 @@ validize_symbol_for_target (symtab_node *node) >> } >> } >> >> -/* Mangle NODE symbol name into a local name. >> - This is necessary to do >> - 1) if two or more static vars of same assembler name >> - are merged into single ltrans unit. >> - 2) if previously static var was promoted hidden to avoid possible conflict >> - with symbols defined out of the LTO world. */ >> +/* Helper for privatize_symbol_name. Mangle NODE symbol name >> + represented by DECL. */ >> >> static bool >> -privatize_symbol_name (symtab_node *node) >> +privatize_symbol_name_1 (symtab_node *node, tree decl) >> { >> - tree decl = node->decl; >> - const char *name; >> - cgraph_node *cnode = dyn_cast <cgraph_node *> (node); >> - >> - /* If we want to privatize instrumentation clone >> - then we need to change original function name >> - which is used via transparent alias chain. */ >> - if (cnode && cnode->instrumentation_clone) >> - decl = cnode->orig_decl; >> - >> - name = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl)); >> + const char *name = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl)); >> >> if (must_not_rename (node, name)) >> return false; >> @@ -906,31 +892,57 @@ privatize_symbol_name (symtab_node *node) >> symtab->change_decl_assembler_name (decl, >> clone_function_name_1 (name, >> "lto_priv")); >> + >> if (node->lto_file_data) >> lto_record_renamed_decl (node->lto_file_data, name, >> IDENTIFIER_POINTER >> (DECL_ASSEMBLER_NAME (decl))); >> + >> + if (symtab->dump_file) >> + fprintf (symtab->dump_file, >> + "Privatizing symbol name: %s -> %s\n", >> + name, IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl))); >> + >> + return true; >> +} >> + >> +/* Mangle NODE symbol name into a local name. >> + This is necessary to do >> + 1) if two or more static vars of same assembler name >> + are merged into single ltrans unit. >> + 2) if previously static var was promoted hidden to avoid possible conflict >> + with symbols defined out of the LTO world. */ >> + >> +static bool >> +privatize_symbol_name (symtab_node *node) >> +{ >> + if (!privatize_symbol_name_1 (node, node->decl)) >> + return false; >> + >> /* We could change name which is a target of transparent alias >> chain of instrumented function name. Fix alias chain if so .*/ >> - if (cnode) >> + if (cgraph_node *cnode = dyn_cast <cgraph_node *> (node)) >> { >> tree iname = NULL_TREE; >> if (cnode->instrumentation_clone) >> - iname = DECL_ASSEMBLER_NAME (cnode->decl); >> + { >> + /* If we want to privatize instrumentation clone >> + then we also need to privatize original function. */ >> + if (cnode->instrumented_version) >> + privatize_symbol_name (cnode->instrumented_version); >> + else >> + privatize_symbol_name_1 (cnode, cnode->orig_decl); >> + iname = DECL_ASSEMBLER_NAME (cnode->decl); >> + TREE_CHAIN (iname) = DECL_ASSEMBLER_NAME (cnode->orig_decl); >> + } >> else if (cnode->instrumented_version >> - && cnode->instrumented_version->orig_decl == decl) >> - iname = DECL_ASSEMBLER_NAME (cnode->instrumented_version->decl); >> - >> - if (iname) >> + && cnode->instrumented_version->orig_decl == cnode->decl) >> { >> - gcc_assert (IDENTIFIER_TRANSPARENT_ALIAS (iname)); >> - TREE_CHAIN (iname) = DECL_ASSEMBLER_NAME (decl); >> + iname = DECL_ASSEMBLER_NAME (cnode->instrumented_version->decl); >> + TREE_CHAIN (iname) = DECL_ASSEMBLER_NAME (cnode->decl); >> } >> } >> - if (symtab->dump_file) >> - fprintf (symtab->dump_file, >> - "Privatizing symbol name: %s -> %s\n", >> - name, IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl))); >> + >> return true; >> } >> >> diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_0.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_0.c >> new file mode 100644 >> index 0000000..2054aa15 >> --- /dev/null >> +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_0.c >> @@ -0,0 +1,17 @@ >> +/* { dg-lto-do link } */ >> +/* { dg-require-effective-target mpx } */ >> +/* { dg-lto-options { { -Ofast -flto -fcheck-pointer-bounds -mmpx } } } */ >> + >> +extern int __attribute__((noinline)) f1 (int i); >> + >> +static int __attribute__((noinline)) >> +f2 (int i) >> +{ >> + return i + 6; >> +} >> + >> +int >> +main (int argc, char **argv) >> +{ >> + return f1 (argc) + f2 (argc); >> +} >> diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_1.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_1.c >> new file mode 100644 >> index 0000000..4fa8656 >> --- /dev/null >> +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_1.c >> @@ -0,0 +1,11 @@ >> +static int __attribute__((noinline)) >> +f2 (int i) >> +{ >> + return 2 * i; >> +} >> + >> +int __attribute__((noinline)) >> +f1 (int i) >> +{ >> + return f2 (i) + 10; >> +} >> diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_0.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_0.c >> new file mode 100644 >> index 0000000..be7f601 >> --- /dev/null >> +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_0.c >> @@ -0,0 +1,18 @@ >> +/* { dg-lto-do link } */ >> +/* { dg-require-effective-target mpx } */ >> +/* { dg-lto-options { { -Ofast -flto -fcheck-pointer-bounds -mmpx } } } */ >> + >> +static int >> +__attribute__ ((noinline)) >> +func1 (int i) >> +{ >> + return i + 2; >> +} >> + >> +extern int func2 (int i); >> + >> +int >> +main (int argc, char **argv) >> +{ >> + return func1 (argc) + func2 (argc) + 1; >> +} >> diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_1.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_1.c >> new file mode 100644 >> index 0000000..db39e7f >> --- /dev/null >> +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_1.c >> @@ -0,0 +1,8 @@ >> +int func1 = 10; >> + >> +int >> +func2 (int i) >> +{ >> + func1++; >> + return i + func1; >> +}
Ping 2015-05-19 12:40 GMT+03:00 Ilya Enkovich <enkovich.gnu@gmail.com>: > Ping > > 2015-05-05 11:06 GMT+03:00 Ilya Enkovich <enkovich.gnu@gmail.com>: >> Ping >> >> 2015-04-14 12:14 GMT+03:00 Ilya Enkovich <enkovich.gnu@gmail.com>: >>> On 10 Apr 03:15, Jan Hubicka wrote: >>>> > >>>> > References are not streamed out for nodes which are referenced in a >>>> > partition but don't belong to it ('continue' condition in output_refs >>>> > loop). >>>> >>>> Yeah, but it already special cases aliases (because we now always preserve IPA_REF_ALIAS link >>>> in the boundary, so I think making IPA_REF_CHKP to be rpeserved should go the same path) >>>> so we can special case the instrumentation thunks too? >>> >>> Any cgraph_node having instrumented clone should have it, not only instrumentation thunks. Surely we can make an exception for IPA_REF_CHKP. >>> >>>> > >>>> > > >>>> > > I suppose we still need to >>>> > > 1) write_symbol and lto-symtab should know that transparent aliases are not real >>>> > > symbols and ignore them. We have predicate symtab_node::real_symbol_p, >>>> > > I suppose it should return true. >>>> > > >>>> > > I think cgraph_merge and varpool_merge in lto-symtab needs to know what to do >>>> > > when merging two symbols with transparent aliases. >>>> > > 2) At some point we need to populate transparent alias links as discussed in the >>>> > > other thread. >>>> > > 3) For safety, I guess we want symbol_table::change_decl_assembler_name to either >>>> > > sanity check there are no trasparent alias links pointing to old assembler >>>> > > names or update it. >>>> > >>>> > Wouldn't search for possible referring via transparent aliases nodes >>>> > be too expensive? >>>> >>>> Changing of decl_assembler_name is not very common and if we set up the links >>>> only after renaming of statics, i think we are safe. But it would be better to >>>> keep verify it at some point. >>>> >>>> I suppose verify_node check that there is no transparent alias link on symbols >>>> were it is not supposed to be and that there is link when it is supposed to be (i.e. >>>> symtab_state is >=EXPANSION and symbol is either weakref on tragets w/o native weakrefs >>>> or instrumentation cone. >>>> >>>> Would you please mind adding the test and making sure it triggers when things are >>>> out of sync? >>>> >>> >>> OK, but I suppose it should be a part of transparent alias links rework discussed in another thread. BTW are you going to intoroduce transparent aliases in cgraph soon? >>> >>>> > >>>> > > 4) I think we want verify_node to check that transparent alias links and chkp points >>>> > > as intended and only on those symbols where they need to be >>>> > > >>>> > > There is also logic in lto-partition to always insert alias target >>>> > >> > OK, so you have the chkp references that links to instrumented_version. >>>> > >> > You do not stream them for boundary symbols but for some reason they are needed, >>>> > >> > but when you can end up with wrong CHKP link? >>>> > >> >>>> > >> Wrong links may appear when cgraph nodes are merged. >>>> > > >>>> > > I see, I think you really want to fix these at merging time as sugested in 1). >>>> > > In this case even the node->instrumented_version pointer may be out of date and >>>> > > point to a node that lost in merging? >>>> > >>>> > node->instrumented_version is redirected and never points to lost >>>> > symbol. But during merge we may have situations when >>>> > (node->instrumented_version->instrumented_version != node) because of >>>> > multiple not merged (yet) symbols referencing the same merged target. >>>> >>>> OK, which should not be a problem if we stream the links instead of rebuilding them, right? >>> >>> IPA_REF_CHKP refs don't affect instrumented_version merging. And I actually don't see here any problems, instrumented_version links always come into consistent state when symbol merging finishes. >>> >>> >>> Here is a new patch version. It has no chkp_maybe_fix_chkp_ref function, IPA_REF_CHKP references are streamed out instead. Bootstrapped and tested on x86_64-unknown-linux-gnu. OK for trunk? I also want to port it to GCC 5 branch after release. >>> >>> Thanks, >>> Ilya >>> -- >>> gcc/ >>> >>> 2015-04-14 Ilya Enkovich <ilya.enkovich@intel.com> >>> >>> * ipa.c (symbol_table::remove_unreachable_nodes): Don't >>> remove instumentation thunks calling reachable functions. >>> * lto-cgraph.c (output_refs): Always output IPA_REF_CHKP. >>> * lto/lto-partition.c (privatize_symbol_name_1): New. >>> (privatize_symbol_name): Privatize both decl and orig_decl >>> names for instrumented functions. >>> >>> gcc/testsuite/ >>> >>> 2015-04-14 Ilya Enkovich <ilya.enkovich@intel.com> >>> >>> * gcc.dg/lto/chkp-privatize-1_0.c: New. >>> * gcc.dg/lto/chkp-privatize-1_1.c: New. >>> * gcc.dg/lto/chkp-privatize-2_0.c: New. >>> * gcc.dg/lto/chkp-privatize-2_1.c: New. >>> >>> >>> diff --git a/gcc/ipa.c b/gcc/ipa.c >>> index b3752de..3054afe 100644 >>> --- a/gcc/ipa.c >>> +++ b/gcc/ipa.c >>> @@ -492,7 +492,22 @@ symbol_table::remove_unreachable_nodes (FILE *file) >>> } >>> else if (cnode->thunk.thunk_p) >>> enqueue_node (cnode->callees->callee, &first, &reachable); >>> - >>> + >>> + /* For instrumentation clones we always need original >>> + function node for proper LTO privatization. */ >>> + if (cnode->instrumentation_clone >>> + && reachable.contains (cnode) >>> + && cnode->definition) >>> + { >>> + gcc_assert (cnode->instrumented_version || in_lto_p); >>> + if (cnode->instrumented_version) >>> + { >>> + enqueue_node (cnode->instrumented_version, &first, >>> + &reachable); >>> + reachable.add (cnode->instrumented_version); >>> + } >>> + } >>> + >>> /* If any reachable function has simd clones, mark them as >>> reachable as well. */ >>> if (cnode->simd_clones) >>> diff --git a/gcc/lto-cgraph.c b/gcc/lto-cgraph.c >>> index ac50e4b..ea352f1 100644 >>> --- a/gcc/lto-cgraph.c >>> +++ b/gcc/lto-cgraph.c >>> @@ -805,8 +805,33 @@ output_refs (lto_symtab_encoder_t encoder) >>> { >>> symtab_node *node = lto_symtab_encoder_deref (encoder, i); >>> >>> + /* IPA_REF_ALIAS and IPA_REF_CHKP references are always preserved >>> + in the boundary. Alias node can't have other references and >>> + can be always handled as if it's not in the boundary. */ >>> if (!node->alias && !lto_symtab_encoder_in_partition_p (encoder, node)) >>> - continue; >>> + { >>> + cgraph_node *cnode = dyn_cast <cgraph_node *> (node); >>> + /* Output IPA_REF_CHKP reference. */ >>> + if (cnode >>> + && cnode->instrumented_version >>> + && !cnode->instrumentation_clone) >>> + { >>> + for (int i = 0; node->iterate_reference (i, ref); i++) >>> + if (ref->use == IPA_REF_CHKP) >>> + { >>> + if (lto_symtab_encoder_lookup (encoder, ref->referred) >>> + != LCC_NOT_FOUND) >>> + { >>> + int nref = lto_symtab_encoder_lookup (encoder, node); >>> + streamer_write_gcov_count_stream (ob->main_stream, 1); >>> + streamer_write_uhwi_stream (ob->main_stream, nref); >>> + lto_output_ref (ob, ref, encoder); >>> + } >>> + break; >>> + } >>> + } >>> + continue; >>> + } >>> >>> count = node->ref_list.nreferences (); >>> if (count) >>> diff --git a/gcc/lto/lto-partition.c b/gcc/lto/lto-partition.c >>> index 235b735..7d117e9 100644 >>> --- a/gcc/lto/lto-partition.c >>> +++ b/gcc/lto/lto-partition.c >>> @@ -877,27 +877,13 @@ validize_symbol_for_target (symtab_node *node) >>> } >>> } >>> >>> -/* Mangle NODE symbol name into a local name. >>> - This is necessary to do >>> - 1) if two or more static vars of same assembler name >>> - are merged into single ltrans unit. >>> - 2) if previously static var was promoted hidden to avoid possible conflict >>> - with symbols defined out of the LTO world. */ >>> +/* Helper for privatize_symbol_name. Mangle NODE symbol name >>> + represented by DECL. */ >>> >>> static bool >>> -privatize_symbol_name (symtab_node *node) >>> +privatize_symbol_name_1 (symtab_node *node, tree decl) >>> { >>> - tree decl = node->decl; >>> - const char *name; >>> - cgraph_node *cnode = dyn_cast <cgraph_node *> (node); >>> - >>> - /* If we want to privatize instrumentation clone >>> - then we need to change original function name >>> - which is used via transparent alias chain. */ >>> - if (cnode && cnode->instrumentation_clone) >>> - decl = cnode->orig_decl; >>> - >>> - name = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl)); >>> + const char *name = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl)); >>> >>> if (must_not_rename (node, name)) >>> return false; >>> @@ -906,31 +892,57 @@ privatize_symbol_name (symtab_node *node) >>> symtab->change_decl_assembler_name (decl, >>> clone_function_name_1 (name, >>> "lto_priv")); >>> + >>> if (node->lto_file_data) >>> lto_record_renamed_decl (node->lto_file_data, name, >>> IDENTIFIER_POINTER >>> (DECL_ASSEMBLER_NAME (decl))); >>> + >>> + if (symtab->dump_file) >>> + fprintf (symtab->dump_file, >>> + "Privatizing symbol name: %s -> %s\n", >>> + name, IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl))); >>> + >>> + return true; >>> +} >>> + >>> +/* Mangle NODE symbol name into a local name. >>> + This is necessary to do >>> + 1) if two or more static vars of same assembler name >>> + are merged into single ltrans unit. >>> + 2) if previously static var was promoted hidden to avoid possible conflict >>> + with symbols defined out of the LTO world. */ >>> + >>> +static bool >>> +privatize_symbol_name (symtab_node *node) >>> +{ >>> + if (!privatize_symbol_name_1 (node, node->decl)) >>> + return false; >>> + >>> /* We could change name which is a target of transparent alias >>> chain of instrumented function name. Fix alias chain if so .*/ >>> - if (cnode) >>> + if (cgraph_node *cnode = dyn_cast <cgraph_node *> (node)) >>> { >>> tree iname = NULL_TREE; >>> if (cnode->instrumentation_clone) >>> - iname = DECL_ASSEMBLER_NAME (cnode->decl); >>> + { >>> + /* If we want to privatize instrumentation clone >>> + then we also need to privatize original function. */ >>> + if (cnode->instrumented_version) >>> + privatize_symbol_name (cnode->instrumented_version); >>> + else >>> + privatize_symbol_name_1 (cnode, cnode->orig_decl); >>> + iname = DECL_ASSEMBLER_NAME (cnode->decl); >>> + TREE_CHAIN (iname) = DECL_ASSEMBLER_NAME (cnode->orig_decl); >>> + } >>> else if (cnode->instrumented_version >>> - && cnode->instrumented_version->orig_decl == decl) >>> - iname = DECL_ASSEMBLER_NAME (cnode->instrumented_version->decl); >>> - >>> - if (iname) >>> + && cnode->instrumented_version->orig_decl == cnode->decl) >>> { >>> - gcc_assert (IDENTIFIER_TRANSPARENT_ALIAS (iname)); >>> - TREE_CHAIN (iname) = DECL_ASSEMBLER_NAME (decl); >>> + iname = DECL_ASSEMBLER_NAME (cnode->instrumented_version->decl); >>> + TREE_CHAIN (iname) = DECL_ASSEMBLER_NAME (cnode->decl); >>> } >>> } >>> - if (symtab->dump_file) >>> - fprintf (symtab->dump_file, >>> - "Privatizing symbol name: %s -> %s\n", >>> - name, IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl))); >>> + >>> return true; >>> } >>> >>> diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_0.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_0.c >>> new file mode 100644 >>> index 0000000..2054aa15 >>> --- /dev/null >>> +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_0.c >>> @@ -0,0 +1,17 @@ >>> +/* { dg-lto-do link } */ >>> +/* { dg-require-effective-target mpx } */ >>> +/* { dg-lto-options { { -Ofast -flto -fcheck-pointer-bounds -mmpx } } } */ >>> + >>> +extern int __attribute__((noinline)) f1 (int i); >>> + >>> +static int __attribute__((noinline)) >>> +f2 (int i) >>> +{ >>> + return i + 6; >>> +} >>> + >>> +int >>> +main (int argc, char **argv) >>> +{ >>> + return f1 (argc) + f2 (argc); >>> +} >>> diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_1.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_1.c >>> new file mode 100644 >>> index 0000000..4fa8656 >>> --- /dev/null >>> +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_1.c >>> @@ -0,0 +1,11 @@ >>> +static int __attribute__((noinline)) >>> +f2 (int i) >>> +{ >>> + return 2 * i; >>> +} >>> + >>> +int __attribute__((noinline)) >>> +f1 (int i) >>> +{ >>> + return f2 (i) + 10; >>> +} >>> diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_0.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_0.c >>> new file mode 100644 >>> index 0000000..be7f601 >>> --- /dev/null >>> +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_0.c >>> @@ -0,0 +1,18 @@ >>> +/* { dg-lto-do link } */ >>> +/* { dg-require-effective-target mpx } */ >>> +/* { dg-lto-options { { -Ofast -flto -fcheck-pointer-bounds -mmpx } } } */ >>> + >>> +static int >>> +__attribute__ ((noinline)) >>> +func1 (int i) >>> +{ >>> + return i + 2; >>> +} >>> + >>> +extern int func2 (int i); >>> + >>> +int >>> +main (int argc, char **argv) >>> +{ >>> + return func1 (argc) + func2 (argc) + 1; >>> +} >>> diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_1.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_1.c >>> new file mode 100644 >>> index 0000000..db39e7f >>> --- /dev/null >>> +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_1.c >>> @@ -0,0 +1,8 @@ >>> +int func1 = 10; >>> + >>> +int >>> +func2 (int i) >>> +{ >>> + func1++; >>> + return i + func1; >>> +}
> Ping I am really sorry for ignoring this so long - I would like to reorg the code and replace instrumentaiton thunks by the notion of transparent aliases, but did not have time to do that yet. Have quite busy time now. > >>> > >>> 2015-04-14 Ilya Enkovich <ilya.enkovich@intel.com> > >>> > >>> * ipa.c (symbol_table::remove_unreachable_nodes): Don't > >>> remove instumentation thunks calling reachable functions. > >>> * lto-cgraph.c (output_refs): Always output IPA_REF_CHKP. > >>> * lto/lto-partition.c (privatize_symbol_name_1): New. > >>> (privatize_symbol_name): Privatize both decl and orig_decl > >>> names for instrumented functions. > >>> > >>> gcc/testsuite/ > >>> > >>> 2015-04-14 Ilya Enkovich <ilya.enkovich@intel.com> > >>> > >>> * gcc.dg/lto/chkp-privatize-1_0.c: New. > >>> * gcc.dg/lto/chkp-privatize-1_1.c: New. > >>> * gcc.dg/lto/chkp-privatize-2_0.c: New. > >>> * gcc.dg/lto/chkp-privatize-2_1.c: New. > >>> > >>> > >>> diff --git a/gcc/ipa.c b/gcc/ipa.c > >>> index b3752de..3054afe 100644 > >>> --- a/gcc/ipa.c > >>> +++ b/gcc/ipa.c > >>> @@ -492,7 +492,22 @@ symbol_table::remove_unreachable_nodes (FILE *file) > >>> } > >>> else if (cnode->thunk.thunk_p) > >>> enqueue_node (cnode->callees->callee, &first, &reachable); > >>> - > >>> + > >>> + /* For instrumentation clones we always need original > >>> + function node for proper LTO privatization. */ > >>> + if (cnode->instrumentation_clone > >>> + && reachable.contains (cnode) reachable.contains (cnode) is !in_boundary_p. > >>> + && cnode->definition) > >>> + { > >>> + gcc_assert (cnode->instrumented_version || in_lto_p); > >>> + if (cnode->instrumented_version) > >>> + { > >>> + enqueue_node (cnode->instrumented_version, &first, > >>> + &reachable); > >>> + reachable.add (cnode->instrumented_version); Why do you need the other tests. Can we have instrumentation node that is not definition? I suppose you can remove if (cnode->instrumented_version) because you assert it anyway. > >>> - if (cnode) > >>> + if (cgraph_node *cnode = dyn_cast <cgraph_node *> (node)) > >>> { > >>> tree iname = NULL_TREE; > >>> if (cnode->instrumentation_clone) > >>> - iname = DECL_ASSEMBLER_NAME (cnode->decl); > >>> + { > >>> + /* If we want to privatize instrumentation clone > >>> + then we also need to privatize original function. */ > >>> + if (cnode->instrumented_version) > >>> + privatize_symbol_name (cnode->instrumented_version); > >>> + else > >>> + privatize_symbol_name_1 (cnode, cnode->orig_decl); > >>> + iname = DECL_ASSEMBLER_NAME (cnode->decl); > >>> + TREE_CHAIN (iname) = DECL_ASSEMBLER_NAME (cnode->orig_decl); > >>> + } > >>> else if (cnode->instrumented_version > >>> - && cnode->instrumented_version->orig_decl == decl) > >>> - iname = DECL_ASSEMBLER_NAME (cnode->instrumented_version->decl); > >>> - > >>> - if (iname) > >>> + && cnode->instrumented_version->orig_decl == cnode->decl) > >>> { > >>> - gcc_assert (IDENTIFIER_TRANSPARENT_ALIAS (iname)); > >>> - TREE_CHAIN (iname) = DECL_ASSEMBLER_NAME (decl); > >>> + iname = DECL_ASSEMBLER_NAME (cnode->instrumented_version->decl); > >>> + TREE_CHAIN (iname) = DECL_ASSEMBLER_NAME (cnode->decl); > >>> } > >>> } > >>> - if (symtab->dump_file) > >>> - fprintf (symtab->dump_file, > >>> - "Privatizing symbol name: %s -> %s\n", > >>> - name, IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl))); > >>> + I think we ought to have verify_symtab_node checking for this. All the handling of the name links seems somewhat fragile (I am mostly concerned about getting it right for LTO before remaning takes place) OK with the changes above for mainline and for branch if it does not cause problems for a week. Honza > >>> return true; > >>> } > >>> > >>> diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_0.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_0.c > >>> new file mode 100644 > >>> index 0000000..2054aa15 > >>> --- /dev/null > >>> +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_0.c > >>> @@ -0,0 +1,17 @@ > >>> +/* { dg-lto-do link } */ > >>> +/* { dg-require-effective-target mpx } */ > >>> +/* { dg-lto-options { { -Ofast -flto -fcheck-pointer-bounds -mmpx } } } */ > >>> + > >>> +extern int __attribute__((noinline)) f1 (int i); > >>> + > >>> +static int __attribute__((noinline)) > >>> +f2 (int i) > >>> +{ > >>> + return i + 6; > >>> +} > >>> + > >>> +int > >>> +main (int argc, char **argv) > >>> +{ > >>> + return f1 (argc) + f2 (argc); > >>> +} > >>> diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_1.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_1.c > >>> new file mode 100644 > >>> index 0000000..4fa8656 > >>> --- /dev/null > >>> +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_1.c > >>> @@ -0,0 +1,11 @@ > >>> +static int __attribute__((noinline)) > >>> +f2 (int i) > >>> +{ > >>> + return 2 * i; > >>> +} > >>> + > >>> +int __attribute__((noinline)) > >>> +f1 (int i) > >>> +{ > >>> + return f2 (i) + 10; > >>> +} > >>> diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_0.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_0.c > >>> new file mode 100644 > >>> index 0000000..be7f601 > >>> --- /dev/null > >>> +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_0.c > >>> @@ -0,0 +1,18 @@ > >>> +/* { dg-lto-do link } */ > >>> +/* { dg-require-effective-target mpx } */ > >>> +/* { dg-lto-options { { -Ofast -flto -fcheck-pointer-bounds -mmpx } } } */ > >>> + > >>> +static int > >>> +__attribute__ ((noinline)) > >>> +func1 (int i) > >>> +{ > >>> + return i + 2; > >>> +} > >>> + > >>> +extern int func2 (int i); > >>> + > >>> +int > >>> +main (int argc, char **argv) > >>> +{ > >>> + return func1 (argc) + func2 (argc) + 1; > >>> +} > >>> diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_1.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_1.c > >>> new file mode 100644 > >>> index 0000000..db39e7f > >>> --- /dev/null > >>> +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_1.c > >>> @@ -0,0 +1,8 @@ > >>> +int func1 = 10; > >>> + > >>> +int > >>> +func2 (int i) > >>> +{ > >>> + func1++; > >>> + return i + func1; > >>> +}
diff --git a/gcc/ipa.c b/gcc/ipa.c index b3752de..3054afe 100644 --- a/gcc/ipa.c +++ b/gcc/ipa.c @@ -492,7 +492,22 @@ symbol_table::remove_unreachable_nodes (FILE *file) } else if (cnode->thunk.thunk_p) enqueue_node (cnode->callees->callee, &first, &reachable); - + + /* For instrumentation clones we always need original + function node for proper LTO privatization. */ + if (cnode->instrumentation_clone + && reachable.contains (cnode) + && cnode->definition) + { + gcc_assert (cnode->instrumented_version || in_lto_p); + if (cnode->instrumented_version) + { + enqueue_node (cnode->instrumented_version, &first, + &reachable); + reachable.add (cnode->instrumented_version); + } + } + /* If any reachable function has simd clones, mark them as reachable as well. */ if (cnode->simd_clones) diff --git a/gcc/lto-cgraph.c b/gcc/lto-cgraph.c index ac50e4b..ea352f1 100644 --- a/gcc/lto-cgraph.c +++ b/gcc/lto-cgraph.c @@ -805,8 +805,33 @@ output_refs (lto_symtab_encoder_t encoder) { symtab_node *node = lto_symtab_encoder_deref (encoder, i); + /* IPA_REF_ALIAS and IPA_REF_CHKP references are always preserved + in the boundary. Alias node can't have other references and + can be always handled as if it's not in the boundary. */ if (!node->alias && !lto_symtab_encoder_in_partition_p (encoder, node)) - continue; + { + cgraph_node *cnode = dyn_cast <cgraph_node *> (node); + /* Output IPA_REF_CHKP reference. */ + if (cnode + && cnode->instrumented_version + && !cnode->instrumentation_clone) + { + for (int i = 0; node->iterate_reference (i, ref); i++) + if (ref->use == IPA_REF_CHKP) + { + if (lto_symtab_encoder_lookup (encoder, ref->referred) + != LCC_NOT_FOUND) + { + int nref = lto_symtab_encoder_lookup (encoder, node); + streamer_write_gcov_count_stream (ob->main_stream, 1); + streamer_write_uhwi_stream (ob->main_stream, nref); + lto_output_ref (ob, ref, encoder); + } + break; + } + } + continue; + } count = node->ref_list.nreferences (); if (count) diff --git a/gcc/lto/lto-partition.c b/gcc/lto/lto-partition.c index 235b735..7d117e9 100644 --- a/gcc/lto/lto-partition.c +++ b/gcc/lto/lto-partition.c @@ -877,27 +877,13 @@ validize_symbol_for_target (symtab_node *node) } } -/* Mangle NODE symbol name into a local name. - This is necessary to do - 1) if two or more static vars of same assembler name - are merged into single ltrans unit. - 2) if previously static var was promoted hidden to avoid possible conflict - with symbols defined out of the LTO world. */ +/* Helper for privatize_symbol_name. Mangle NODE symbol name + represented by DECL. */ static bool -privatize_symbol_name (symtab_node *node) +privatize_symbol_name_1 (symtab_node *node, tree decl) { - tree decl = node->decl; - const char *name; - cgraph_node *cnode = dyn_cast <cgraph_node *> (node); - - /* If we want to privatize instrumentation clone - then we need to change original function name - which is used via transparent alias chain. */ - if (cnode && cnode->instrumentation_clone) - decl = cnode->orig_decl; - - name = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl)); + const char *name = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl)); if (must_not_rename (node, name)) return false; @@ -906,31 +892,57 @@ privatize_symbol_name (symtab_node *node) symtab->change_decl_assembler_name (decl, clone_function_name_1 (name, "lto_priv")); + if (node->lto_file_data) lto_record_renamed_decl (node->lto_file_data, name, IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl))); + + if (symtab->dump_file) + fprintf (symtab->dump_file, + "Privatizing symbol name: %s -> %s\n", + name, IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl))); + + return true; +} + +/* Mangle NODE symbol name into a local name. + This is necessary to do + 1) if two or more static vars of same assembler name + are merged into single ltrans unit. + 2) if previously static var was promoted hidden to avoid possible conflict + with symbols defined out of the LTO world. */ + +static bool +privatize_symbol_name (symtab_node *node) +{ + if (!privatize_symbol_name_1 (node, node->decl)) + return false; + /* We could change name which is a target of transparent alias chain of instrumented function name. Fix alias chain if so .*/ - if (cnode) + if (cgraph_node *cnode = dyn_cast <cgraph_node *> (node)) { tree iname = NULL_TREE; if (cnode->instrumentation_clone) - iname = DECL_ASSEMBLER_NAME (cnode->decl); + { + /* If we want to privatize instrumentation clone + then we also need to privatize original function. */ + if (cnode->instrumented_version) + privatize_symbol_name (cnode->instrumented_version); + else + privatize_symbol_name_1 (cnode, cnode->orig_decl); + iname = DECL_ASSEMBLER_NAME (cnode->decl); + TREE_CHAIN (iname) = DECL_ASSEMBLER_NAME (cnode->orig_decl); + } else if (cnode->instrumented_version - && cnode->instrumented_version->orig_decl == decl) - iname = DECL_ASSEMBLER_NAME (cnode->instrumented_version->decl); - - if (iname) + && cnode->instrumented_version->orig_decl == cnode->decl) { - gcc_assert (IDENTIFIER_TRANSPARENT_ALIAS (iname)); - TREE_CHAIN (iname) = DECL_ASSEMBLER_NAME (decl); + iname = DECL_ASSEMBLER_NAME (cnode->instrumented_version->decl); + TREE_CHAIN (iname) = DECL_ASSEMBLER_NAME (cnode->decl); } } - if (symtab->dump_file) - fprintf (symtab->dump_file, - "Privatizing symbol name: %s -> %s\n", - name, IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl))); + return true; } diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_0.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_0.c new file mode 100644 index 0000000..2054aa15 --- /dev/null +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_0.c @@ -0,0 +1,17 @@ +/* { dg-lto-do link } */ +/* { dg-require-effective-target mpx } */ +/* { dg-lto-options { { -Ofast -flto -fcheck-pointer-bounds -mmpx } } } */ + +extern int __attribute__((noinline)) f1 (int i); + +static int __attribute__((noinline)) +f2 (int i) +{ + return i + 6; +} + +int +main (int argc, char **argv) +{ + return f1 (argc) + f2 (argc); +} diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_1.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_1.c new file mode 100644 index 0000000..4fa8656 --- /dev/null +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-1_1.c @@ -0,0 +1,11 @@ +static int __attribute__((noinline)) +f2 (int i) +{ + return 2 * i; +} + +int __attribute__((noinline)) +f1 (int i) +{ + return f2 (i) + 10; +} diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_0.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_0.c new file mode 100644 index 0000000..be7f601 --- /dev/null +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_0.c @@ -0,0 +1,18 @@ +/* { dg-lto-do link } */ +/* { dg-require-effective-target mpx } */ +/* { dg-lto-options { { -Ofast -flto -fcheck-pointer-bounds -mmpx } } } */ + +static int +__attribute__ ((noinline)) +func1 (int i) +{ + return i + 2; +} + +extern int func2 (int i); + +int +main (int argc, char **argv) +{ + return func1 (argc) + func2 (argc) + 1; +} diff --git a/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_1.c b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_1.c new file mode 100644 index 0000000..db39e7f --- /dev/null +++ b/gcc/testsuite/gcc.dg/lto/chkp-privatize-2_1.c @@ -0,0 +1,8 @@ +int func1 = 10; + +int +func2 (int i) +{ + func1++; + return i + func1; +}