Don't mark IFUNC resolver as only called directly

Message ID 20180412112925.GA222547@intel.com
State New
Headers show
Series
  • Don't mark IFUNC resolver as only called directly
Related show

Commit Message

H.J. Lu April 12, 2018, 11:29 a.m.
Since IFUNC resolver is called indirectly, don't mark IFUNC resolver as
only called directly.

OK for trunk?


H.J.
---
gcc/

	PR target/85345
	* cgraph.h: Include stringpool.h" and "attribs.h".
	(cgraph_node::only_called_directly_or_aliased_p): Return false
	for IFUNC resolver.

gcc/testsuite/

	PR target/85345
	* gcc.target/i386/pr85345.c: New test.
---
 gcc/cgraph.h                            |  5 +++-
 gcc/testsuite/gcc.target/i386/pr85345.c | 44 +++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/pr85345.c

Comments

Richard Biener April 12, 2018, 12:13 p.m. | #1
On Thu, Apr 12, 2018 at 1:29 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:
> Since IFUNC resolver is called indirectly, don't mark IFUNC resolver as
> only called directly.
>
> OK for trunk?
>
>
> H.J.
> ---
> gcc/
>
>         PR target/85345
>         * cgraph.h: Include stringpool.h" and "attribs.h".
>         (cgraph_node::only_called_directly_or_aliased_p): Return false
>         for IFUNC resolver.
>
> gcc/testsuite/
>
>         PR target/85345
>         * gcc.target/i386/pr85345.c: New test.
> ---
>  gcc/cgraph.h                            |  5 +++-
>  gcc/testsuite/gcc.target/i386/pr85345.c | 44 +++++++++++++++++++++++++++++++++
>  2 files changed, 48 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/gcc.target/i386/pr85345.c
>
> diff --git a/gcc/cgraph.h b/gcc/cgraph.h
> index d1ef8408497..9e195824fcc 100644
> --- a/gcc/cgraph.h
> +++ b/gcc/cgraph.h
> @@ -24,6 +24,8 @@ along with GCC; see the file COPYING3.  If not see
>  #include "profile-count.h"
>  #include "ipa-ref.h"
>  #include "plugin-api.h"
> +#include "stringpool.h"
> +#include "attribs.h"
>
>  class ipa_opt_pass_d;
>  typedef ipa_opt_pass_d *ipa_opt_pass;
> @@ -2894,7 +2896,8 @@ cgraph_node::only_called_directly_or_aliased_p (void)
>           && !DECL_STATIC_CONSTRUCTOR (decl)
>           && !DECL_STATIC_DESTRUCTOR (decl)
>           && !used_from_object_file_p ()
> -         && !externally_visible);
> +         && !externally_visible
> +         && !lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl)));

How's it handled for our own generated resolver functions?  That is,
isn't there sth cheaper than doing a lookup_attribute here?  I see
that make_dispatcher_decl nor ix86_get_function_versions_dispatcher
adds the 'ifunc' attribute (though they are TREE_PUBLIC there).

Richard.

>  }
>
>  /* Return true when function can be removed from callgraph
> diff --git a/gcc/testsuite/gcc.target/i386/pr85345.c b/gcc/testsuite/gcc.target/i386/pr85345.c
> new file mode 100644
> index 00000000000..63f771294ad
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr85345.c
> @@ -0,0 +1,44 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fcf-protection -mcet" } */
> +/* { dg-final { scan-assembler-times {\mendbr} 4 } } */
> +
> +int resolver_fn = 0;
> +int resolved_fn = 0;
> +
> +static inline void
> +do_it_right_at_runtime_A (void)
> +{
> +  resolved_fn++;
> +}
> +
> +static inline void
> +do_it_right_at_runtime_B (void)
> +{
> +  resolved_fn++;
> +}
> +
> +static inline void do_it_right_at_runtime (void);
> +
> +void do_it_right_at_runtime (void)
> +  __attribute__ ((ifunc ("resolve_do_it_right_at_runtime")));
> +
> +extern int r;
> +static void (*resolve_do_it_right_at_runtime (void)) (void)
> +{
> +  resolver_fn++;
> +
> +  typeof(do_it_right_at_runtime) *func;
> +  if (r & 1)
> +    func = do_it_right_at_runtime_A;
> +  else
> +    func = do_it_right_at_runtime_B;
> +
> +  return (void *) func;
> +}
> +
> +int
> +main ()
> +{
> +  do_it_right_at_runtime ();
> +  return 0;
> +}
> --
> 2.14.3
>
Jan Hubicka April 12, 2018, 12:17 p.m. | #2
> On Thu, Apr 12, 2018 at 1:29 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:
> > Since IFUNC resolver is called indirectly, don't mark IFUNC resolver as
> > only called directly.
> >
> > OK for trunk?
> >
> >
> > H.J.
> > ---
> > gcc/
> >
> >         PR target/85345
> >         * cgraph.h: Include stringpool.h" and "attribs.h".
> >         (cgraph_node::only_called_directly_or_aliased_p): Return false
> >         for IFUNC resolver.
> >
> > gcc/testsuite/
> >
> >         PR target/85345
> >         * gcc.target/i386/pr85345.c: New test.
> > ---
> >  gcc/cgraph.h                            |  5 +++-
> >  gcc/testsuite/gcc.target/i386/pr85345.c | 44 +++++++++++++++++++++++++++++++++
> >  2 files changed, 48 insertions(+), 1 deletion(-)
> >  create mode 100644 gcc/testsuite/gcc.target/i386/pr85345.c
> >
> > diff --git a/gcc/cgraph.h b/gcc/cgraph.h
> > index d1ef8408497..9e195824fcc 100644
> > --- a/gcc/cgraph.h
> > +++ b/gcc/cgraph.h
> > @@ -24,6 +24,8 @@ along with GCC; see the file COPYING3.  If not see
> >  #include "profile-count.h"
> >  #include "ipa-ref.h"
> >  #include "plugin-api.h"
> > +#include "stringpool.h"
> > +#include "attribs.h"
> >
> >  class ipa_opt_pass_d;
> >  typedef ipa_opt_pass_d *ipa_opt_pass;
> > @@ -2894,7 +2896,8 @@ cgraph_node::only_called_directly_or_aliased_p (void)
> >           && !DECL_STATIC_CONSTRUCTOR (decl)
> >           && !DECL_STATIC_DESTRUCTOR (decl)
> >           && !used_from_object_file_p ()
> > -         && !externally_visible);
> > +         && !externally_visible
> > +         && !lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl)));
> 
> How's it handled for our own generated resolver functions?  That is,
> isn't there sth cheaper than doing a lookup_attribute here?  I see
> that make_dispatcher_decl nor ix86_get_function_versions_dispatcher
> adds the 'ifunc' attribute (though they are TREE_PUBLIC there).

Is there any drawback of setting force_output flag?
Honza
> 
> Richard.
> 
> >  }
> >
> >  /* Return true when function can be removed from callgraph
> > diff --git a/gcc/testsuite/gcc.target/i386/pr85345.c b/gcc/testsuite/gcc.target/i386/pr85345.c
> > new file mode 100644
> > index 00000000000..63f771294ad
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/i386/pr85345.c
> > @@ -0,0 +1,44 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2 -fcf-protection -mcet" } */
> > +/* { dg-final { scan-assembler-times {\mendbr} 4 } } */
> > +
> > +int resolver_fn = 0;
> > +int resolved_fn = 0;
> > +
> > +static inline void
> > +do_it_right_at_runtime_A (void)
> > +{
> > +  resolved_fn++;
> > +}
> > +
> > +static inline void
> > +do_it_right_at_runtime_B (void)
> > +{
> > +  resolved_fn++;
> > +}
> > +
> > +static inline void do_it_right_at_runtime (void);
> > +
> > +void do_it_right_at_runtime (void)
> > +  __attribute__ ((ifunc ("resolve_do_it_right_at_runtime")));
> > +
> > +extern int r;
> > +static void (*resolve_do_it_right_at_runtime (void)) (void)
> > +{
> > +  resolver_fn++;
> > +
> > +  typeof(do_it_right_at_runtime) *func;
> > +  if (r & 1)
> > +    func = do_it_right_at_runtime_A;
> > +  else
> > +    func = do_it_right_at_runtime_B;
> > +
> > +  return (void *) func;
> > +}
> > +
> > +int
> > +main ()
> > +{
> > +  do_it_right_at_runtime ();
> > +  return 0;
> > +}
> > --
> > 2.14.3
> >
H.J. Lu April 12, 2018, 1:37 p.m. | #3
On Thu, Apr 12, 2018 at 5:13 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Thu, Apr 12, 2018 at 1:29 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:
>> Since IFUNC resolver is called indirectly, don't mark IFUNC resolver as
>> only called directly.
>>
>> OK for trunk?
>>
>>
>> H.J.
>> ---
>> gcc/
>>
>>         PR target/85345
>>         * cgraph.h: Include stringpool.h" and "attribs.h".
>>         (cgraph_node::only_called_directly_or_aliased_p): Return false
>>         for IFUNC resolver.
>>
>> gcc/testsuite/
>>
>>         PR target/85345
>>         * gcc.target/i386/pr85345.c: New test.
>> ---
>>  gcc/cgraph.h                            |  5 +++-
>>  gcc/testsuite/gcc.target/i386/pr85345.c | 44 +++++++++++++++++++++++++++++++++
>>  2 files changed, 48 insertions(+), 1 deletion(-)
>>  create mode 100644 gcc/testsuite/gcc.target/i386/pr85345.c
>>
>> diff --git a/gcc/cgraph.h b/gcc/cgraph.h
>> index d1ef8408497..9e195824fcc 100644
>> --- a/gcc/cgraph.h
>> +++ b/gcc/cgraph.h
>> @@ -24,6 +24,8 @@ along with GCC; see the file COPYING3.  If not see
>>  #include "profile-count.h"
>>  #include "ipa-ref.h"
>>  #include "plugin-api.h"
>> +#include "stringpool.h"
>> +#include "attribs.h"
>>
>>  class ipa_opt_pass_d;
>>  typedef ipa_opt_pass_d *ipa_opt_pass;
>> @@ -2894,7 +2896,8 @@ cgraph_node::only_called_directly_or_aliased_p (void)
>>           && !DECL_STATIC_CONSTRUCTOR (decl)
>>           && !DECL_STATIC_DESTRUCTOR (decl)
>>           && !used_from_object_file_p ()
>> -         && !externally_visible);
>> +         && !externally_visible
>> +         && !lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl)));
>
> How's it handled for our own generated resolver functions?  That is,
> isn't there sth cheaper than doing a lookup_attribute here?  I see
> that make_dispatcher_decl nor ix86_get_function_versions_dispatcher
> adds the 'ifunc' attribute (though they are TREE_PUBLIC there).
>

ext/mv*.C tests failed to compile:

error: '-fcf-protection=full' requires Intel CET support. Use -mcet or
both of -mibt and -mshstk options to enable CET

with -fcf-protection -mcet.   So it is unsupported.
H.J. Lu April 12, 2018, 1:39 p.m. | #4
On Thu, Apr 12, 2018 at 5:17 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> On Thu, Apr 12, 2018 at 1:29 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:
>> > Since IFUNC resolver is called indirectly, don't mark IFUNC resolver as
>> > only called directly.
>> >
>> > OK for trunk?
>> >
>> >
>> > H.J.
>> > ---
>> > gcc/
>> >
>> >         PR target/85345
>> >         * cgraph.h: Include stringpool.h" and "attribs.h".
>> >         (cgraph_node::only_called_directly_or_aliased_p): Return false
>> >         for IFUNC resolver.
>> >
>> > gcc/testsuite/
>> >
>> >         PR target/85345
>> >         * gcc.target/i386/pr85345.c: New test.
>> > ---
>> >  gcc/cgraph.h                            |  5 +++-
>> >  gcc/testsuite/gcc.target/i386/pr85345.c | 44 +++++++++++++++++++++++++++++++++
>> >  2 files changed, 48 insertions(+), 1 deletion(-)
>> >  create mode 100644 gcc/testsuite/gcc.target/i386/pr85345.c
>> >
>> > diff --git a/gcc/cgraph.h b/gcc/cgraph.h
>> > index d1ef8408497..9e195824fcc 100644
>> > --- a/gcc/cgraph.h
>> > +++ b/gcc/cgraph.h
>> > @@ -24,6 +24,8 @@ along with GCC; see the file COPYING3.  If not see
>> >  #include "profile-count.h"
>> >  #include "ipa-ref.h"
>> >  #include "plugin-api.h"
>> > +#include "stringpool.h"
>> > +#include "attribs.h"
>> >
>> >  class ipa_opt_pass_d;
>> >  typedef ipa_opt_pass_d *ipa_opt_pass;
>> > @@ -2894,7 +2896,8 @@ cgraph_node::only_called_directly_or_aliased_p (void)
>> >           && !DECL_STATIC_CONSTRUCTOR (decl)
>> >           && !DECL_STATIC_DESTRUCTOR (decl)
>> >           && !used_from_object_file_p ()
>> > -         && !externally_visible);
>> > +         && !externally_visible
>> > +         && !lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl)));
>>
>> How's it handled for our own generated resolver functions?  That is,
>> isn't there sth cheaper than doing a lookup_attribute here?  I see
>> that make_dispatcher_decl nor ix86_get_function_versions_dispatcher
>> adds the 'ifunc' attribute (though they are TREE_PUBLIC there).
>
> Is there any drawback of setting force_output flag?
> Honza

Setting force_output may prevent some optimizations.  Can we add a bit
for IFUNC resolver?
H.J. Lu April 12, 2018, 10:50 p.m. | #5
On Thu, Apr 12, 2018 at 6:39 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Apr 12, 2018 at 5:17 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>>> On Thu, Apr 12, 2018 at 1:29 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:
>>> > Since IFUNC resolver is called indirectly, don't mark IFUNC resolver as
>>> > only called directly.
>>> >
>>> > OK for trunk?
>>> >
>>> >
>>> > H.J.
>>> > ---
>>> > gcc/
>>> >
>>> >         PR target/85345
>>> >         * cgraph.h: Include stringpool.h" and "attribs.h".
>>> >         (cgraph_node::only_called_directly_or_aliased_p): Return false
>>> >         for IFUNC resolver.
>>> >
>>> > gcc/testsuite/
>>> >
>>> >         PR target/85345
>>> >         * gcc.target/i386/pr85345.c: New test.
>>> > ---
>>> >  gcc/cgraph.h                            |  5 +++-
>>> >  gcc/testsuite/gcc.target/i386/pr85345.c | 44 +++++++++++++++++++++++++++++++++
>>> >  2 files changed, 48 insertions(+), 1 deletion(-)
>>> >  create mode 100644 gcc/testsuite/gcc.target/i386/pr85345.c
>>> >
>>> > diff --git a/gcc/cgraph.h b/gcc/cgraph.h
>>> > index d1ef8408497..9e195824fcc 100644
>>> > --- a/gcc/cgraph.h
>>> > +++ b/gcc/cgraph.h
>>> > @@ -24,6 +24,8 @@ along with GCC; see the file COPYING3.  If not see
>>> >  #include "profile-count.h"
>>> >  #include "ipa-ref.h"
>>> >  #include "plugin-api.h"
>>> > +#include "stringpool.h"
>>> > +#include "attribs.h"
>>> >
>>> >  class ipa_opt_pass_d;
>>> >  typedef ipa_opt_pass_d *ipa_opt_pass;
>>> > @@ -2894,7 +2896,8 @@ cgraph_node::only_called_directly_or_aliased_p (void)
>>> >           && !DECL_STATIC_CONSTRUCTOR (decl)
>>> >           && !DECL_STATIC_DESTRUCTOR (decl)
>>> >           && !used_from_object_file_p ()
>>> > -         && !externally_visible);
>>> > +         && !externally_visible
>>> > +         && !lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl)));
>>>
>>> How's it handled for our own generated resolver functions?  That is,
>>> isn't there sth cheaper than doing a lookup_attribute here?  I see
>>> that make_dispatcher_decl nor ix86_get_function_versions_dispatcher
>>> adds the 'ifunc' attribute (though they are TREE_PUBLIC there).
>>
>> Is there any drawback of setting force_output flag?
>> Honza
>
> Setting force_output may prevent some optimizations.  Can we add a bit
> for IFUNC resolver?
>

Here is the patch to add ifunc_resolver to cgraph_node. Tested on x86-64
and i686.  Any comments?

Thanks.

Patch

diff --git a/gcc/cgraph.h b/gcc/cgraph.h
index d1ef8408497..9e195824fcc 100644
--- a/gcc/cgraph.h
+++ b/gcc/cgraph.h
@@ -24,6 +24,8 @@  along with GCC; see the file COPYING3.  If not see
 #include "profile-count.h"
 #include "ipa-ref.h"
 #include "plugin-api.h"
+#include "stringpool.h"
+#include "attribs.h"
 
 class ipa_opt_pass_d;
 typedef ipa_opt_pass_d *ipa_opt_pass;
@@ -2894,7 +2896,8 @@  cgraph_node::only_called_directly_or_aliased_p (void)
 	  && !DECL_STATIC_CONSTRUCTOR (decl)
 	  && !DECL_STATIC_DESTRUCTOR (decl)
 	  && !used_from_object_file_p ()
-	  && !externally_visible);
+	  && !externally_visible
+	  && !lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl)));
 }
 
 /* Return true when function can be removed from callgraph
diff --git a/gcc/testsuite/gcc.target/i386/pr85345.c b/gcc/testsuite/gcc.target/i386/pr85345.c
new file mode 100644
index 00000000000..63f771294ad
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr85345.c
@@ -0,0 +1,44 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -fcf-protection -mcet" } */
+/* { dg-final { scan-assembler-times {\mendbr} 4 } } */
+
+int resolver_fn = 0;
+int resolved_fn = 0;
+
+static inline void
+do_it_right_at_runtime_A (void)
+{
+  resolved_fn++;
+}
+
+static inline void
+do_it_right_at_runtime_B (void)
+{
+  resolved_fn++;
+}
+
+static inline void do_it_right_at_runtime (void);
+
+void do_it_right_at_runtime (void)
+  __attribute__ ((ifunc ("resolve_do_it_right_at_runtime")));
+
+extern int r;
+static void (*resolve_do_it_right_at_runtime (void)) (void)
+{
+  resolver_fn++;
+
+  typeof(do_it_right_at_runtime) *func;
+  if (r & 1)
+    func = do_it_right_at_runtime_A;
+  else
+    func = do_it_right_at_runtime_B;
+
+  return (void *) func;
+}
+
+int
+main ()
+{
+  do_it_right_at_runtime ();
+  return 0;
+}