This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: PING^2: [PATCH] Don't mark IFUNC resolver as only called directly
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.
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