> 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 transparent aliases soon (to fix the
FORTIFY_SOURCE
LTO bug), so perhaps this will become easier?
THis is also OK. We may want to have verifier code that checks that the
visibility
match.
Honza
