On 9/18/18 10:00 AM, Martin Liška wrote:
> On 9/17/18 1:39 PM, Martin Liška wrote:
>> On 9/12/18 4:48 PM, Richard Biener wrote:
>>> On Wed, Sep 12, 2018 at 11:27 AM Martin Liška <mli...@suse.cz> wrote:
>>>>
>>>> On 9/11/18 5:08 PM, Alexander Monakov wrote:
>>>>> On Tue, 11 Sep 2018, Martin Liška wrote:
>>>>>> I've discussed the topic with Alexander on the Cauldron and we hoped
>>>>>> that the issue with unique __gcov_root can be handled with DECL_COMMON
>>>>>> in each DSO.
>>>>>> Apparently this approach does not work as all DSOs in an executable have
>>>>>> eventually
>>>>>> just a single instance of the __gcov_root, which is bad.
>>>>>
>>>>> No, that happens only if they have default visibility. Emitting them with
>>>>> "hidden" visibility solves this.
>>>>
>>>> Ah, ok, thanks. So theoretically that way should work.
>>>>
>>>>>
>>>>>> So that I returned back to catch the root of the failure. It shows that
>>>>>> even with
>>>>>> -Wl,--dynamic-list-data __gcov_indirect_call_callee point to a different
>>>>>> variable
>>>>>> among the DSOs. The reason is that the variable is TLS:
>>>>>
>>>>> (no, that the variable is TLS is not causing the bug; the issue is
>>>>> libraries
>>>>> carry public definitions of just one or both variables depending on if
>>>>> they
>>>>> have indirect calls or not, and the library state becomes "diverged" when
>>>>> one variable gets interposed while the other does not)
>>>>
>>>> I see, I'm attaching patch that does that. I can confirm your test-case
>>>> works
>>>> fine w/o -Wl,--dynamic-list-data.
>>>>
>>>> I'm wondering if it will work as well with dlopen/dlsym machinery? Or now
>>>> the linker flag will be needed?
>>>>
>>>>>
>>>>>> That said I would like to go this direction. I would be happy to receive
>>>>>> a feedback. Then I'll test the patch.
>>>>>
>>>>> Hm, this TLS change is attacking the issue from a wrong direction. What I
>>>>> suggested on the Cauldron as a simple and backportable way of solving this
>>>>> is to consolidate the two TLS variables into one TLS struct, which is then
>>>>> either interposed or not as a whole.
>>>>
>>>> Got it, current I prefer the solution.
>>>
>>> Sounds good. I notice the code had this before but...
>>>
>>> + TREE_PUBLIC (ic_tuple_var) = 1;
>>> + DECL_EXTERNAL (ic_tuple_var) = 1;
>>> + TREE_STATIC (ic_tuple_var) = 1;
>>>
>>> that's one flag too much?! I suggest to drop DECL_EXTERNAL.
>>
>> I've done that. I'm sending updated version of the patch.
>> I'm currently testing the patch. Ready after it finishes tests?
>>
>> Martin
>>
>>>
>>> Richard.
>>>
>>>> Martin
>>>>
>>>>>
>>>>> Alexander
>>>>>
>>>>
>>
>
> Hi.
>
> This is tested version where DECL_EXTERNAL is needed for LTO partitioning
> to work properly. Note that the previous variables emitted in tree-profile.c
> were
> also DECL_EXTERNAL.
>
> Patch survives tests on ppcl64-linux-gnu.
>
> Martin
>
Ok.
I'm sending final version of the patch that I'm going to install.
I removed TREE_STATIC and also removed few places where ptr_type_node
can replace build_pointer_type (void_type_node).
Martin
>From 3aa0d5a783bcef796d2dee09ff788d736ab0672d Mon Sep 17 00:00:00 2001
From: marxin <mli...@suse.cz>
Date: Wed, 12 Sep 2018 11:24:59 +0200
Subject: [PATCH] Fix divergence in indirect profiling (PR gcov-profile/84107).
gcc/ChangeLog:
2018-09-17 Martin Liska <mli...@suse.cz>
PR gcov-profile/84107
* tree-profile.c (init_ic_make_global_vars):
Remove ic_void_ptr_var and ic_gcov_type_ptr_var.
Come up with new ic_tuple* variables. Emit
__gcov_indirect_call{,_topn} variables.
(gimple_gen_ic_profiler): Access the variable
and emit gimple.
(gimple_gen_ic_func_profiler): Access
__gcov_indirect_call.callee field.
(gimple_init_gcov_profiler): Use ptr_type_node.
* value-prof.c (gimple_ic): Use ptr_type_node.
libgcc/ChangeLog:
2018-09-17 Martin Liska <mli...@suse.cz>
PR gcov-profile/84107
* libgcov-profiler.c (__gcov_indirect_call):
Change type to indirect_call_tuple.
(struct indirect_call_tuple): New struct.
(__gcov_indirect_call_topn_profiler): Change type.
(__gcov_indirect_call_profiler_v2): Use the new
variables.
* libgcov.h (struct indirect_call_tuple): New struct
definition.
---
gcc/tree-profile.c | 84 +++++++++++++++++++++------------------
gcc/value-prof.c | 7 ++--
libgcc/libgcov-profiler.c | 25 ++++--------
libgcc/libgcov.h | 9 +++++
4 files changed, 66 insertions(+), 59 deletions(-)
diff --git a/gcc/tree-profile.c b/gcc/tree-profile.c
index f96bd4b9704..d8f2a3b1ba4 100644
--- a/gcc/tree-profile.c
+++ b/gcc/tree-profile.c
@@ -53,6 +53,8 @@ along with GCC; see the file COPYING3. If not see
#include "stringpool.h"
#include "attribs.h"
#include "tree-pretty-print.h"
+#include "langhooks.h"
+#include "stor-layout.h"
static GTY(()) tree gcov_type_node;
static GTY(()) tree tree_interval_profiler_fn;
@@ -64,9 +66,9 @@ static GTY(()) tree tree_ior_profiler_fn;
static GTY(()) tree tree_time_profiler_counter;
-static GTY(()) tree ic_void_ptr_var;
-static GTY(()) tree ic_gcov_type_ptr_var;
-static GTY(()) tree ptr_void;
+static GTY(()) tree ic_tuple_var;
+static GTY(()) tree ic_tuple_counters_field;
+static GTY(()) tree ic_tuple_callee_field;
/* Do initialization work for the edge profiler. */
@@ -80,39 +82,35 @@ init_ic_make_global_vars (void)
{
tree gcov_type_ptr;
- ptr_void = build_pointer_type (void_type_node);
+ gcov_type_ptr = build_pointer_type (get_gcov_type ());
- ic_void_ptr_var
- = build_decl (UNKNOWN_LOCATION, VAR_DECL,
- get_identifier (
- (PARAM_VALUE (PARAM_INDIR_CALL_TOPN_PROFILE) ?
- "__gcov_indirect_call_topn_callee" :
- "__gcov_indirect_call_callee")),
- ptr_void);
- TREE_PUBLIC (ic_void_ptr_var) = 1;
- DECL_EXTERNAL (ic_void_ptr_var) = 1;
- TREE_STATIC (ic_void_ptr_var) = 1;
- DECL_ARTIFICIAL (ic_void_ptr_var) = 1;
- DECL_INITIAL (ic_void_ptr_var) = NULL;
- if (targetm.have_tls)
- set_decl_tls_model (ic_void_ptr_var, decl_default_tls_model (ic_void_ptr_var));
+ tree tuple_type = lang_hooks.types.make_type (RECORD_TYPE);
- gcov_type_ptr = build_pointer_type (get_gcov_type ());
+ /* callee */
+ ic_tuple_callee_field = build_decl (BUILTINS_LOCATION, FIELD_DECL, NULL_TREE,
+ ptr_type_node);
- ic_gcov_type_ptr_var
+ /* counters */
+ ic_tuple_counters_field = build_decl (BUILTINS_LOCATION, FIELD_DECL,
+ NULL_TREE, gcov_type_ptr);
+ DECL_CHAIN (ic_tuple_counters_field) = ic_tuple_callee_field;
+
+ finish_builtin_struct (tuple_type, "indirect_call_tuple",
+ ic_tuple_counters_field, NULL_TREE);
+
+ ic_tuple_var
= build_decl (UNKNOWN_LOCATION, VAR_DECL,
get_identifier (
(PARAM_VALUE (PARAM_INDIR_CALL_TOPN_PROFILE) ?
- "__gcov_indirect_call_topn_counters" :
- "__gcov_indirect_call_counters")),
- gcov_type_ptr);
- TREE_PUBLIC (ic_gcov_type_ptr_var) = 1;
- DECL_EXTERNAL (ic_gcov_type_ptr_var) = 1;
- TREE_STATIC (ic_gcov_type_ptr_var) = 1;
- DECL_ARTIFICIAL (ic_gcov_type_ptr_var) = 1;
- DECL_INITIAL (ic_gcov_type_ptr_var) = NULL;
+ "__gcov_indirect_call_topn" :
+ "__gcov_indirect_call")),
+ tuple_type);
+ TREE_PUBLIC (ic_tuple_var) = 1;
+ DECL_ARTIFICIAL (ic_tuple_var) = 1;
+ DECL_INITIAL (ic_tuple_var) = NULL;
+ DECL_EXTERNAL (ic_tuple_var) = 1;
if (targetm.have_tls)
- set_decl_tls_model (ic_gcov_type_ptr_var, decl_default_tls_model (ic_gcov_type_ptr_var));
+ set_decl_tls_model (ic_tuple_var, decl_default_tls_model (tuple_type));
}
/* Create the type and function decls for the interface with gcov. */
@@ -185,7 +183,7 @@ gimple_init_gcov_profiler (void)
ic_profiler_fn_type
= build_function_type_list (void_type_node,
gcov_type_node,
- ptr_void,
+ ptr_type_node,
NULL_TREE);
profiler_fn_name = "__gcov_indirect_call_profiler_v2";
if (PARAM_VALUE (PARAM_INDIR_CALL_TOPN_PROFILE))
@@ -388,22 +386,29 @@ gimple_gen_ic_profiler (histogram_value value, unsigned tag, unsigned base)
/* Insert code:
- stmt1: __gcov_indirect_call_counters = get_relevant_counter_ptr ();
+ stmt1: __gcov_indirect_call.counters = get_relevant_counter_ptr ();
stmt2: tmp1 = (void *) (indirect call argument value)
- stmt3: __gcov_indirect_call_callee = tmp1;
+ stmt3: __gcov_indirect_call.callee = tmp1;
Example:
f_1 = foo;
- __gcov_indirect_call_counters = &__gcov4.main[0];
+ __gcov_indirect_call.counters = &__gcov4.main[0];
PROF_9 = f_1;
__gcov_indirect_call_callee = PROF_9;
_4 = f_1 ();
*/
- stmt1 = gimple_build_assign (ic_gcov_type_ptr_var, ref_ptr);
- tmp1 = make_temp_ssa_name (ptr_void, NULL, "PROF");
+ tree gcov_type_ptr = build_pointer_type (get_gcov_type ());
+
+ tree counter_ref = build3 (COMPONENT_REF, gcov_type_ptr,
+ ic_tuple_var, ic_tuple_counters_field, NULL_TREE);
+
+ stmt1 = gimple_build_assign (counter_ref, ref_ptr);
+ tmp1 = make_temp_ssa_name (ptr_type_node, NULL, "PROF");
stmt2 = gimple_build_assign (tmp1, unshare_expr (value->hvalue.value));
- stmt3 = gimple_build_assign (ic_void_ptr_var, gimple_assign_lhs (stmt2));
+ tree callee_ref = build3 (COMPONENT_REF, ptr_type_node,
+ ic_tuple_var, ic_tuple_callee_field, NULL_TREE);
+ stmt3 = gimple_build_assign (callee_ref, tmp1);
gsi_insert_before (&gsi, stmt1, GSI_SAME_STMT);
gsi_insert_before (&gsi, stmt2, GSI_SAME_STMT);
@@ -459,9 +464,12 @@ gimple_gen_ic_func_profiler (void)
resetting __gcov_indirect_call_callee to NULL. */
gimple_stmt_iterator gsi = gsi_start_bb (cond_bb);
- void0 = build_int_cst (build_pointer_type (void_type_node), 0);
+ void0 = build_int_cst (ptr_type_node, 0);
+
+ tree callee_ref = build3 (COMPONENT_REF, ptr_type_node,
+ ic_tuple_var, ic_tuple_callee_field, NULL_TREE);
- tree ref = force_gimple_operand_gsi (&gsi, ic_void_ptr_var, true, NULL_TREE,
+ tree ref = force_gimple_operand_gsi (&gsi, callee_ref, true, NULL_TREE,
true, GSI_SAME_STMT);
gcond *cond = gimple_build_cond (NE_EXPR, ref,
diff --git a/gcc/value-prof.c b/gcc/value-prof.c
index 29489e0f85f..60b982793d1 100644
--- a/gcc/value-prof.c
+++ b/gcc/value-prof.c
@@ -1290,7 +1290,6 @@ gimple_ic (gcall *icall_stmt, struct cgraph_node *direct_call,
gcond *cond_stmt;
tree tmp0, tmp1, tmp;
basic_block cond_bb, dcall_bb, icall_bb, join_bb = NULL;
- tree optype = build_pointer_type (void_type_node);
edge e_cd, e_ci, e_di, e_dj = NULL, e_ij;
gimple_stmt_iterator gsi;
int lp_nr, dflags;
@@ -1300,13 +1299,13 @@ gimple_ic (gcall *icall_stmt, struct cgraph_node *direct_call,
cond_bb = gimple_bb (icall_stmt);
gsi = gsi_for_stmt (icall_stmt);
- tmp0 = make_temp_ssa_name (optype, NULL, "PROF");
- tmp1 = make_temp_ssa_name (optype, NULL, "PROF");
+ tmp0 = make_temp_ssa_name (ptr_type_node, NULL, "PROF");
+ tmp1 = make_temp_ssa_name (ptr_type_node, NULL, "PROF");
tmp = unshare_expr (gimple_call_fn (icall_stmt));
load_stmt = gimple_build_assign (tmp0, tmp);
gsi_insert_before (&gsi, load_stmt, GSI_SAME_STMT);
- tmp = fold_convert (optype, build_addr (direct_call->decl));
+ tmp = fold_convert (ptr_type_node, build_addr (direct_call->decl));
load_stmt = gimple_build_assign (tmp1, tmp);
gsi_insert_before (&gsi, load_stmt, GSI_SAME_STMT);
diff --git a/libgcc/libgcov-profiler.c b/libgcc/libgcov-profiler.c
index 7e208d75d86..7a5e50009ce 100644
--- a/libgcc/libgcov-profiler.c
+++ b/libgcc/libgcov-profiler.c
@@ -271,12 +271,7 @@ __gcov_topn_value_profiler_body (gcov_type *counters, gcov_type value)
#if defined(HAVE_CC_TLS) && !defined (USE_EMUTLS)
__thread
#endif
-gcov_type *__gcov_indirect_call_topn_counters ATTRIBUTE_HIDDEN;
-
-#if defined(HAVE_CC_TLS) && !defined (USE_EMUTLS)
-__thread
-#endif
-void *__gcov_indirect_call_topn_callee ATTRIBUTE_HIDDEN;
+struct indirect_call_tuple __gcov_indirect_call_topn;
#ifdef TARGET_VTABLE_USES_DESCRIPTORS
#define VTABLE_USES_DESCRIPTORS 1
@@ -290,14 +285,14 @@ void *__gcov_indirect_call_topn_callee ATTRIBUTE_HIDDEN;
void
__gcov_indirect_call_topn_profiler (gcov_type value, void* cur_func)
{
- void *callee_func = __gcov_indirect_call_topn_callee;
+ void *callee_func = __gcov_indirect_call_topn.callee;
/* If the C++ virtual tables contain function descriptors then one
function may have multiple descriptors and we need to dereference
the descriptors to see if they point to the same function. */
if (cur_func == callee_func
|| (VTABLE_USES_DESCRIPTORS && callee_func
&& *(void **) cur_func == *(void **) callee_func))
- __gcov_topn_value_profiler_body (__gcov_indirect_call_topn_counters, value);
+ __gcov_topn_value_profiler_body (__gcov_indirect_call_topn.counters, value);
}
#endif
@@ -311,11 +306,7 @@ __gcov_indirect_call_topn_profiler (gcov_type value, void* cur_func)
#if defined(HAVE_CC_TLS) && !defined (USE_EMUTLS)
__thread
#endif
-void * __gcov_indirect_call_callee;
-#if defined(HAVE_CC_TLS) && !defined (USE_EMUTLS)
-__thread
-#endif
-gcov_type * __gcov_indirect_call_counters;
+struct indirect_call_tuple __gcov_indirect_call;
/* By default, the C++ compiler will use function addresses in the
vtable entries. Setting TARGET_VTABLE_USES_DESCRIPTORS to nonzero
@@ -332,12 +323,12 @@ __gcov_indirect_call_profiler_v2 (gcov_type value, void* cur_func)
/* If the C++ virtual tables contain function descriptors then one
function may have multiple descriptors and we need to dereference
the descriptors to see if they point to the same function. */
- if (cur_func == __gcov_indirect_call_callee
+ if (cur_func == __gcov_indirect_call.callee
|| (__LIBGCC_VTABLE_USES_DESCRIPTORS__
- && *(void **) cur_func == *(void **) __gcov_indirect_call_callee))
- __gcov_one_value_profiler_body (__gcov_indirect_call_counters, value, 0);
+ && *(void **) cur_func == *(void **) __gcov_indirect_call.callee))
+ __gcov_one_value_profiler_body (__gcov_indirect_call.counters, value, 0);
- __gcov_indirect_call_callee = NULL;
+ __gcov_indirect_call.callee = NULL;
}
#endif
diff --git a/libgcc/libgcov.h b/libgcc/libgcov.h
index 21422873cf2..ee05a68d2b2 100644
--- a/libgcc/libgcov.h
+++ b/libgcc/libgcov.h
@@ -226,6 +226,15 @@ struct gcov_master
gcov_unsigned_t version;
struct gcov_root *root;
};
+
+struct indirect_call_tuple
+{
+ /* Callee function. */
+ void *callee;
+
+ /* Pointer to counters. */
+ gcov_type *counters;
+};
/* Exactly one of these will be active in the process. */
extern struct gcov_master __gcov_master;
--
2.19.0
