Re: [CHKP, PATCH] Fix LTO cgraph merge for instrumented functions

2015-05-28 Thread Jan Hubicka
 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.enkov...@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.enkov...@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 000..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 000..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 
  

Re: [CHKP, PATCH] Fix LTO cgraph merge for instrumented functions

2015-05-26 Thread Ilya Enkovich
Ping

2015-05-19 12:40 GMT+03:00 Ilya Enkovich enkovich@gmail.com:
 Ping

 2015-05-05 11:06 GMT+03:00 Ilya Enkovich enkovich@gmail.com:
 Ping

 2015-04-14 12:14 GMT+03:00 Ilya Enkovich enkovich@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.enkov...@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.enkov...@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 

Re: [CHKP, PATCH] Fix LTO cgraph merge for instrumented functions

2015-05-19 Thread Ilya Enkovich
Ping

2015-05-05 11:06 GMT+03:00 Ilya Enkovich enkovich@gmail.com:
 Ping

 2015-04-14 12:14 GMT+03:00 Ilya Enkovich enkovich@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.enkov...@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.enkov...@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.  */
 + 

Re: [CHKP, PATCH] Fix LTO cgraph merge for instrumented functions

2015-05-05 Thread Ilya Enkovich
Ping

2015-04-14 12:14 GMT+03:00 Ilya Enkovich enkovich@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.enkov...@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.enkov...@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 

Re: [CHKP, PATCH] Fix LTO cgraph merge for instrumented functions

2015-04-14 Thread Ilya Enkovich
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.enkov...@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.enkov...@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 

Re: [CHKP, PATCH] Fix LTO cgraph merge for instrumented functions

2015-04-06 Thread Ilya Enkovich
2015-04-03 21:49 GMT+03:00 Jan Hubicka hubi...@ucw.cz:
 Assembler name of instrumented function is a transparent alias of
 original function's name.  Alias chains are not taken into account by
 analysis. Thus we see no conflict between instrumented function's name
 and a variable name but emit them with the same assembler name. To
 detect name conflict I keep original function node alive.

 Hmm, so lets see if I understand your situation.  You always have transparent 
 alias TA
 and function F connected by IPA_REF_CHKP and because TA is represented as 
 thunk
 you also have a fake call from TA to F?

 I do not understand how you can miss the link these days.  I extended 
 lto-cgraph to
 always keep thunk and its target in every unit that reffers to thunk. That 
 should
 solve you problem?  How IPA_REF_CHKP can link get lost?

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).


 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?

  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.

Thanks,
Ilya


 Honza

 Thanks
 Ilya


Re: [CHKP, PATCH] Fix LTO cgraph merge for instrumented functions

2015-04-03 Thread Ilya Enkovich
2015-04-02 20:01 GMT+03:00 Jan Hubicka hubi...@ucw.cz:
 Ping
 It would help if you add hubi...@ucw.cz to CC for IPA related patches.

 2015-03-19 11:34 GMT+03:00 Ilya Enkovich enkovich@gmail.com:
  On 12 Mar 13:27, Ilya Enkovich wrote:
  Hi,
 
  Currently cgraph merge has several issues with instrumented code:
   - original function node may be removed = no assembler name conflict is 
  detected between function and variable

 Why do you need to detect this one?

Assembler name of instrumented function is a transparent alias of
original function's name.  Alias chains are not taken into account by
analysis. Thus we see no conflict between instrumented function's name
and a variable name but emit them with the same assembler name. To
detect name conflict I keep original function node alive.

   - only orig_decl name is privatized for instrumented function = node 
  still shares assembler name which causes infinite privatization loop
   - information about changed name is stored in file_data of instrumented 
  node = original section name may be not found for original function
   - chkp reference is not fixed when nodes are merged
 
  This patch should fix theese problems by keeping instrumentation thunks 
  reachable, privatizing both nodes and fixing chkp references.  
  Bootstrapped and tested on x86_64-unknown-linux-gnu.  OK for trunk?

 Next stage1 I definitly hope we will be able to reduce number of special cases
 we need for chck nodes.  I will try to read the code and your design document
 and give it some tought.

Original design didn't take into account several LTO aspect and
additional patches appeared to fix various LTO issues. It would be
nice to improve it with your help. I'll need to update a design
document to reflect recent problems and used fixes.

  2015-03-19  Ilya Enkovich  ilya.enkov...@intel.com
 
  * ipa-chkp.h (chkp_maybe_fix_chkp_ref): New.
  * ipa-chkp.c (chkp_maybe_fix_chkp_ref): New.
  * ipa.c (symbol_table::remove_unreachable_nodes): Don't
  remove instumentation thunks calling reachable functions.
  * lto-cgraph.c: Include ipa-chkp.h.
  (input_symtab): Fix chkp references for boundary nodes.
  * lto/lto-partition.c (privatize_symbol_name_1): New.
  (privatize_symbol_name): Privatize both decl and orig_decl
  names for instrumented functions.
  * lto/lto-symtab.c: Include ipa-chkp.h.
  (lto_cgraph_replace_node): Fix chkp references for merged
  function nodes.
 
  gcc/testsuite/
 
  2015-03-19  Ilya Enkovich  ilya.enkov...@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-chkp.c b/gcc/ipa-chkp.c
  index 0b857ff..7a7fc3c 100644
  --- a/gcc/ipa-chkp.c
  +++ b/gcc/ipa-chkp.c
  @@ -414,6 +414,40 @@ chkp_instrumentable_p (tree fndecl)
 (!fn || !copy_forbidden (fn, fndecl)));
   }
 
  +/* Check NODE has a correct IPA_REF_CHKP reference.
  +   Create a new reference if required.  */
  +
  +void
  +chkp_maybe_fix_chkp_ref (cgraph_node *node)

 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.

Thanks
Ilya


Re: [CHKP, PATCH] Fix LTO cgraph merge for instrumented functions

2015-04-03 Thread Jan Hubicka
 Assembler name of instrumented function is a transparent alias of
 original function's name.  Alias chains are not taken into account by
 analysis. Thus we see no conflict between instrumented function's name
 and a variable name but emit them with the same assembler name. To
 detect name conflict I keep original function node alive.

Hmm, so lets see if I understand your situation.  You always have transparent 
alias TA
and function F connected by IPA_REF_CHKP and because TA is represented as thunk
you also have a fake call from TA to F?

I do not understand how you can miss the link these days.  I extended 
lto-cgraph to
always keep thunk and its target in every unit that reffers to thunk. That 
should
solve you problem?  How IPA_REF_CHKP can link get lost?

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.
 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?

Honza
 
 Thanks
 Ilya


Re: [CHKP, PATCH] Fix LTO cgraph merge for instrumented functions

2015-04-02 Thread Ilya Enkovich
Ping

2015-03-19 11:34 GMT+03:00 Ilya Enkovich enkovich@gmail.com:
 On 12 Mar 13:27, Ilya Enkovich wrote:
 Hi,

 Currently cgraph merge has several issues with instrumented code:
  - original function node may be removed = no assembler name conflict is 
 detected between function and variable
  - only orig_decl name is privatized for instrumented function = node still 
 shares assembler name which causes infinite privatization loop
  - information about changed name is stored in file_data of instrumented 
 node = original section name may be not found for original function
  - chkp reference is not fixed when nodes are merged

 This patch should fix theese problems by keeping instrumentation thunks 
 reachable, privatizing both nodes and fixing chkp references.  Bootstrapped 
 and tested on x86_64-unknown-linux-gnu.  OK for trunk?


 Thanks,
 Ilya

 Here is an updated version where chkp_maybe_fix_chkp_ref also removes 
 duplicating references which may appear during cgraph nodes merge.  
 Bootstrapped and tested on x86_64-unknown-linux-gnu.  OK for trunk?


 Thanks,
 Ilya
 --
 gcc/

 2015-03-19  Ilya Enkovich  ilya.enkov...@intel.com

 * ipa-chkp.h (chkp_maybe_fix_chkp_ref): New.
 * ipa-chkp.c (chkp_maybe_fix_chkp_ref): New.
 * ipa.c (symbol_table::remove_unreachable_nodes): Don't
 remove instumentation thunks calling reachable functions.
 * lto-cgraph.c: Include ipa-chkp.h.
 (input_symtab): Fix chkp references for boundary nodes.
 * lto/lto-partition.c (privatize_symbol_name_1): New.
 (privatize_symbol_name): Privatize both decl and orig_decl
 names for instrumented functions.
 * lto/lto-symtab.c: Include ipa-chkp.h.
 (lto_cgraph_replace_node): Fix chkp references for merged
 function nodes.

 gcc/testsuite/

 2015-03-19  Ilya Enkovich  ilya.enkov...@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-chkp.c b/gcc/ipa-chkp.c
 index 0b857ff..7a7fc3c 100644
 --- a/gcc/ipa-chkp.c
 +++ b/gcc/ipa-chkp.c
 @@ -414,6 +414,40 @@ chkp_instrumentable_p (tree fndecl)
(!fn || !copy_forbidden (fn, fndecl)));
  }

 +/* Check NODE has a correct IPA_REF_CHKP reference.
 +   Create a new reference if required.  */
 +
 +void
 +chkp_maybe_fix_chkp_ref (cgraph_node *node)
 +{
 +  /* Firstly check node needs IPA_REF_CHKP.  */
 +  if (node-instrumentation_clone
 +  || !node-instrumented_version)
 +return;
 +
 +  /* Check we already have a proper IPA_REF_CHKP.
 + Remove incorrect refs.  */
 +  int i;
 +  ipa_ref *ref = NULL;
 +  bool found = false;
 +  for (i = 0; node-iterate_reference (i, ref); i++)
 +if (ref-use == IPA_REF_CHKP)
 +  {
 +   /* Found proper reference.  */
 +   if (ref-referred == node-instrumented_version
 +!found)
 + found = true;
 +   else
 + {
 +   ref-remove_reference ();
 +   i--;
 + }
 +  }
 +
 +  if (!found)
 +node-create_reference (node-instrumented_version, IPA_REF_CHKP, NULL);
 +}
 +
  /* Return clone created for instrumentation of NODE or NULL.  */

  cgraph_node *
 diff --git a/gcc/ipa-chkp.h b/gcc/ipa-chkp.h
 index 6708fe9..5fa7d88 100644
 --- a/gcc/ipa-chkp.h
 +++ b/gcc/ipa-chkp.h
 @@ -24,5 +24,6 @@ extern tree chkp_copy_function_type_adding_bounds (tree 
 orig_type);
  extern tree chkp_maybe_clone_builtin_fndecl (tree fndecl);
  extern cgraph_node *chkp_maybe_create_clone (tree fndecl);
  extern bool chkp_instrumentable_p (tree fndecl);
 +extern void chkp_maybe_fix_chkp_ref (cgraph_node *node);

  #endif /* GCC_IPA_CHKP_H */
 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 c875fed..b9196eb 100644
 --- a/gcc/lto-cgraph.c
 +++ b/gcc/lto-cgraph.c
 @@ -80,6 +80,7 @@ along with GCC; 

Re: [CHKP, PATCH] Fix LTO cgraph merge for instrumented functions

2015-04-02 Thread Jan Hubicka
 Ping
It would help if you add hubi...@ucw.cz to CC for IPA related patches.
 
 2015-03-19 11:34 GMT+03:00 Ilya Enkovich enkovich@gmail.com:
  On 12 Mar 13:27, Ilya Enkovich wrote:
  Hi,
 
  Currently cgraph merge has several issues with instrumented code:
   - original function node may be removed = no assembler name conflict is 
  detected between function and variable

Why do you need to detect this one?
   - only orig_decl name is privatized for instrumented function = node 
  still shares assembler name which causes infinite privatization loop
   - information about changed name is stored in file_data of instrumented 
  node = original section name may be not found for original function
   - chkp reference is not fixed when nodes are merged
 
  This patch should fix theese problems by keeping instrumentation thunks 
  reachable, privatizing both nodes and fixing chkp references.  
  Bootstrapped and tested on x86_64-unknown-linux-gnu.  OK for trunk?

Next stage1 I definitly hope we will be able to reduce number of special cases
we need for chck nodes.  I will try to read the code and your design document
and give it some tought.
  2015-03-19  Ilya Enkovich  ilya.enkov...@intel.com
 
  * ipa-chkp.h (chkp_maybe_fix_chkp_ref): New.
  * ipa-chkp.c (chkp_maybe_fix_chkp_ref): New.
  * ipa.c (symbol_table::remove_unreachable_nodes): Don't
  remove instumentation thunks calling reachable functions.
  * lto-cgraph.c: Include ipa-chkp.h.
  (input_symtab): Fix chkp references for boundary nodes.
  * lto/lto-partition.c (privatize_symbol_name_1): New.
  (privatize_symbol_name): Privatize both decl and orig_decl
  names for instrumented functions.
  * lto/lto-symtab.c: Include ipa-chkp.h.
  (lto_cgraph_replace_node): Fix chkp references for merged
  function nodes.
 
  gcc/testsuite/
 
  2015-03-19  Ilya Enkovich  ilya.enkov...@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-chkp.c b/gcc/ipa-chkp.c
  index 0b857ff..7a7fc3c 100644
  --- a/gcc/ipa-chkp.c
  +++ b/gcc/ipa-chkp.c
  @@ -414,6 +414,40 @@ chkp_instrumentable_p (tree fndecl)
 (!fn || !copy_forbidden (fn, fndecl)));
   }
 
  +/* Check NODE has a correct IPA_REF_CHKP reference.
  +   Create a new reference if required.  */
  +
  +void
  +chkp_maybe_fix_chkp_ref (cgraph_node *node)

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?
  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);
  +   }
  +   }
  +
This is OK
  +/* 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);

This is because these are TRANSPARENT_ALIASes and thus visibility needs to 
match?
I plan to add generic support for 

Re: [CHKP, PATCH] Fix LTO cgraph merge for instrumented functions

2015-03-19 Thread Ilya Enkovich
On 12 Mar 13:27, Ilya Enkovich wrote:
 Hi,
 
 Currently cgraph merge has several issues with instrumented code:
  - original function node may be removed = no assembler name conflict is 
 detected between function and variable
  - only orig_decl name is privatized for instrumented function = node still 
 shares assembler name which causes infinite privatization loop
  - information about changed name is stored in file_data of instrumented node 
 = original section name may be not found for original function
  - chkp reference is not fixed when nodes are merged
 
 This patch should fix theese problems by keeping instrumentation thunks 
 reachable, privatizing both nodes and fixing chkp references.  Bootstrapped 
 and tested on x86_64-unknown-linux-gnu.  OK for trunk?
 
 
 Thanks,
 Ilya

Here is an updated version where chkp_maybe_fix_chkp_ref also removes 
duplicating references which may appear during cgraph nodes merge.  
Bootstrapped and tested on x86_64-unknown-linux-gnu.  OK for trunk?


Thanks,
Ilya
--
gcc/

2015-03-19  Ilya Enkovich  ilya.enkov...@intel.com

* ipa-chkp.h (chkp_maybe_fix_chkp_ref): New.
* ipa-chkp.c (chkp_maybe_fix_chkp_ref): New.
* ipa.c (symbol_table::remove_unreachable_nodes): Don't
remove instumentation thunks calling reachable functions.
* lto-cgraph.c: Include ipa-chkp.h.
(input_symtab): Fix chkp references for boundary nodes.
* lto/lto-partition.c (privatize_symbol_name_1): New.
(privatize_symbol_name): Privatize both decl and orig_decl
names for instrumented functions.
* lto/lto-symtab.c: Include ipa-chkp.h.
(lto_cgraph_replace_node): Fix chkp references for merged
function nodes.

gcc/testsuite/

2015-03-19  Ilya Enkovich  ilya.enkov...@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-chkp.c b/gcc/ipa-chkp.c
index 0b857ff..7a7fc3c 100644
--- a/gcc/ipa-chkp.c
+++ b/gcc/ipa-chkp.c
@@ -414,6 +414,40 @@ chkp_instrumentable_p (tree fndecl)
   (!fn || !copy_forbidden (fn, fndecl)));
 }
 
+/* Check NODE has a correct IPA_REF_CHKP reference.
+   Create a new reference if required.  */
+
+void
+chkp_maybe_fix_chkp_ref (cgraph_node *node)
+{
+  /* Firstly check node needs IPA_REF_CHKP.  */
+  if (node-instrumentation_clone
+  || !node-instrumented_version)
+return;
+
+  /* Check we already have a proper IPA_REF_CHKP.
+ Remove incorrect refs.  */
+  int i;
+  ipa_ref *ref = NULL;
+  bool found = false;
+  for (i = 0; node-iterate_reference (i, ref); i++)
+if (ref-use == IPA_REF_CHKP)
+  {
+   /* Found proper reference.  */
+   if (ref-referred == node-instrumented_version
+!found)
+ found = true;
+   else
+ {
+   ref-remove_reference ();
+   i--;
+ }
+  }
+
+  if (!found)
+node-create_reference (node-instrumented_version, IPA_REF_CHKP, NULL);
+}
+
 /* Return clone created for instrumentation of NODE or NULL.  */
 
 cgraph_node *
diff --git a/gcc/ipa-chkp.h b/gcc/ipa-chkp.h
index 6708fe9..5fa7d88 100644
--- a/gcc/ipa-chkp.h
+++ b/gcc/ipa-chkp.h
@@ -24,5 +24,6 @@ extern tree chkp_copy_function_type_adding_bounds (tree 
orig_type);
 extern tree chkp_maybe_clone_builtin_fndecl (tree fndecl);
 extern cgraph_node *chkp_maybe_create_clone (tree fndecl);
 extern bool chkp_instrumentable_p (tree fndecl);
+extern void chkp_maybe_fix_chkp_ref (cgraph_node *node);
 
 #endif /* GCC_IPA_CHKP_H */
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 c875fed..b9196eb 100644
--- a/gcc/lto-cgraph.c
+++ b/gcc/lto-cgraph.c
@@ -80,6 +80,7 @@ along with GCC; see the file COPYING3.  If not see
 #include pass_manager.h
 #include ipa-utils.h
 #include omp-low.h
+#include ipa-chkp.h
 
 /* True when asm nodes has been 

[CHKP, PATCH] Fix LTO cgraph merge for instrumented functions

2015-03-12 Thread Ilya Enkovich
Hi,

Currently cgraph merge has several issues with instrumented code:
 - original function node may be removed = no assembler name conflict is 
detected between function and variable
 - only orig_decl name is privatized for instrumented function = node still 
shares assembler name which causes infinite privatization loop
 - information about changed name is stored in file_data of instrumented node 
= original section name may be not found for original function
 - chkp reference is not fixed when nodes are merged

This patch should fix theese problems by keeping instrumentation thunks 
reachable, privatizing both nodes and fixing chkp references.  Bootstrapped and 
tested on x86_64-unknown-linux-gnu.  OK for trunk?


Thanks,
Ilya
--
gcc/

2015-03-12  Ilya Enkovich  ilya.enkov...@intel.com

* ipa-chkp.h (chkp_maybe_fix_chkp_ref): New.
* ipa-chkp.c (chkp_maybe_fix_chkp_ref): New.
* ipa.c (symbol_table::remove_unreachable_nodes): Don't
remove instumentation thunks calling reachable functions.
* lto-cgraph.c: Include ipa-chkp.h.
(input_symtab): Fix chkp references for boundary nodes.
* lto/lto-partition.c (privatize_symbol_name_1): New.
(privatize_symbol_name): Privatize both decl and orig_decl
names for instrumented functions.
* lto/lto-symtab.c: Include ipa-chkp.h.
(lto_cgraph_replace_node): Fix chkp references for merged
function nodes.

gcc/testsuite/

2015-03-12  Ilya Enkovich  ilya.enkov...@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-chkp.c b/gcc/ipa-chkp.c
index 0b857ff..223f4ed 100644
--- a/gcc/ipa-chkp.c
+++ b/gcc/ipa-chkp.c
@@ -414,6 +414,36 @@ chkp_instrumentable_p (tree fndecl)
   (!fn || !copy_forbidden (fn, fndecl)));
 }
 
+/* Check NODE has a correct IPA_REF_CHKP reference.
+   Create a new reference if required.  */
+
+void
+chkp_maybe_fix_chkp_ref (cgraph_node *node)
+{
+  /* Firstly check node needs IPA_REF_CHKP.  */
+  if (node-instrumentation_clone
+  || !node-instrumented_version)
+return;
+
+  /* Check we already have a proper IPA_REF_CHKP.
+ Remove incorrect refs.  */
+  int i;
+  ipa_ref *ref = NULL;
+  for (i = 0; node-iterate_reference (i, ref); i++)
+if (ref-use == IPA_REF_CHKP)
+  {
+   /* Found proper reference.  */
+   if (ref-referred == node-instrumented_version)
+ return;
+
+   /* Need to recreate reference.  */
+   ref-remove_reference ();
+   break;
+  }
+
+  node-create_reference (node-instrumented_version, IPA_REF_CHKP, NULL);
+}
+
 /* Return clone created for instrumentation of NODE or NULL.  */
 
 cgraph_node *
diff --git a/gcc/ipa-chkp.h b/gcc/ipa-chkp.h
index 6708fe9..5fa7d88 100644
--- a/gcc/ipa-chkp.h
+++ b/gcc/ipa-chkp.h
@@ -24,5 +24,6 @@ extern tree chkp_copy_function_type_adding_bounds (tree 
orig_type);
 extern tree chkp_maybe_clone_builtin_fndecl (tree fndecl);
 extern cgraph_node *chkp_maybe_create_clone (tree fndecl);
 extern bool chkp_instrumentable_p (tree fndecl);
+extern void chkp_maybe_fix_chkp_ref (cgraph_node *node);
 
 #endif /* GCC_IPA_CHKP_H */
diff --git a/gcc/ipa.c b/gcc/ipa.c
index b3752de..ae6269f 100644
--- a/gcc/ipa.c
+++ b/gcc/ipa.c
@@ -492,7 +492,18 @@ 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);
+ 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 c875fed..b9196eb 100644
--- a/gcc/lto-cgraph.c
+++ b/gcc/lto-cgraph.c
@@ -80,6 +80,7 @@ along with GCC; see the file COPYING3.  If not see
 #include pass_manager.h
 #include ipa-utils.h
 #include omp-low.h
+#include ipa-chkp.h
 
 /* True when asm nodes has been output.  */
 bool asm_nodes_output = false;
@@ -1888,6 +1889,10 @@ input_symtab (void)
 context of the nested function.  */
   if (node-lto_file_data)
node-aux = NULL;
+
+  /* May need to fix chkp reference because we don't stream
+them for boundary symbols.  */
+  chkp_maybe_fix_chkp_ref (node);
 }
 }
 
diff --git a/gcc/lto/lto-partition.c b/gcc/lto/lto-partition.c
index 235b735..7d117e9 100644
--- a/gcc/lto/lto-partition.c
+++