Re: [PATCH] Introduce libgcov.so run-time library (PR gcov-profile/84107).

[Martin Liška <mliska at suse dot cz>]

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.

Martin

> 
> Alexander
> 

>From ce708b5d3f3742b3e8b930aa6eb74181273efd9f Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Wed, 12 Sep 2018 11:24:59 +0200
Subject: [PATCH] Patch candidate.

---
 gcc/tree-profile.c        | 88 +++++++++++++++++++++------------------
 libgcc/libgcov-profiler.c | 25 ++++-------
 libgcc/libgcov.h          |  9 ++++
 3 files changed, 64 insertions(+), 58 deletions(-)

diff --git a/gcc/tree-profile.c b/gcc/tree-profile.c
index f96bd4b9704..e2df8f005be 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,8 +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 ic_tuple_var;
+static GTY(()) tree ic_tuple_counters_field;
+static GTY(()) tree ic_tuple_callee_field;
 static GTY(()) tree ptr_void;
 
 /* Do initialization work for the edge profiler.  */
@@ -81,38 +84,34 @@ 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_void);
 
-  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_EXTERNAL (ic_tuple_var) = 1;
+  TREE_STATIC (ic_tuple_var) = 1;
+  DECL_ARTIFICIAL (ic_tuple_var) = 1;
+  DECL_INITIAL (ic_tuple_var) = NULL;
   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.  */
@@ -371,8 +370,7 @@ gimple_gen_one_value_profiler (histogram_value value, unsigned tag, unsigned bas
 void
 gimple_gen_ic_profiler (histogram_value value, unsigned tag, unsigned base)
 {
-  tree tmp1;
-  gassign *stmt1, *stmt2, *stmt3;
+  gassign *stmt1, *stmt2;
   gimple *stmt = value->hvalue.stmt;
   gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
   tree ref_ptr = tree_coverage_counter_addr (tag, base);
@@ -388,26 +386,30 @@ gimple_gen_ic_profiler (histogram_value value, unsigned tag, unsigned base)
 
   /* Insert code:
 
-    stmt1: __gcov_indirect_call_counters = get_relevant_counter_ptr ();
-    stmt2: tmp1 = (void *) (indirect call argument value)
-    stmt3: __gcov_indirect_call_callee = tmp1;
+    stmt1: __gcov_indirect_call.counters = get_relevant_counter_ptr ();
+    stmt2: __gcov_indirect_call.callee = indirect call argument value;
 
     Example:
       f_1 = foo;
-      __gcov_indirect_call_counters = &__gcov4.main[0];
-      PROF_9 = f_1;
-      __gcov_indirect_call_callee = PROF_9;
+      __gcov_indirect_call.counters = &__gcov4.main[0];
+      __gcov_indirect_call_callee = f_1;
       _4 = f_1 ();
    */
 
-  stmt1 = gimple_build_assign (ic_gcov_type_ptr_var, ref_ptr);
-  tmp1 = make_temp_ssa_name (ptr_void, 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 ptr_void = build_pointer_type (void_type_node);
+  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);
+
+  tree callee_ref = build3 (COMPONENT_REF, ptr_void,
+			     ic_tuple_var, ic_tuple_callee_field, NULL_TREE);
+  stmt2 = gimple_build_assign (callee_ref, unshare_expr (value->hvalue.value));
 
   gsi_insert_before (&gsi, stmt1, GSI_SAME_STMT);
   gsi_insert_before (&gsi, stmt2, GSI_SAME_STMT);
-  gsi_insert_before (&gsi, stmt3, GSI_SAME_STMT);
 }
 
 
@@ -461,7 +463,11 @@ gimple_gen_ic_func_profiler (void)
   gimple_stmt_iterator gsi = gsi_start_bb (cond_bb);
   void0 = build_int_cst (build_pointer_type (void_type_node), 0);
 
-  tree ref = force_gimple_operand_gsi (&gsi, ic_void_ptr_var, true, NULL_TREE,
+  tree ptr_void = build_pointer_type (void_type_node);
+  tree callee_ref = build3 (COMPONENT_REF, ptr_void,
+			    ic_tuple_var, ic_tuple_callee_field, 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/libgcc/libgcov-profiler.c b/libgcc/libgcov-profiler.c
index 7e208d75d86..c3e05db33eb 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.18.0