PING^2: [PATCH] Don't mark IFUNC resolver as only called directly

PING^2: [PATCH] Don't mark IFUNC resolver as only called directly

[H.J. Lu]

On Wed, Apr 25, 2018 at 8:49 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Apr 12, 2018 at 3:50 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> 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?
>>
>
> PING:
>
> https://gcc.gnu.org/ml/gcc-patches/2018-04/msg00647.html
>

PING.


-- 
H.J.

Re: PING^2: [PATCH] Don't mark IFUNC resolver as only called directly

[Jan Hubicka]

> >>>>> >  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?
> >>
> >
> > PING:
> >
> > https://gcc.gnu.org/ml/gcc-patches/2018-04/msg00647.html
> >
> 
> PING.
OK, but please extend the verifier that ifunc_resolver flag is equivalent to
lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl))
so we are sure things stays in sync.

Thanks and sorry for the delay,
Honza
> 
> 
> -- 
> H.J.

Re: PING^2: [PATCH] Don't mark IFUNC resolver as only called directly

[H.J. Lu]

On Tue, May 22, 2018 at 9:21 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> >>>>> >  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?
>> >>
>> >
>> > PING:
>> >
>> > https://gcc.gnu.org/ml/gcc-patches/2018-04/msg00647.html
>> >
>>
>> PING.
> OK, but please extend the verifier that ifunc_resolver flag is equivalent to
> lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl))
> so we are sure things stays in sync.
>

Like this

diff --git a/gcc/symtab.c b/gcc/symtab.c
index 80f6f910c3b..954920b6dff 100644
--- a/gcc/symtab.c
+++ b/gcc/symtab.c
@@ -998,6 +998,13 @@ symtab_node::verify_base (void)
           error ("function symbol is not function");
           error_found = true;
   }
+      else if ((lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl))
+     != NULL)
+         != dyn_cast <cgraph_node *> (this)->ifunc_resolver)
+  {
+          error ("inconsistent `ifunc' attribute");
+          error_found = true;
+  }
     }
   else if (is_a <varpool_node *> (this))
     {


Thanks.

-- 
H.J.

Re: PING^2: [PATCH] Don't mark IFUNC resolver as only called directly

[Jan Hubicka]

> On Tue, May 22, 2018 at 9:21 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> >> >>>>> >  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?
> >> >>
> >> >
> >> > PING:
> >> >
> >> > https://gcc.gnu.org/ml/gcc-patches/2018-04/msg00647.html
> >> >
> >>
> >> PING.
> > OK, but please extend the verifier that ifunc_resolver flag is equivalent to
> > lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl))
> > so we are sure things stays in sync.
> >
> 
> Like this
> 
> diff --git a/gcc/symtab.c b/gcc/symtab.c
> index 80f6f910c3b..954920b6dff 100644
> --- a/gcc/symtab.c
> +++ b/gcc/symtab.c
> @@ -998,6 +998,13 @@ symtab_node::verify_base (void)
>            error ("function symbol is not function");
>            error_found = true;
>    }
> +      else if ((lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl))
> +     != NULL)
> +         != dyn_cast <cgraph_node *> (this)->ifunc_resolver)
> +  {
> +          error ("inconsistent `ifunc' attribute");
> +          error_found = true;
> +  }
>      }
>    else if (is_a <varpool_node *> (this))
>      {
> 
> 
> Thanks.
Yes, thanks!
Honza
> 
> -- 
> H.J.

Re: PING^2: [PATCH] Don't mark IFUNC resolver as only called directly

[H.J. Lu]

On Wed, May 23, 2018 at 2:01 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> On Tue, May 22, 2018 at 9:21 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> >> >>>>> >  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?
>> >> >>
>> >> >
>> >> > PING:
>> >> >
>> >> > https://gcc.gnu.org/ml/gcc-patches/2018-04/msg00647.html
>> >> >
>> >>
>> >> PING.
>> > OK, but please extend the verifier that ifunc_resolver flag is equivalent to
>> > lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl))
>> > so we are sure things stays in sync.
>> >
>>
>> Like this
>>
>> diff --git a/gcc/symtab.c b/gcc/symtab.c
>> index 80f6f910c3b..954920b6dff 100644
>> --- a/gcc/symtab.c
>> +++ b/gcc/symtab.c
>> @@ -998,6 +998,13 @@ symtab_node::verify_base (void)
>>            error ("function symbol is not function");
>>            error_found = true;
>>    }
>> +      else if ((lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl))
>> +     != NULL)
>> +         != dyn_cast <cgraph_node *> (this)->ifunc_resolver)
>> +  {
>> +          error ("inconsistent `ifunc' attribute");
>> +          error_found = true;
>> +  }
>>      }
>>    else if (is_a <varpool_node *> (this))
>>      {
>>
>>
>> Thanks.
> Yes, thanks!
> Honza

I'd like to also fix it on GCC 8 branch for CET.  Should I backport my
patch to GCC 8 after a few days or use the simple patch for GCC 8:

https://gcc.gnu.org/ml/gcc-patches/2018-04/msg00588.html

Thanks.

-- 
H.J.

Re: PING^2: [PATCH] Don't mark IFUNC resolver as only called directly

[Jan Hubicka]

> On Wed, May 23, 2018 at 2:01 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> >> On Tue, May 22, 2018 at 9:21 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> >> >> >>>>> >  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?
> >> >> >>
> >> >> >
> >> >> > PING:
> >> >> >
> >> >> > https://gcc.gnu.org/ml/gcc-patches/2018-04/msg00647.html
> >> >> >
> >> >>
> >> >> PING.
> >> > OK, but please extend the verifier that ifunc_resolver flag is equivalent to
> >> > lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl))
> >> > so we are sure things stays in sync.
> >> >
> >>
> >> Like this
> >>
> >> diff --git a/gcc/symtab.c b/gcc/symtab.c
> >> index 80f6f910c3b..954920b6dff 100644
> >> --- a/gcc/symtab.c
> >> +++ b/gcc/symtab.c
> >> @@ -998,6 +998,13 @@ symtab_node::verify_base (void)
> >>            error ("function symbol is not function");
> >>            error_found = true;
> >>    }
> >> +      else if ((lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl))
> >> +     != NULL)
> >> +         != dyn_cast <cgraph_node *> (this)->ifunc_resolver)
> >> +  {
> >> +          error ("inconsistent `ifunc' attribute");
> >> +          error_found = true;
> >> +  }
> >>      }
> >>    else if (is_a <varpool_node *> (this))
> >>      {
> >>
> >>
> >> Thanks.
> > Yes, thanks!
> > Honza
> 
> I'd like to also fix it on GCC 8 branch for CET.  Should I backport my
> patch to GCC 8 after a few days or use the simple patch for GCC 8:
> 
> https://gcc.gnu.org/ml/gcc-patches/2018-04/msg00588.html

I would backport this one so we don't unnecesarily diverge.
Thanks!
Honza
> 
> Thanks.
> 
> -- 
> H.J.

Re: PING^2: [PATCH] Don't mark IFUNC resolver as only called directly

[H.J. Lu]

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

On Wed, May 23, 2018 at 8:11 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> On Wed, May 23, 2018 at 2:01 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> >> On Tue, May 22, 2018 at 9:21 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> >> >> >>>>> >  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?
>> >> >> >>
>> >> >> >
>> >> >> > PING:
>> >> >> >
>> >> >> > https://gcc.gnu.org/ml/gcc-patches/2018-04/msg00647.html
>> >> >> >
>> >> >>
>> >> >> PING.
>> >> > OK, but please extend the verifier that ifunc_resolver flag is equivalent to
>> >> > lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl))
>> >> > so we are sure things stays in sync.
>> >> >
>> >>
>> >> Like this
>> >>
>> >> diff --git a/gcc/symtab.c b/gcc/symtab.c
>> >> index 80f6f910c3b..954920b6dff 100644
>> >> --- a/gcc/symtab.c
>> >> +++ b/gcc/symtab.c
>> >> @@ -998,6 +998,13 @@ symtab_node::verify_base (void)
>> >>            error ("function symbol is not function");
>> >>            error_found = true;
>> >>    }
>> >> +      else if ((lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl))
>> >> +     != NULL)
>> >> +         != dyn_cast <cgraph_node *> (this)->ifunc_resolver)
>> >> +  {
>> >> +          error ("inconsistent `ifunc' attribute");
>> >> +          error_found = true;
>> >> +  }
>> >>      }
>> >>    else if (is_a <varpool_node *> (this))
>> >>      {
>> >>
>> >>
>> >> Thanks.
>> > Yes, thanks!
>> > Honza
>>
>> I'd like to also fix it on GCC 8 branch for CET.  Should I backport my
>> patch to GCC 8 after a few days or use the simple patch for GCC 8:
>>
>> https://gcc.gnu.org/ml/gcc-patches/2018-04/msg00588.html
>
> I would backport this one so we don't unnecesarily diverge.
> Thanks!
> Honza

This is the backport which I will check into GCC 8 branch next week.

Thanks.

-- 
H.J.

[-- Attachment #2: 0001-Don-t-mark-IFUNC-resolver-as-only-called-directly.patch --]
[-- Type: text/x-patch, Size: 8219 bytes --]

From a938f6318261eb93cb02ede65b0966c2d8a78880 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Wed, 11 Apr 2018 12:31:21 -0700
Subject: [PATCH] Don't mark IFUNC resolver as only called directly

Since IFUNC resolver is called indirectly, don't mark IFUNC resolver as
only called directly.  This patch adds ifunc_resolver to cgraph_node,
sets ifunc_resolver for ifunc attribute and checks ifunc_resolver
instead of looking up ifunc attribute.

gcc/

	Backport from mainline
	2018-05-22  H.J. Lu  <hongjiu.lu@intel.com>

	PR target/85345
	* cgraph.h (cgraph_node::create): Set ifunc_resolver for ifunc
	attribute.
	(cgraph_node::create_alias): Likewise.
	(cgraph_node::get_availability): Check ifunc_resolver instead
	of looking up ifunc attribute.
	* cgraphunit.c (maybe_diag_incompatible_alias): Likewise.
	* varasm.c (do_assemble_alias): Likewise.
	(assemble_alias): Likewise.
	(default_binds_local_p_3): Likewise.
	* cgraph.h (cgraph_node): Add ifunc_resolver.
	(cgraph_node::only_called_directly_or_aliased_p): Return false
	for IFUNC resolver.
	* lto-cgraph.c (input_node): Set ifunc_resolver for ifunc
	attribute.
	* symtab.c (symtab_node::verify_base): Verify that ifunc_resolver
	is equivalent to lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl)).
	(symtab_node::binds_to_current_def_p): Check ifunc_resolver
	instead of looking up ifunc attribute.

gcc/testsuite/

	Backport from mainline
	2018-05-22  H.J. Lu  <hongjiu.lu@intel.com>

	PR target/85345
	* gcc.target/i386/pr85345.c: New test.
---
 gcc/cgraph.c                            |  7 +++-
 gcc/cgraph.h                            |  4 +++
 gcc/cgraphunit.c                        |  2 +-
 gcc/lto-cgraph.c                        |  2 ++
 gcc/symtab.c                            | 11 +++++--
 gcc/testsuite/gcc.target/i386/pr85345.c | 44 +++++++++++++++++++++++++
 gcc/varasm.c                            |  8 +++--
 7 files changed, 71 insertions(+), 7 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/pr85345.c

diff --git a/gcc/cgraph.c b/gcc/cgraph.c
index 9a7d54d7cee..9f3a2929f6b 100644
--- a/gcc/cgraph.c
+++ b/gcc/cgraph.c
@@ -517,6 +517,9 @@ cgraph_node::create (tree decl)
 	g->have_offload = true;
     }
 
+  if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl)))
+    node->ifunc_resolver = true;
+
   node->register_symbol ();
 
   if (DECL_CONTEXT (decl) && TREE_CODE (DECL_CONTEXT (decl)) == FUNCTION_DECL)
@@ -575,6 +578,8 @@ cgraph_node::create_alias (tree alias, tree target)
   alias_node->alias = true;
   if (lookup_attribute ("weakref", DECL_ATTRIBUTES (alias)) != NULL)
     alias_node->transparent_alias = alias_node->weakref = true;
+  if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (alias)))
+    alias_node->ifunc_resolver = true;
   return alias_node;
 }
 
@@ -2299,7 +2304,7 @@ cgraph_node::get_availability (symtab_node *ref)
     avail = AVAIL_AVAILABLE;
   else if (transparent_alias)
     ultimate_alias_target (&avail, ref);
-  else if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl))
+  else if (ifunc_resolver
 	   || lookup_attribute ("noipa", DECL_ATTRIBUTES (decl)))
     avail = AVAIL_INTERPOSABLE;
   else if (!externally_visible)
diff --git a/gcc/cgraph.h b/gcc/cgraph.h
index ee7ebb41c24..afb2745a841 100644
--- a/gcc/cgraph.h
+++ b/gcc/cgraph.h
@@ -530,6 +530,9 @@ public:
   /* Set when symbol can be streamed into bytecode for offloading.  */
   unsigned offloadable : 1;
 
+  /* Set when symbol is an IFUNC resolver.  */
+  unsigned ifunc_resolver : 1;
+
 
   /* Ordering of all symtab entries.  */
   int order;
@@ -2886,6 +2889,7 @@ cgraph_node::only_called_directly_or_aliased_p (void)
 {
   gcc_assert (!global.inlined_to);
   return (!force_output && !address_taken
+	  && !ifunc_resolver
 	  && !used_from_other_partition
 	  && !DECL_VIRTUAL_P (decl)
 	  && !DECL_STATIC_CONSTRUCTOR (decl)
diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index e418ec04149..212ee7b8340 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -1307,7 +1307,7 @@ maybe_diag_incompatible_alias (tree alias, tree target)
   tree altype = TREE_TYPE (alias);
   tree targtype = TREE_TYPE (target);
 
-  bool ifunc = lookup_attribute ("ifunc", DECL_ATTRIBUTES (alias));
+  bool ifunc = cgraph_node::get (alias)->ifunc_resolver;
   tree funcptr = altype;
 
   if (ifunc)
diff --git a/gcc/lto-cgraph.c b/gcc/lto-cgraph.c
index dcd5391012c..40baf858ca5 100644
--- a/gcc/lto-cgraph.c
+++ b/gcc/lto-cgraph.c
@@ -1257,6 +1257,8 @@ input_node (struct lto_file_decl_data *file_data,
 	 of ipa passes is done.  Alays forcingly create a fresh node.  */
       node = symtab->create_empty ();
       node->decl = fn_decl;
+      if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (fn_decl)))
+	node->ifunc_resolver = 1;
       node->register_symbol ();
     }
 
diff --git a/gcc/symtab.c b/gcc/symtab.c
index c1533083573..954920b6dff 100644
--- a/gcc/symtab.c
+++ b/gcc/symtab.c
@@ -998,6 +998,13 @@ symtab_node::verify_base (void)
           error ("function symbol is not function");
           error_found = true;
 	}
+      else if ((lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl))
+		!= NULL)
+	       != dyn_cast <cgraph_node *> (this)->ifunc_resolver)
+	{
+          error ("inconsistent `ifunc' attribute");
+          error_found = true;
+	}
     }
   else if (is_a <varpool_node *> (this))
     {
@@ -2253,13 +2260,13 @@ symtab_node::binds_to_current_def_p (symtab_node *ref)
   if (transparent_alias)
     return definition
 	   && get_alias_target()->binds_to_current_def_p (ref);
-  if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl)))
+  cgraph_node *cnode = dyn_cast <cgraph_node *> (this);
+  if (cnode && cnode->ifunc_resolver)
     return false;
   if (decl_binds_to_current_def_p (decl))
     return true;
 
   /* Inline clones always binds locally.  */
-  cgraph_node *cnode = dyn_cast <cgraph_node *> (this);
   if (cnode && cnode->global.inlined_to)
     return true;
 
diff --git a/gcc/testsuite/gcc.target/i386/pr85345.c b/gcc/testsuite/gcc.target/i386/pr85345.c
new file mode 100644
index 00000000000..ceb94e4b940
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr85345.c
@@ -0,0 +1,44 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fcf-protection" } */
+/* { 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;
+}
diff --git a/gcc/varasm.c b/gcc/varasm.c
index d24bac4ad8f..85b0b16b8d0 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -5821,7 +5821,8 @@ do_assemble_alias (tree decl, tree target)
       globalize_decl (decl);
       maybe_assemble_visibility (decl);
     }
-  if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl)))
+  if (TREE_CODE (decl) == FUNCTION_DECL
+      && cgraph_node::get (decl)->ifunc_resolver)
     {
 #if defined (ASM_OUTPUT_TYPE_DIRECTIVE)
       if (targetm.has_ifunc_p ())
@@ -5904,7 +5905,7 @@ assemble_alias (tree decl, tree target)
 # else
       if (!DECL_WEAK (decl))
 	{
-	  if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl)))
+	  if (cgraph_node::get (decl)->ifunc_resolver)
 	    error_at (DECL_SOURCE_LOCATION (decl),
 		      "ifunc is not supported in this configuration");
 	  else
@@ -7024,7 +7025,8 @@ default_binds_local_p_3 (const_tree exp, bool shlib, bool weak_dominate,
      weakref alias.  */
   if (lookup_attribute ("weakref", DECL_ATTRIBUTES (exp))
       || (TREE_CODE (exp) == FUNCTION_DECL
-	  && lookup_attribute ("ifunc", DECL_ATTRIBUTES (exp))))
+	  && cgraph_node::get (exp)
+	  && cgraph_node::get (exp)->ifunc_resolver))
     return false;
 
   /* Static variables are always local.  */
-- 
2.17.0

Re: PING^2: [PATCH] Don't mark IFUNC resolver as only called directly

[H.J. Lu]

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

On Wed, May 23, 2018 at 8:35 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Wed, May 23, 2018 at 8:11 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>>> On Wed, May 23, 2018 at 2:01 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>>> >> On Tue, May 22, 2018 at 9:21 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>>> >> >> >>>>> >  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?
>>> >> >> >>
>>> >> >> >
>>> >> >> > PING:
>>> >> >> >
>>> >> >> > https://gcc.gnu.org/ml/gcc-patches/2018-04/msg00647.html
>>> >> >> >
>>> >> >>
>>> >> >> PING.
>>> >> > OK, but please extend the verifier that ifunc_resolver flag is equivalent to
>>> >> > lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl))
>>> >> > so we are sure things stays in sync.
>>> >> >
>>> >>
>>> >> Like this
>>> >>
>>> >> diff --git a/gcc/symtab.c b/gcc/symtab.c
>>> >> index 80f6f910c3b..954920b6dff 100644
>>> >> --- a/gcc/symtab.c
>>> >> +++ b/gcc/symtab.c
>>> >> @@ -998,6 +998,13 @@ symtab_node::verify_base (void)
>>> >>            error ("function symbol is not function");
>>> >>            error_found = true;
>>> >>    }
>>> >> +      else if ((lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl))
>>> >> +     != NULL)
>>> >> +         != dyn_cast <cgraph_node *> (this)->ifunc_resolver)
>>> >> +  {
>>> >> +          error ("inconsistent `ifunc' attribute");
>>> >> +          error_found = true;
>>> >> +  }
>>> >>      }
>>> >>    else if (is_a <varpool_node *> (this))
>>> >>      {
>>> >>
>>> >>
>>> >> Thanks.
>>> > Yes, thanks!
>>> > Honza
>>>
>>> I'd like to also fix it on GCC 8 branch for CET.  Should I backport my
>>> patch to GCC 8 after a few days or use the simple patch for GCC 8:
>>>
>>> https://gcc.gnu.org/ml/gcc-patches/2018-04/msg00588.html
>>
>> I would backport this one so we don't unnecesarily diverge.
>> Thanks!
>> Honza
>
> This is the backport which I will check into GCC 8 branch next week.
>

This is the updated backport which I will check into GCC 8 branch next week.


-- 
H.J.

[-- Attachment #2: 0001-Don-t-mark-IFUNC-resolver-as-only-called-directly.patch --]
[-- Type: text/x-patch, Size: 8561 bytes --]

From 4516b6c76f4af7de2c09d34ab2ed1834c5ff31a0 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Wed, 11 Apr 2018 12:31:21 -0700
Subject: [PATCH] Don't mark IFUNC resolver as only called directly

Since IFUNC resolver is called indirectly, don't mark IFUNC resolver as
only called directly.  This patch adds ifunc_resolver to cgraph_node,
sets ifunc_resolver for ifunc attribute and checks ifunc_resolver
instead of looking up ifunc attribute.

gcc/

	Backport from mainline
	2018-05-24  H.J. Lu  <hongjiu.lu@intel.com>

	PR target/85900
	PR target/85345
	* varasm.c (assemble_alias): Check ifunc_resolver only on
	FUNCTION_DECL.

	2018-05-22  H.J. Lu  <hongjiu.lu@intel.com>

	PR target/85345
	* cgraph.h (cgraph_node::create): Set ifunc_resolver for ifunc
	attribute.
	(cgraph_node::create_alias): Likewise.
	(cgraph_node::get_availability): Check ifunc_resolver instead
	of looking up ifunc attribute.
	* cgraphunit.c (maybe_diag_incompatible_alias): Likewise.
	* varasm.c (do_assemble_alias): Likewise.
	(assemble_alias): Likewise.
	(default_binds_local_p_3): Likewise.
	* cgraph.h (cgraph_node): Add ifunc_resolver.
	(cgraph_node::only_called_directly_or_aliased_p): Return false
	for IFUNC resolver.
	* lto-cgraph.c (input_node): Set ifunc_resolver for ifunc
	attribute.
	* symtab.c (symtab_node::verify_base): Verify that ifunc_resolver
	is equivalent to lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl)).
	(symtab_node::binds_to_current_def_p): Check ifunc_resolver
	instead of looking up ifunc attribute.

gcc/testsuite/

	Backport from mainline
	2018-05-24  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>

	* gcc.target/i386/pr85345.c: Require ifunc support.

	2018-05-22  H.J. Lu  <hongjiu.lu@intel.com>

	PR target/85345
	* gcc.target/i386/pr85345.c: New test.
---
 gcc/cgraph.c                            |  7 +++-
 gcc/cgraph.h                            |  4 +++
 gcc/cgraphunit.c                        |  2 +-
 gcc/lto-cgraph.c                        |  2 ++
 gcc/symtab.c                            | 11 ++++--
 gcc/testsuite/gcc.target/i386/pr85345.c | 45 +++++++++++++++++++++++++
 gcc/varasm.c                            |  9 +++--
 7 files changed, 73 insertions(+), 7 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/pr85345.c

diff --git a/gcc/cgraph.c b/gcc/cgraph.c
index 9a7d54d7cee..9f3a2929f6b 100644
--- a/gcc/cgraph.c
+++ b/gcc/cgraph.c
@@ -517,6 +517,9 @@ cgraph_node::create (tree decl)
 	g->have_offload = true;
     }
 
+  if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl)))
+    node->ifunc_resolver = true;
+
   node->register_symbol ();
 
   if (DECL_CONTEXT (decl) && TREE_CODE (DECL_CONTEXT (decl)) == FUNCTION_DECL)
@@ -575,6 +578,8 @@ cgraph_node::create_alias (tree alias, tree target)
   alias_node->alias = true;
   if (lookup_attribute ("weakref", DECL_ATTRIBUTES (alias)) != NULL)
     alias_node->transparent_alias = alias_node->weakref = true;
+  if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (alias)))
+    alias_node->ifunc_resolver = true;
   return alias_node;
 }
 
@@ -2299,7 +2304,7 @@ cgraph_node::get_availability (symtab_node *ref)
     avail = AVAIL_AVAILABLE;
   else if (transparent_alias)
     ultimate_alias_target (&avail, ref);
-  else if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl))
+  else if (ifunc_resolver
 	   || lookup_attribute ("noipa", DECL_ATTRIBUTES (decl)))
     avail = AVAIL_INTERPOSABLE;
   else if (!externally_visible)
diff --git a/gcc/cgraph.h b/gcc/cgraph.h
index ee7ebb41c24..afb2745a841 100644
--- a/gcc/cgraph.h
+++ b/gcc/cgraph.h
@@ -530,6 +530,9 @@ public:
   /* Set when symbol can be streamed into bytecode for offloading.  */
   unsigned offloadable : 1;
 
+  /* Set when symbol is an IFUNC resolver.  */
+  unsigned ifunc_resolver : 1;
+
 
   /* Ordering of all symtab entries.  */
   int order;
@@ -2886,6 +2889,7 @@ cgraph_node::only_called_directly_or_aliased_p (void)
 {
   gcc_assert (!global.inlined_to);
   return (!force_output && !address_taken
+	  && !ifunc_resolver
 	  && !used_from_other_partition
 	  && !DECL_VIRTUAL_P (decl)
 	  && !DECL_STATIC_CONSTRUCTOR (decl)
diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index e418ec04149..212ee7b8340 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -1307,7 +1307,7 @@ maybe_diag_incompatible_alias (tree alias, tree target)
   tree altype = TREE_TYPE (alias);
   tree targtype = TREE_TYPE (target);
 
-  bool ifunc = lookup_attribute ("ifunc", DECL_ATTRIBUTES (alias));
+  bool ifunc = cgraph_node::get (alias)->ifunc_resolver;
   tree funcptr = altype;
 
   if (ifunc)
diff --git a/gcc/lto-cgraph.c b/gcc/lto-cgraph.c
index dcd5391012c..40baf858ca5 100644
--- a/gcc/lto-cgraph.c
+++ b/gcc/lto-cgraph.c
@@ -1257,6 +1257,8 @@ input_node (struct lto_file_decl_data *file_data,
 	 of ipa passes is done.  Alays forcingly create a fresh node.  */
       node = symtab->create_empty ();
       node->decl = fn_decl;
+      if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (fn_decl)))
+	node->ifunc_resolver = 1;
       node->register_symbol ();
     }
 
diff --git a/gcc/symtab.c b/gcc/symtab.c
index c1533083573..954920b6dff 100644
--- a/gcc/symtab.c
+++ b/gcc/symtab.c
@@ -998,6 +998,13 @@ symtab_node::verify_base (void)
           error ("function symbol is not function");
           error_found = true;
 	}
+      else if ((lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl))
+		!= NULL)
+	       != dyn_cast <cgraph_node *> (this)->ifunc_resolver)
+	{
+          error ("inconsistent `ifunc' attribute");
+          error_found = true;
+	}
     }
   else if (is_a <varpool_node *> (this))
     {
@@ -2253,13 +2260,13 @@ symtab_node::binds_to_current_def_p (symtab_node *ref)
   if (transparent_alias)
     return definition
 	   && get_alias_target()->binds_to_current_def_p (ref);
-  if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl)))
+  cgraph_node *cnode = dyn_cast <cgraph_node *> (this);
+  if (cnode && cnode->ifunc_resolver)
     return false;
   if (decl_binds_to_current_def_p (decl))
     return true;
 
   /* Inline clones always binds locally.  */
-  cgraph_node *cnode = dyn_cast <cgraph_node *> (this);
   if (cnode && cnode->global.inlined_to)
     return true;
 
diff --git a/gcc/testsuite/gcc.target/i386/pr85345.c b/gcc/testsuite/gcc.target/i386/pr85345.c
new file mode 100644
index 00000000000..4ad9c01a974
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr85345.c
@@ -0,0 +1,45 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fcf-protection" } */
+/* { dg-require-ifunc "" } */
+/* { 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;
+}
diff --git a/gcc/varasm.c b/gcc/varasm.c
index d24bac4ad8f..ec073ecd7e4 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -5821,7 +5821,8 @@ do_assemble_alias (tree decl, tree target)
       globalize_decl (decl);
       maybe_assemble_visibility (decl);
     }
-  if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl)))
+  if (TREE_CODE (decl) == FUNCTION_DECL
+      && cgraph_node::get (decl)->ifunc_resolver)
     {
 #if defined (ASM_OUTPUT_TYPE_DIRECTIVE)
       if (targetm.has_ifunc_p ())
@@ -5904,7 +5905,8 @@ assemble_alias (tree decl, tree target)
 # else
       if (!DECL_WEAK (decl))
 	{
-	  if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl)))
+	  if (TREE_CODE (decl) == FUNCTION_DECL
+	      && cgraph_node::get (decl)->ifunc_resolver)
 	    error_at (DECL_SOURCE_LOCATION (decl),
 		      "ifunc is not supported in this configuration");
 	  else
@@ -7024,7 +7026,8 @@ default_binds_local_p_3 (const_tree exp, bool shlib, bool weak_dominate,
      weakref alias.  */
   if (lookup_attribute ("weakref", DECL_ATTRIBUTES (exp))
       || (TREE_CODE (exp) == FUNCTION_DECL
-	  && lookup_attribute ("ifunc", DECL_ATTRIBUTES (exp))))
+	  && cgraph_node::get (exp)
+	  && cgraph_node::get (exp)->ifunc_resolver))
     return false;
 
   /* Static variables are always local.  */
-- 
2.17.0

Re: PING^2: [PATCH] Don't mark IFUNC resolver as only called directly

[H.J. Lu]

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

On Thu, May 24, 2018 at 1:47 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Wed, May 23, 2018 at 8:35 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Wed, May 23, 2018 at 8:11 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>>>> On Wed, May 23, 2018 at 2:01 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>>>> >> On Tue, May 22, 2018 at 9:21 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>>>> >> >> >>>>> >  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?
>>>> >> >> >>
>>>> >> >> >
>>>> >> >> > PING:
>>>> >> >> >
>>>> >> >> > https://gcc.gnu.org/ml/gcc-patches/2018-04/msg00647.html
>>>> >> >> >
>>>> >> >>
>>>> >> >> PING.
>>>> >> > OK, but please extend the verifier that ifunc_resolver flag is equivalent to
>>>> >> > lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl))
>>>> >> > so we are sure things stays in sync.
>>>> >> >
>>>> >>
>>>> >> Like this
>>>> >>
>>>> >> diff --git a/gcc/symtab.c b/gcc/symtab.c
>>>> >> index 80f6f910c3b..954920b6dff 100644
>>>> >> --- a/gcc/symtab.c
>>>> >> +++ b/gcc/symtab.c
>>>> >> @@ -998,6 +998,13 @@ symtab_node::verify_base (void)
>>>> >>            error ("function symbol is not function");
>>>> >>            error_found = true;
>>>> >>    }
>>>> >> +      else if ((lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl))
>>>> >> +     != NULL)
>>>> >> +         != dyn_cast <cgraph_node *> (this)->ifunc_resolver)
>>>> >> +  {
>>>> >> +          error ("inconsistent `ifunc' attribute");
>>>> >> +          error_found = true;
>>>> >> +  }
>>>> >>      }
>>>> >>    else if (is_a <varpool_node *> (this))
>>>> >>      {
>>>> >>
>>>> >>
>>>> >> Thanks.
>>>> > Yes, thanks!
>>>> > Honza
>>>>
>>>> I'd like to also fix it on GCC 8 branch for CET.  Should I backport my
>>>> patch to GCC 8 after a few days or use the simple patch for GCC 8:
>>>>
>>>> https://gcc.gnu.org/ml/gcc-patches/2018-04/msg00588.html
>>>
>>> I would backport this one so we don't unnecesarily diverge.
>>> Thanks!
>>> Honza
>>
>> This is the backport which I will check into GCC 8 branch next week.
>>
>
> This is the updated backport which I will check into GCC 8 branch next week.
>

This is the updated backport which I will check into GCC 8 branch next week.


-- 
H.J.

[-- Attachment #2: 0001-Don-t-mark-IFUNC-resolver-as-only-called-directly.patch --]
[-- Type: text/x-patch, Size: 8784 bytes --]

From 5ebddef01e810c1684ed0927c0dbb1239cf3c178 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Wed, 11 Apr 2018 12:31:21 -0700
Subject: [PATCH] Don't mark IFUNC resolver as only called directly

Since IFUNC resolver is called indirectly, don't mark IFUNC resolver as
only called directly.  This patch adds ifunc_resolver to cgraph_node,
sets ifunc_resolver for ifunc attribute and checks ifunc_resolver
instead of looking up ifunc attribute.

gcc/

	Backport from mainline
	2018-05-26  H.J. Lu  <hongjiu.lu@intel.com>

	PR target/85900
	PR target/85345
	* varasm.c (assemble_alias): Lookup ifunc attribute on error.

	2018-05-24  H.J. Lu  <hongjiu.lu@intel.com>

	PR target/85900
	PR target/85345
	* varasm.c (assemble_alias): Check ifunc_resolver only on
	FUNCTION_DECL.

	2018-05-22  H.J. Lu  <hongjiu.lu@intel.com>

	PR target/85345
	* cgraph.h (cgraph_node::create): Set ifunc_resolver for ifunc
	attribute.
	(cgraph_node::create_alias): Likewise.
	(cgraph_node::get_availability): Check ifunc_resolver instead
	of looking up ifunc attribute.
	* cgraphunit.c (maybe_diag_incompatible_alias): Likewise.
	* varasm.c (do_assemble_alias): Likewise.
	(assemble_alias): Likewise.
	(default_binds_local_p_3): Likewise.
	* cgraph.h (cgraph_node): Add ifunc_resolver.
	(cgraph_node::only_called_directly_or_aliased_p): Return false
	for IFUNC resolver.
	* lto-cgraph.c (input_node): Set ifunc_resolver for ifunc
	attribute.
	* symtab.c (symtab_node::verify_base): Verify that ifunc_resolver
	is equivalent to lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl)).
	(symtab_node::binds_to_current_def_p): Check ifunc_resolver
	instead of looking up ifunc attribute.

gcc/testsuite/

	Backport from mainline
	2018-05-24  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>

	* gcc.target/i386/pr85345.c: Require ifunc support.

	2018-05-22  H.J. Lu  <hongjiu.lu@intel.com>

	PR target/85345
	* gcc.target/i386/pr85345.c: New test.
---
 gcc/cgraph.c                            |  7 +++-
 gcc/cgraph.h                            |  4 +++
 gcc/cgraphunit.c                        |  2 +-
 gcc/lto-cgraph.c                        |  2 ++
 gcc/symtab.c                            | 11 ++++--
 gcc/testsuite/gcc.target/i386/pr85345.c | 45 +++++++++++++++++++++++++
 gcc/varasm.c                            | 10 ++++--
 7 files changed, 74 insertions(+), 7 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/pr85345.c

diff --git a/gcc/cgraph.c b/gcc/cgraph.c
index 9a7d54d7cee..9f3a2929f6b 100644
--- a/gcc/cgraph.c
+++ b/gcc/cgraph.c
@@ -517,6 +517,9 @@ cgraph_node::create (tree decl)
 	g->have_offload = true;
     }
 
+  if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl)))
+    node->ifunc_resolver = true;
+
   node->register_symbol ();
 
   if (DECL_CONTEXT (decl) && TREE_CODE (DECL_CONTEXT (decl)) == FUNCTION_DECL)
@@ -575,6 +578,8 @@ cgraph_node::create_alias (tree alias, tree target)
   alias_node->alias = true;
   if (lookup_attribute ("weakref", DECL_ATTRIBUTES (alias)) != NULL)
     alias_node->transparent_alias = alias_node->weakref = true;
+  if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (alias)))
+    alias_node->ifunc_resolver = true;
   return alias_node;
 }
 
@@ -2299,7 +2304,7 @@ cgraph_node::get_availability (symtab_node *ref)
     avail = AVAIL_AVAILABLE;
   else if (transparent_alias)
     ultimate_alias_target (&avail, ref);
-  else if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl))
+  else if (ifunc_resolver
 	   || lookup_attribute ("noipa", DECL_ATTRIBUTES (decl)))
     avail = AVAIL_INTERPOSABLE;
   else if (!externally_visible)
diff --git a/gcc/cgraph.h b/gcc/cgraph.h
index ee7ebb41c24..afb2745a841 100644
--- a/gcc/cgraph.h
+++ b/gcc/cgraph.h
@@ -530,6 +530,9 @@ public:
   /* Set when symbol can be streamed into bytecode for offloading.  */
   unsigned offloadable : 1;
 
+  /* Set when symbol is an IFUNC resolver.  */
+  unsigned ifunc_resolver : 1;
+
 
   /* Ordering of all symtab entries.  */
   int order;
@@ -2886,6 +2889,7 @@ cgraph_node::only_called_directly_or_aliased_p (void)
 {
   gcc_assert (!global.inlined_to);
   return (!force_output && !address_taken
+	  && !ifunc_resolver
 	  && !used_from_other_partition
 	  && !DECL_VIRTUAL_P (decl)
 	  && !DECL_STATIC_CONSTRUCTOR (decl)
diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index e418ec04149..212ee7b8340 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -1307,7 +1307,7 @@ maybe_diag_incompatible_alias (tree alias, tree target)
   tree altype = TREE_TYPE (alias);
   tree targtype = TREE_TYPE (target);
 
-  bool ifunc = lookup_attribute ("ifunc", DECL_ATTRIBUTES (alias));
+  bool ifunc = cgraph_node::get (alias)->ifunc_resolver;
   tree funcptr = altype;
 
   if (ifunc)
diff --git a/gcc/lto-cgraph.c b/gcc/lto-cgraph.c
index dcd5391012c..40baf858ca5 100644
--- a/gcc/lto-cgraph.c
+++ b/gcc/lto-cgraph.c
@@ -1257,6 +1257,8 @@ input_node (struct lto_file_decl_data *file_data,
 	 of ipa passes is done.  Alays forcingly create a fresh node.  */
       node = symtab->create_empty ();
       node->decl = fn_decl;
+      if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (fn_decl)))
+	node->ifunc_resolver = 1;
       node->register_symbol ();
     }
 
diff --git a/gcc/symtab.c b/gcc/symtab.c
index c1533083573..954920b6dff 100644
--- a/gcc/symtab.c
+++ b/gcc/symtab.c
@@ -998,6 +998,13 @@ symtab_node::verify_base (void)
           error ("function symbol is not function");
           error_found = true;
 	}
+      else if ((lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl))
+		!= NULL)
+	       != dyn_cast <cgraph_node *> (this)->ifunc_resolver)
+	{
+          error ("inconsistent `ifunc' attribute");
+          error_found = true;
+	}
     }
   else if (is_a <varpool_node *> (this))
     {
@@ -2253,13 +2260,13 @@ symtab_node::binds_to_current_def_p (symtab_node *ref)
   if (transparent_alias)
     return definition
 	   && get_alias_target()->binds_to_current_def_p (ref);
-  if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl)))
+  cgraph_node *cnode = dyn_cast <cgraph_node *> (this);
+  if (cnode && cnode->ifunc_resolver)
     return false;
   if (decl_binds_to_current_def_p (decl))
     return true;
 
   /* Inline clones always binds locally.  */
-  cgraph_node *cnode = dyn_cast <cgraph_node *> (this);
   if (cnode && cnode->global.inlined_to)
     return true;
 
diff --git a/gcc/testsuite/gcc.target/i386/pr85345.c b/gcc/testsuite/gcc.target/i386/pr85345.c
new file mode 100644
index 00000000000..4ad9c01a974
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr85345.c
@@ -0,0 +1,45 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fcf-protection" } */
+/* { dg-require-ifunc "" } */
+/* { 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;
+}
diff --git a/gcc/varasm.c b/gcc/varasm.c
index d24bac4ad8f..58efbc6441b 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -5821,7 +5821,8 @@ do_assemble_alias (tree decl, tree target)
       globalize_decl (decl);
       maybe_assemble_visibility (decl);
     }
-  if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl)))
+  if (TREE_CODE (decl) == FUNCTION_DECL
+      && cgraph_node::get (decl)->ifunc_resolver)
     {
 #if defined (ASM_OUTPUT_TYPE_DIRECTIVE)
       if (targetm.has_ifunc_p ())
@@ -5904,7 +5905,9 @@ assemble_alias (tree decl, tree target)
 # else
       if (!DECL_WEAK (decl))
 	{
-	  if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl)))
+	  /* NB: ifunc_resolver isn't set when an error is detected.  */
+	  if (TREE_CODE (decl) == FUNCTION_DECL
+	      && lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl)))
 	    error_at (DECL_SOURCE_LOCATION (decl),
 		      "ifunc is not supported in this configuration");
 	  else
@@ -7024,7 +7027,8 @@ default_binds_local_p_3 (const_tree exp, bool shlib, bool weak_dominate,
      weakref alias.  */
   if (lookup_attribute ("weakref", DECL_ATTRIBUTES (exp))
       || (TREE_CODE (exp) == FUNCTION_DECL
-	  && lookup_attribute ("ifunc", DECL_ATTRIBUTES (exp))))
+	  && cgraph_node::get (exp)
+	  && cgraph_node::get (exp)->ifunc_resolver))
     return false;
 
   /* Static variables are always local.  */
-- 
2.17.0