[PATCH v3] tree-profile: Disable indirect call profiling for IFUNC resolvers

[PATCH v3] tree-profile: Disable indirect call profiling for IFUNC resolvers

[H.J. Lu]

We can't profile indirect calls to IFUNC resolvers nor their callees as
it requires TLS which hasn't been set up yet when the dynamic linker is
resolving IFUNC symbols.

Add an IFUNC resolver caller marker to cgraph_node and set it if the
function is called by an IFUNC resolver.  Disable indirect call profiling
for IFUNC resolvers and their callees.

Tested with profiledbootstrap on Fedora 39/x86-64.

gcc/ChangeLog:

	PR tree-optimization/114115
	* cgraph.h (symtab_node): Add check_ifunc_callee_symtab_nodes.
	(cgraph_node): Add called_by_ifunc_resolver.
	* cgraphunit.cc (symbol_table::compile): Call
	symtab_node::check_ifunc_callee_symtab_nodes.
	* symtab.cc (check_ifunc_resolver): New.
	(ifunc_ref_map): Likewise.
	(is_caller_ifunc_resolver): Likewise.
	(symtab_node::check_ifunc_callee_symtab_nodes): Likewise.
	* tree-profile.cc (gimple_gen_ic_func_profiler): Disable indirect
	call profiling for IFUNC resolvers and their callees.

gcc/testsuite/ChangeLog:

	PR tree-optimization/114115
	* gcc.dg/pr114115.c: New test.
---
 gcc/cgraph.h                    |  6 +++
 gcc/cgraphunit.cc               |  2 +
 gcc/symtab.cc                   | 89 +++++++++++++++++++++++++++++++++
 gcc/testsuite/gcc.dg/pr114115.c | 24 +++++++++
 gcc/tree-profile.cc             |  5 +-
 5 files changed, 125 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.dg/pr114115.c

diff --git a/gcc/cgraph.h b/gcc/cgraph.h
index 47f35e8078d..a8c3224802c 100644
--- a/gcc/cgraph.h
+++ b/gcc/cgraph.h
@@ -479,6 +479,9 @@ public:
      Return NULL if there's no such node.  */
   static symtab_node *get_for_asmname (const_tree asmname);
 
+  /* Check symbol table for callees of IFUNC resolvers.  */
+  static void check_ifunc_callee_symtab_nodes (void);
+
   /* Verify symbol table for internal consistency.  */
   static DEBUG_FUNCTION void verify_symtab_nodes (void);
 
@@ -896,6 +899,7 @@ struct GTY((tag ("SYMTAB_FUNCTION"))) cgraph_node : public symtab_node
       redefined_extern_inline (false), tm_may_enter_irr (false),
       ipcp_clone (false), declare_variant_alt (false),
       calls_declare_variant_alt (false), gc_candidate (false),
+      called_by_ifunc_resolver (false),
       m_uid (uid), m_summary_id (-1)
   {}
 
@@ -1495,6 +1499,8 @@ struct GTY((tag ("SYMTAB_FUNCTION"))) cgraph_node : public symtab_node
      is set for local SIMD clones when they are created and cleared if the
      vectorizer uses them.  */
   unsigned gc_candidate : 1;
+  /* Set if the function is called by an IFUNC resolver.  */
+  unsigned called_by_ifunc_resolver : 1;
 
 private:
   /* Unique id of the node.  */
diff --git a/gcc/cgraphunit.cc b/gcc/cgraphunit.cc
index d200166f7e9..2bd0289ffba 100644
--- a/gcc/cgraphunit.cc
+++ b/gcc/cgraphunit.cc
@@ -2317,6 +2317,8 @@ symbol_table::compile (void)
 
   symtab_node::checking_verify_symtab_nodes ();
 
+  symtab_node::check_ifunc_callee_symtab_nodes ();
+
   timevar_push (TV_CGRAPHOPT);
   if (pre_ipa_mem_report)
     dump_memory_report ("Memory consumption before IPA");
diff --git a/gcc/symtab.cc b/gcc/symtab.cc
index 4c7e3c135ca..3256133891d 100644
--- a/gcc/symtab.cc
+++ b/gcc/symtab.cc
@@ -1369,6 +1369,95 @@ symtab_node::verify (void)
   timevar_pop (TV_CGRAPH_VERIFY);
 }
 
+/* Return true and set *DATA to true if NODE is an ifunc resolver.  */
+
+static bool
+check_ifunc_resolver (cgraph_node *node, void *data)
+{
+  if (node->ifunc_resolver)
+    {
+      bool *is_ifunc_resolver = (bool *) data;
+      *is_ifunc_resolver = true;
+      return true;
+    }
+  return false;
+}
+
+static auto_bitmap ifunc_ref_map;
+
+/* Return true if any caller of NODE is an ifunc resolver.  */
+
+static bool
+is_caller_ifunc_resolver (cgraph_node *node)
+{
+  bool is_ifunc_resolver = false;
+
+  for (cgraph_edge *e = node->callers; e; e = e->next_caller)
+    {
+      /* Return true if caller is known to be an IFUNC resolver.  */
+      if (e->caller->called_by_ifunc_resolver)
+	return true;
+
+      /* Check for recursive call.  */
+      if (e->caller == node)
+	continue;
+
+      /* Skip if it has been visited.  */
+      unsigned int uid = e->caller->get_uid ();
+      if (bitmap_bit_p (ifunc_ref_map, uid))
+	continue;
+      bitmap_set_bit (ifunc_ref_map, uid);
+
+      if (is_caller_ifunc_resolver (e->caller))
+	{
+	  /* Return true if caller is an IFUNC resolver.  */
+	  e->caller->called_by_ifunc_resolver = true;
+	  return true;
+	}
+
+      /* Check if caller's alias is an IFUNC resolver.  */
+      e->caller->call_for_symbol_and_aliases (check_ifunc_resolver,
+					      &is_ifunc_resolver,
+					      true);
+      if (is_ifunc_resolver)
+	{
+	  /* Return true if caller's alias is an IFUNC resolver.  */
+	  e->caller->called_by_ifunc_resolver = true;
+	  return true;
+	}
+    }
+
+  return false;
+}
+
+/* Check symbol table for callees of IFUNC resolvers.  */
+
+void
+symtab_node::check_ifunc_callee_symtab_nodes (void)
+{
+  symtab_node *node;
+
+  FOR_EACH_SYMBOL (node)
+    {
+      cgraph_node *cnode = dyn_cast <cgraph_node *> (node);
+      if (!cnode)
+	continue;
+
+      unsigned int uid = cnode->get_uid ();
+      if (bitmap_bit_p (ifunc_ref_map, uid))
+	continue;
+      bitmap_set_bit (ifunc_ref_map, uid);
+
+      bool is_ifunc_resolver = false;
+      cnode->call_for_symbol_and_aliases (check_ifunc_resolver,
+					  &is_ifunc_resolver, true);
+      if (is_ifunc_resolver || is_caller_ifunc_resolver (cnode))
+	cnode->called_by_ifunc_resolver = true;
+    }
+
+  bitmap_clear (ifunc_ref_map);
+}
+
 /* Verify symbol table for internal consistency.  */
 
 DEBUG_FUNCTION void
diff --git a/gcc/testsuite/gcc.dg/pr114115.c b/gcc/testsuite/gcc.dg/pr114115.c
new file mode 100644
index 00000000000..2629f591877
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr114115.c
@@ -0,0 +1,24 @@
+/* { dg-do compile } */
+/* { dg-options "-O0 -fprofile-generate -fdump-tree-optimized" } */
+/* { dg-require-profiling "-fprofile-generate" } */
+/* { dg-require-ifunc "" } */
+
+void *foo_ifunc2() __attribute__((ifunc("foo_resolver")));
+
+void bar(void)
+{
+}
+
+static int f3()
+{
+  bar ();
+  return 5;
+}
+
+void (*foo_resolver(void))(void)
+{
+  f3();
+  return bar;
+}
+
+/* { dg-final { scan-tree-dump-not "__gcov_indirect_call_profiler_v" "optimized" } } */
diff --git a/gcc/tree-profile.cc b/gcc/tree-profile.cc
index aed13e2b1bc..373dbd60481 100644
--- a/gcc/tree-profile.cc
+++ b/gcc/tree-profile.cc
@@ -520,7 +520,10 @@ gimple_gen_ic_func_profiler (void)
   gcall *stmt1;
   tree tree_uid, cur_func, void0;
 
-  if (c_node->only_called_directly_p ())
+  /* Disable indirect call profiling for an IFUNC resolver and its
+     callees.  */
+  if (c_node->only_called_directly_p ()
+      || c_node->called_by_ifunc_resolver)
     return;
 
   gimple_init_gcov_profiler ();
-- 
2.44.0

Re: [PATCH v3] tree-profile: Disable indirect call profiling for IFUNC resolvers

[Jan Hubicka]

> We can't profile indirect calls to IFUNC resolvers nor their callees as
> it requires TLS which hasn't been set up yet when the dynamic linker is
> resolving IFUNC symbols.
> 
> Add an IFUNC resolver caller marker to cgraph_node and set it if the
> function is called by an IFUNC resolver.  Disable indirect call profiling
> for IFUNC resolvers and their callees.
> 
> Tested with profiledbootstrap on Fedora 39/x86-64.
> 
> gcc/ChangeLog:
> 
> 	PR tree-optimization/114115
> 	* cgraph.h (symtab_node): Add check_ifunc_callee_symtab_nodes.
> 	(cgraph_node): Add called_by_ifunc_resolver.
> 	* cgraphunit.cc (symbol_table::compile): Call
> 	symtab_node::check_ifunc_callee_symtab_nodes.
> 	* symtab.cc (check_ifunc_resolver): New.
> 	(ifunc_ref_map): Likewise.
> 	(is_caller_ifunc_resolver): Likewise.
> 	(symtab_node::check_ifunc_callee_symtab_nodes): Likewise.
> 	* tree-profile.cc (gimple_gen_ic_func_profiler): Disable indirect
> 	call profiling for IFUNC resolvers and their callees.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR tree-optimization/114115
> 	* gcc.dg/pr114115.c: New test.
> +/* { dg-final { scan-tree-dump-not "__gcov_indirect_call_profiler_v" "optimized" } } */
> diff --git a/gcc/tree-profile.cc b/gcc/tree-profile.cc
> index aed13e2b1bc..373dbd60481 100644
> --- a/gcc/tree-profile.cc
> +++ b/gcc/tree-profile.cc
> @@ -520,7 +520,10 @@ gimple_gen_ic_func_profiler (void)
>    gcall *stmt1;
>    tree tree_uid, cur_func, void0;
>  
> -  if (c_node->only_called_directly_p ())
> +  /* Disable indirect call profiling for an IFUNC resolver and its
> +     callees.  */
Please add a comment here referring to the PR and need to have TLS
initialized.

OK witht that change.
Honza

Re: [PATCH v3] tree-profile: Disable indirect call profiling for IFUNC resolvers

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 2303 bytes --]

On Wed, Apr 3, 2024 at 6:38 AM Jan Hubicka <hubicka@ucw.cz> wrote:
>
> > We can't profile indirect calls to IFUNC resolvers nor their callees as
> > it requires TLS which hasn't been set up yet when the dynamic linker is
> > resolving IFUNC symbols.
> >
> > Add an IFUNC resolver caller marker to cgraph_node and set it if the
> > function is called by an IFUNC resolver.  Disable indirect call profiling
> > for IFUNC resolvers and their callees.
> >
> > Tested with profiledbootstrap on Fedora 39/x86-64.
> >
> > gcc/ChangeLog:
> >
> >       PR tree-optimization/114115
> >       * cgraph.h (symtab_node): Add check_ifunc_callee_symtab_nodes.
> >       (cgraph_node): Add called_by_ifunc_resolver.
> >       * cgraphunit.cc (symbol_table::compile): Call
> >       symtab_node::check_ifunc_callee_symtab_nodes.
> >       * symtab.cc (check_ifunc_resolver): New.
> >       (ifunc_ref_map): Likewise.
> >       (is_caller_ifunc_resolver): Likewise.
> >       (symtab_node::check_ifunc_callee_symtab_nodes): Likewise.
> >       * tree-profile.cc (gimple_gen_ic_func_profiler): Disable indirect
> >       call profiling for IFUNC resolvers and their callees.
> >
> > gcc/testsuite/ChangeLog:
> >
> >       PR tree-optimization/114115
> >       * gcc.dg/pr114115.c: New test.
> > +/* { dg-final { scan-tree-dump-not "__gcov_indirect_call_profiler_v" "optimized" } } */
> > diff --git a/gcc/tree-profile.cc b/gcc/tree-profile.cc
> > index aed13e2b1bc..373dbd60481 100644
> > --- a/gcc/tree-profile.cc
> > +++ b/gcc/tree-profile.cc
> > @@ -520,7 +520,10 @@ gimple_gen_ic_func_profiler (void)
> >    gcall *stmt1;
> >    tree tree_uid, cur_func, void0;
> >
> > -  if (c_node->only_called_directly_p ())
> > +  /* Disable indirect call profiling for an IFUNC resolver and its
> > +     callees.  */
> Please add a comment here referring to the PR and need to have TLS
> initialized.
>
> OK witht that change.
> Honza

I am checking in this patch with the updated comments:

  /* Disable indirect call profiling for an IFUNC resolver and its
     callees since it requires TLS which hasn't been set up yet when
     the dynamic linker is resolving IFUNC symbols.  See
     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114115
   */

Thanks.

-- 
H.J.

[-- Attachment #2: 0001-tree-profile-Disable-indirect-call-profiling-for-IFU.patch --]
[-- Type: text/x-patch, Size: 7194 bytes --]

From 65d320ea9652d6b5f68d8b9057838224a8e2322e Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Mon, 26 Feb 2024 08:38:58 -0800
Subject: [PATCH] tree-profile: Disable indirect call profiling for IFUNC
 resolvers

We can't profile indirect calls to IFUNC resolvers nor their callees as
it requires TLS which hasn't been set up yet when the dynamic linker is
resolving IFUNC symbols.

Add an IFUNC resolver caller marker to cgraph_node and set it if the
function is called by an IFUNC resolver.  Disable indirect call profiling
for IFUNC resolvers and their callees.

Tested with profiledbootstrap on Fedora 39/x86-64.

gcc/ChangeLog:

	PR tree-optimization/114115
	* cgraph.h (symtab_node): Add check_ifunc_callee_symtab_nodes.
	(cgraph_node): Add called_by_ifunc_resolver.
	* cgraphunit.cc (symbol_table::compile): Call
	symtab_node::check_ifunc_callee_symtab_nodes.
	* symtab.cc (check_ifunc_resolver): New.
	(ifunc_ref_map): Likewise.
	(is_caller_ifunc_resolver): Likewise.
	(symtab_node::check_ifunc_callee_symtab_nodes): Likewise.
	* tree-profile.cc (gimple_gen_ic_func_profiler): Disable indirect
	call profiling for IFUNC resolvers and their callees.

gcc/testsuite/ChangeLog:

	PR tree-optimization/114115
	* gcc.dg/pr114115.c: New test.
---
 gcc/cgraph.h                    |  6 +++
 gcc/cgraphunit.cc               |  2 +
 gcc/symtab.cc                   | 89 +++++++++++++++++++++++++++++++++
 gcc/testsuite/gcc.dg/pr114115.c | 24 +++++++++
 gcc/tree-profile.cc             |  8 ++-
 5 files changed, 128 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.dg/pr114115.c

diff --git a/gcc/cgraph.h b/gcc/cgraph.h
index 47f35e8078d..a8c3224802c 100644
--- a/gcc/cgraph.h
+++ b/gcc/cgraph.h
@@ -479,6 +479,9 @@ public:
      Return NULL if there's no such node.  */
   static symtab_node *get_for_asmname (const_tree asmname);
 
+  /* Check symbol table for callees of IFUNC resolvers.  */
+  static void check_ifunc_callee_symtab_nodes (void);
+
   /* Verify symbol table for internal consistency.  */
   static DEBUG_FUNCTION void verify_symtab_nodes (void);
 
@@ -896,6 +899,7 @@ struct GTY((tag ("SYMTAB_FUNCTION"))) cgraph_node : public symtab_node
       redefined_extern_inline (false), tm_may_enter_irr (false),
       ipcp_clone (false), declare_variant_alt (false),
       calls_declare_variant_alt (false), gc_candidate (false),
+      called_by_ifunc_resolver (false),
       m_uid (uid), m_summary_id (-1)
   {}
 
@@ -1495,6 +1499,8 @@ struct GTY((tag ("SYMTAB_FUNCTION"))) cgraph_node : public symtab_node
      is set for local SIMD clones when they are created and cleared if the
      vectorizer uses them.  */
   unsigned gc_candidate : 1;
+  /* Set if the function is called by an IFUNC resolver.  */
+  unsigned called_by_ifunc_resolver : 1;
 
 private:
   /* Unique id of the node.  */
diff --git a/gcc/cgraphunit.cc b/gcc/cgraphunit.cc
index d200166f7e9..2bd0289ffba 100644
--- a/gcc/cgraphunit.cc
+++ b/gcc/cgraphunit.cc
@@ -2317,6 +2317,8 @@ symbol_table::compile (void)
 
   symtab_node::checking_verify_symtab_nodes ();
 
+  symtab_node::check_ifunc_callee_symtab_nodes ();
+
   timevar_push (TV_CGRAPHOPT);
   if (pre_ipa_mem_report)
     dump_memory_report ("Memory consumption before IPA");
diff --git a/gcc/symtab.cc b/gcc/symtab.cc
index 4c7e3c135ca..3256133891d 100644
--- a/gcc/symtab.cc
+++ b/gcc/symtab.cc
@@ -1369,6 +1369,95 @@ symtab_node::verify (void)
   timevar_pop (TV_CGRAPH_VERIFY);
 }
 
+/* Return true and set *DATA to true if NODE is an ifunc resolver.  */
+
+static bool
+check_ifunc_resolver (cgraph_node *node, void *data)
+{
+  if (node->ifunc_resolver)
+    {
+      bool *is_ifunc_resolver = (bool *) data;
+      *is_ifunc_resolver = true;
+      return true;
+    }
+  return false;
+}
+
+static auto_bitmap ifunc_ref_map;
+
+/* Return true if any caller of NODE is an ifunc resolver.  */
+
+static bool
+is_caller_ifunc_resolver (cgraph_node *node)
+{
+  bool is_ifunc_resolver = false;
+
+  for (cgraph_edge *e = node->callers; e; e = e->next_caller)
+    {
+      /* Return true if caller is known to be an IFUNC resolver.  */
+      if (e->caller->called_by_ifunc_resolver)
+	return true;
+
+      /* Check for recursive call.  */
+      if (e->caller == node)
+	continue;
+
+      /* Skip if it has been visited.  */
+      unsigned int uid = e->caller->get_uid ();
+      if (bitmap_bit_p (ifunc_ref_map, uid))
+	continue;
+      bitmap_set_bit (ifunc_ref_map, uid);
+
+      if (is_caller_ifunc_resolver (e->caller))
+	{
+	  /* Return true if caller is an IFUNC resolver.  */
+	  e->caller->called_by_ifunc_resolver = true;
+	  return true;
+	}
+
+      /* Check if caller's alias is an IFUNC resolver.  */
+      e->caller->call_for_symbol_and_aliases (check_ifunc_resolver,
+					      &is_ifunc_resolver,
+					      true);
+      if (is_ifunc_resolver)
+	{
+	  /* Return true if caller's alias is an IFUNC resolver.  */
+	  e->caller->called_by_ifunc_resolver = true;
+	  return true;
+	}
+    }
+
+  return false;
+}
+
+/* Check symbol table for callees of IFUNC resolvers.  */
+
+void
+symtab_node::check_ifunc_callee_symtab_nodes (void)
+{
+  symtab_node *node;
+
+  FOR_EACH_SYMBOL (node)
+    {
+      cgraph_node *cnode = dyn_cast <cgraph_node *> (node);
+      if (!cnode)
+	continue;
+
+      unsigned int uid = cnode->get_uid ();
+      if (bitmap_bit_p (ifunc_ref_map, uid))
+	continue;
+      bitmap_set_bit (ifunc_ref_map, uid);
+
+      bool is_ifunc_resolver = false;
+      cnode->call_for_symbol_and_aliases (check_ifunc_resolver,
+					  &is_ifunc_resolver, true);
+      if (is_ifunc_resolver || is_caller_ifunc_resolver (cnode))
+	cnode->called_by_ifunc_resolver = true;
+    }
+
+  bitmap_clear (ifunc_ref_map);
+}
+
 /* Verify symbol table for internal consistency.  */
 
 DEBUG_FUNCTION void
diff --git a/gcc/testsuite/gcc.dg/pr114115.c b/gcc/testsuite/gcc.dg/pr114115.c
new file mode 100644
index 00000000000..2629f591877
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr114115.c
@@ -0,0 +1,24 @@
+/* { dg-do compile } */
+/* { dg-options "-O0 -fprofile-generate -fdump-tree-optimized" } */
+/* { dg-require-profiling "-fprofile-generate" } */
+/* { dg-require-ifunc "" } */
+
+void *foo_ifunc2() __attribute__((ifunc("foo_resolver")));
+
+void bar(void)
+{
+}
+
+static int f3()
+{
+  bar ();
+  return 5;
+}
+
+void (*foo_resolver(void))(void)
+{
+  f3();
+  return bar;
+}
+
+/* { dg-final { scan-tree-dump-not "__gcov_indirect_call_profiler_v" "optimized" } } */
diff --git a/gcc/tree-profile.cc b/gcc/tree-profile.cc
index aed13e2b1bc..625f7e9229a 100644
--- a/gcc/tree-profile.cc
+++ b/gcc/tree-profile.cc
@@ -520,7 +520,13 @@ gimple_gen_ic_func_profiler (void)
   gcall *stmt1;
   tree tree_uid, cur_func, void0;
 
-  if (c_node->only_called_directly_p ())
+  /* Disable indirect call profiling for an IFUNC resolver and its
+     callees since it requires TLS which hasn't been set up yet when
+     the dynamic linker is resolving IFUNC symbols.  See
+     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114115
+   */
+  if (c_node->only_called_directly_p ()
+      || c_node->called_by_ifunc_resolver)
     return;
 
   gimple_init_gcov_profiler ();
-- 
2.44.0

Re: [PATCH v3] tree-profile: Disable indirect call profiling for IFUNC resolvers

[Peter Bergner]

On 4/3/24 7:40 AM, H.J. Lu wrote:
> We can't profile indirect calls to IFUNC resolvers nor their callees as
> it requires TLS which hasn't been set up yet when the dynamic linker is
> resolving IFUNC symbols.
> 
> Add an IFUNC resolver caller marker to cgraph_node and set it if the
> function is called by an IFUNC resolver.  Disable indirect call profiling
> for IFUNC resolvers and their callees.

The IFUNC resolvers on Power do not use TLS, so isn't this a little too
conservative?  Should this be triggered via a target hook so architectures
that don't use TLS in their IFUNC resolvers could still profile them?

Peter

Re: [PATCH v3] tree-profile: Disable indirect call profiling for IFUNC resolvers

[H.J. Lu]

On Wed, Apr 3, 2024 at 8:31 AM Peter Bergner <bergner@linux.ibm.com> wrote:
>
> On 4/3/24 7:40 AM, H.J. Lu wrote:
> > We can't profile indirect calls to IFUNC resolvers nor their callees as
> > it requires TLS which hasn't been set up yet when the dynamic linker is
> > resolving IFUNC symbols.
> >
> > Add an IFUNC resolver caller marker to cgraph_node and set it if the
> > function is called by an IFUNC resolver.  Disable indirect call profiling
> > for IFUNC resolvers and their callees.
>
> The IFUNC resolvers on Power do not use TLS, so isn't this a little too
> conservative?  Should this be triggered via a target hook so architectures
> that don't use TLS in their IFUNC resolvers could still profile them?

It is not about IFUNC resolver using TLS.   TLS is used by indirect call
profiling which hasn't been set up yet when the dynamic linker is resolving
IFUNC symbols.   Doesn't Power need to set up TLS before using TLS?

-- 
H.J.

Re: [PATCH v3] tree-profile: Disable indirect call profiling for IFUNC resolvers

[Andrew Pinski]

On Wed, Apr 3, 2024 at 8:32 AM Peter Bergner <bergner@linux.ibm.com> wrote:
>
> On 4/3/24 7:40 AM, H.J. Lu wrote:
> > We can't profile indirect calls to IFUNC resolvers nor their callees as
> > it requires TLS which hasn't been set up yet when the dynamic linker is
> > resolving IFUNC symbols.
> >
> > Add an IFUNC resolver caller marker to cgraph_node and set it if the
> > function is called by an IFUNC resolver.  Disable indirect call profiling
> > for IFUNC resolvers and their callees.
>
> The IFUNC resolvers on Power do not use TLS, so isn't this a little too
> conservative?  Should this be triggered via a target hook so architectures
> that don't use TLS in their IFUNC resolvers could still profile them?

I think you misunderstood the patch/situtation. Most ifunc resolves
don't use TLS at all; what is happening here is that the profiler
(-fprofile-generate) is adding TLS usage to the ifunc resolver which
then causes issues. And the use of TLS causes a PLT call to be inside
the ifun which causes all the fun stuff.

This is not about ifunc resolves using TLS directly in code but rather
indirectly via -fprofile-generate.

Thanks,
Andrew Pinski


>
> Peter
>
>

Re: [PATCH v3] tree-profile: Disable indirect call profiling for IFUNC resolvers

[Peter Bergner]

On 4/3/24 10:45 AM, Andrew Pinski wrote:
> On Wed, Apr 3, 2024 at 8:32 AM Peter Bergner <bergner@linux.ibm.com> wrote:
> I think you misunderstood the patch/situtation. Most ifunc resolves
> don't use TLS at all; what is happening here is that the profiler
> (-fprofile-generate) is adding TLS usage to the ifunc resolver which
> then causes issues. And the use of TLS causes a PLT call to be inside
> the ifun which causes all the fun stuff.
> 
> This is not about ifunc resolves using TLS directly in code but rather
> indirectly via -fprofile-generate.

Ah, ok.  Thanks to you and H.J. for clarifying.

Peter

Re: [PATCH v3] tree-profile: Disable indirect call profiling for IFUNC resolvers

[rep.dot.nop]

On 3 April 2024 15:49:13 CEST, "H.J. Lu" <hjl.tools@gmail.com> wrote:


>> OK witht that change.
>> Honza
>
>I am checking in this patch with the updated comments:
>
>  /* Disable indirect call profiling for an IFUNC resolver and its
>     callees since it requires TLS which hasn't been set up yet when
>     the dynamic linker is resolving IFUNC symbols.  See
>     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114115
>   */
>
>Thanks.
>

+      /* Skip if it has been visited.  */
+      unsigned int uid = e->caller->get_uid ();
+      if (bitmap_bit_p (ifunc_ref_map, uid))
+	continue;
+      bitmap_set_bit (ifunc_ref_map, uid);

I think you could have written this as
if (!bitmap_set_bit (ifunc_ref_map, uid))
  continue;

FWIW. thanks

Re: [PATCH v3] tree-profile: Disable indirect call profiling for IFUNC resolvers

[H.J. Lu]

On Thu, Apr 4, 2024 at 5:34 PM <rep.dot.nop@gmail.com> wrote:
>
> On 3 April 2024 15:49:13 CEST, "H.J. Lu" <hjl.tools@gmail.com> wrote:
>
>
> >> OK witht that change.
> >> Honza
> >
> >I am checking in this patch with the updated comments:
> >
> >  /* Disable indirect call profiling for an IFUNC resolver and its
> >     callees since it requires TLS which hasn't been set up yet when
> >     the dynamic linker is resolving IFUNC symbols.  See
> >     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114115
> >   */
> >
> >Thanks.
> >
>
> +      /* Skip if it has been visited.  */
> +      unsigned int uid = e->caller->get_uid ();
> +      if (bitmap_bit_p (ifunc_ref_map, uid))
> +       continue;
> +      bitmap_set_bit (ifunc_ref_map, uid);
>
> I think you could have written this as
> if (!bitmap_set_bit (ifunc_ref_map, uid))
>   continue;
>

Feel free to submit a patch.

Thanks.

-- 
H.J.

Re: [PATCH v3] tree-profile: Disable indirect call profiling for IFUNC resolvers

[rep.dot.nop]

On 5 April 2024 03:03:05 CEST, "H.J. Lu" <hjl.tools@gmail.com> wrote:
>On Thu, Apr 4, 2024 at 5:34 PM <rep.dot.nop@gmail.com> wrote:
>>
>> On 3 April 2024 15:49:13 CEST, "H.J. Lu" <hjl.tools@gmail.com> wrote:
>

>> +      /* Skip if it has been visited.  */
>> +      unsigned int uid = e->caller->get_uid ();
>> +      if (bitmap_bit_p (ifunc_ref_map, uid))
>> +       continue;
>> +      bitmap_set_bit (ifunc_ref_map, uid);
>>
>> I think you could have written this as
>> if (!bitmap_set_bit (ifunc_ref_map, uid))
>>   continue;
>>
>
>Feel free to submit a patch.

OK, could be that  https://inbox.sourceware.org/gcc-patches/20211101220212.3d308d1f@nbbrfq/
was not applied yet, the bitmap_clear_bit is the same.
I'll try to remember these for next stage 1.

cheers