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 (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;
+}
