* [PATCH] Do not allow to inline ifunc resolvers (PR ipa/81128).
@ 2017-06-27 12:50 Martin Liška
2017-06-27 14:57 ` Jan Hubicka
0 siblings, 1 reply; 5+ messages in thread
From: Martin Liška @ 2017-06-27 12:50 UTC (permalink / raw)
To: gcc-patches; +Cc: Jan Hubicka, Nathan Sidwell
[-- Attachment #1: Type: text/plain, Size: 1133 bytes --]
Hello.
Currently ifunc is interpreted as normal alias by IPA optimizations. That's problematic
as should not consider ifunc alias as candidate for inlining, or redirection.
Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
And survives MVC tests on x86_64-linux-gnu.
Ready to be installed?
Martin
gcc/ChangeLog:
2017-06-22 Martin Liska <mliska@suse.cz>
* ipa-inline.c (can_inline_edge_p): Return false for ifunc fns.
* ipa-visibility.c (can_replace_by_local_alias): Likewise.
gcc/c-family/ChangeLog:
2017-06-22 Martin Liska <mliska@suse.cz>
* c-attribs.c (handle_alias_ifunc_attribute): Append ifunc alias
to a function declaration.
gcc/testsuite/ChangeLog:
2017-06-22 Martin Liska <mliska@suse.cz>
* gcc.target/i386/pr81128.c: New test.
---
gcc/c-family/c-attribs.c | 11 ++++--
gcc/ipa-inline.c | 2 +
gcc/ipa-visibility.c | 3 +-
gcc/testsuite/gcc.target/i386/pr81128.c | 65 +++++++++++++++++++++++++++++++++
4 files changed, 77 insertions(+), 4 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/i386/pr81128.c
[-- Attachment #2: 0001-Do-not-allow-to-inline-ifunc-resolvers-PR-ipa-81128.patch --]
[-- Type: text/x-patch, Size: 3245 bytes --]
diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index 2b6845f2cbd..626ffa1cde7 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -1846,9 +1846,14 @@ handle_alias_ifunc_attribute (bool is_alias, tree *node, tree name, tree args,
TREE_STATIC (decl) = 1;
if (!is_alias)
- /* ifuncs are also aliases, so set that attribute too. */
- DECL_ATTRIBUTES (decl)
- = tree_cons (get_identifier ("alias"), args, DECL_ATTRIBUTES (decl));
+ {
+ /* ifuncs are also aliases, so set that attribute too. */
+ DECL_ATTRIBUTES (decl)
+ = tree_cons (get_identifier ("alias"), args,
+ DECL_ATTRIBUTES (decl));
+ DECL_ATTRIBUTES (decl) = tree_cons (get_identifier ("ifunc"),
+ NULL, DECL_ATTRIBUTES (decl));
+ }
}
else
{
diff --git a/gcc/ipa-inline.c b/gcc/ipa-inline.c
index fb20d3723cc..588fa9c41e4 100644
--- a/gcc/ipa-inline.c
+++ b/gcc/ipa-inline.c
@@ -370,6 +370,8 @@ can_inline_edge_p (struct cgraph_edge *e, bool report,
e->inline_failed = CIF_ATTRIBUTE_MISMATCH;
inlinable = false;
}
+ else if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (e->callee->decl)))
+ inlinable = false;
/* Check if caller growth allows the inlining. */
else if (!DECL_DISREGARD_INLINE_LIMITS (callee->decl)
&& !disregard_limits
diff --git a/gcc/ipa-visibility.c b/gcc/ipa-visibility.c
index d5a3ae56c46..79d05b41085 100644
--- a/gcc/ipa-visibility.c
+++ b/gcc/ipa-visibility.c
@@ -345,7 +345,8 @@ can_replace_by_local_alias (symtab_node *node)
return (node->get_availability () > AVAIL_INTERPOSABLE
&& !decl_binds_to_current_def_p (node->decl)
- && !node->can_be_discarded_p ());
+ && !node->can_be_discarded_p ()
+ && !lookup_attribute ("ifunc", DECL_ATTRIBUTES (node->decl)));
}
/* Return true if we can replace reference to NODE by local alias
diff --git a/gcc/testsuite/gcc.target/i386/pr81128.c b/gcc/testsuite/gcc.target/i386/pr81128.c
new file mode 100644
index 00000000000..90a567ad690
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr81128.c
@@ -0,0 +1,65 @@
+/* PR ipa/81128 */
+/* { dg-do run } */
+/* { dg-options "-O3" } */
+/* { dg-require-ifunc "" } */
+
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <time.h>
+
+int resolver_fn = 0;
+int resolved_fn = 0;
+
+static inline void
+do_it_right_at_runtime_A ()
+{
+ resolved_fn++;
+}
+
+static inline void
+do_it_right_at_runtime_B ()
+{
+ 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")));
+
+static void (*resolve_do_it_right_at_runtime (void)) (void)
+{
+ srand (time (NULL));
+ int r = rand ();
+ resolver_fn++;
+
+ /* Use intermediate variable to get a warning for non-matching
+ * prototype. */
+ 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 (void)
+{
+ const unsigned int ITERS = 10;
+
+ for (int i = ITERS; i > 0; i--)
+ {
+ do_it_right_at_runtime ();
+ }
+
+ if (resolver_fn != 1)
+ __builtin_abort ();
+
+ if (resolved_fn != 10)
+ __builtin_abort ();
+
+ return 0;
+}
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Do not allow to inline ifunc resolvers (PR ipa/81128).
2017-06-27 12:50 [PATCH] Do not allow to inline ifunc resolvers (PR ipa/81128) Martin Liška
@ 2017-06-27 14:57 ` Jan Hubicka
2017-06-27 15:15 ` Martin Liška
0 siblings, 1 reply; 5+ messages in thread
From: Jan Hubicka @ 2017-06-27 14:57 UTC (permalink / raw)
To: Martin Liška; +Cc: gcc-patches, Nathan Sidwell
> Hello.
>
> Currently ifunc is interpreted as normal alias by IPA optimizations. That's problematic
> as should not consider ifunc alias as candidate for inlining, or redirection.
>
> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
> And survives MVC tests on x86_64-linux-gnu.
>
> Ready to be installed?
Wasn't this supposed to go with arranging availability to be AVAIL_INTERPOSABLE
(as the target will be interposed by linker to the corect target)
Honza
> Martin
>
> gcc/ChangeLog:
>
> 2017-06-22 Martin Liska <mliska@suse.cz>
>
> * ipa-inline.c (can_inline_edge_p): Return false for ifunc fns.
> * ipa-visibility.c (can_replace_by_local_alias): Likewise.
>
> gcc/c-family/ChangeLog:
>
> 2017-06-22 Martin Liska <mliska@suse.cz>
>
> * c-attribs.c (handle_alias_ifunc_attribute): Append ifunc alias
> to a function declaration.
>
> gcc/testsuite/ChangeLog:
>
> 2017-06-22 Martin Liska <mliska@suse.cz>
>
> * gcc.target/i386/pr81128.c: New test.
> ---
> gcc/c-family/c-attribs.c | 11 ++++--
> gcc/ipa-inline.c | 2 +
> gcc/ipa-visibility.c | 3 +-
> gcc/testsuite/gcc.target/i386/pr81128.c | 65 +++++++++++++++++++++++++++++++++
> 4 files changed, 77 insertions(+), 4 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/i386/pr81128.c
>
>
> diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
> index 2b6845f2cbd..626ffa1cde7 100644
> --- a/gcc/c-family/c-attribs.c
> +++ b/gcc/c-family/c-attribs.c
> @@ -1846,9 +1846,14 @@ handle_alias_ifunc_attribute (bool is_alias, tree *node, tree name, tree args,
> TREE_STATIC (decl) = 1;
>
> if (!is_alias)
> - /* ifuncs are also aliases, so set that attribute too. */
> - DECL_ATTRIBUTES (decl)
> - = tree_cons (get_identifier ("alias"), args, DECL_ATTRIBUTES (decl));
> + {
> + /* ifuncs are also aliases, so set that attribute too. */
> + DECL_ATTRIBUTES (decl)
> + = tree_cons (get_identifier ("alias"), args,
> + DECL_ATTRIBUTES (decl));
> + DECL_ATTRIBUTES (decl) = tree_cons (get_identifier ("ifunc"),
> + NULL, DECL_ATTRIBUTES (decl));
> + }
> }
> else
> {
> diff --git a/gcc/ipa-inline.c b/gcc/ipa-inline.c
> index fb20d3723cc..588fa9c41e4 100644
> --- a/gcc/ipa-inline.c
> +++ b/gcc/ipa-inline.c
> @@ -370,6 +370,8 @@ can_inline_edge_p (struct cgraph_edge *e, bool report,
> e->inline_failed = CIF_ATTRIBUTE_MISMATCH;
> inlinable = false;
> }
> + else if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (e->callee->decl)))
> + inlinable = false;
> /* Check if caller growth allows the inlining. */
> else if (!DECL_DISREGARD_INLINE_LIMITS (callee->decl)
> && !disregard_limits
> diff --git a/gcc/ipa-visibility.c b/gcc/ipa-visibility.c
> index d5a3ae56c46..79d05b41085 100644
> --- a/gcc/ipa-visibility.c
> +++ b/gcc/ipa-visibility.c
> @@ -345,7 +345,8 @@ can_replace_by_local_alias (symtab_node *node)
>
> return (node->get_availability () > AVAIL_INTERPOSABLE
> && !decl_binds_to_current_def_p (node->decl)
> - && !node->can_be_discarded_p ());
> + && !node->can_be_discarded_p ()
> + && !lookup_attribute ("ifunc", DECL_ATTRIBUTES (node->decl)));
> }
>
> /* Return true if we can replace reference to NODE by local alias
> diff --git a/gcc/testsuite/gcc.target/i386/pr81128.c b/gcc/testsuite/gcc.target/i386/pr81128.c
> new file mode 100644
> index 00000000000..90a567ad690
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr81128.c
> @@ -0,0 +1,65 @@
> +/* PR ipa/81128 */
> +/* { dg-do run } */
> +/* { dg-options "-O3" } */
> +/* { dg-require-ifunc "" } */
> +
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <time.h>
> +
> +int resolver_fn = 0;
> +int resolved_fn = 0;
> +
> +static inline void
> +do_it_right_at_runtime_A ()
> +{
> + resolved_fn++;
> +}
> +
> +static inline void
> +do_it_right_at_runtime_B ()
> +{
> + 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")));
> +
> +static void (*resolve_do_it_right_at_runtime (void)) (void)
> +{
> + srand (time (NULL));
> + int r = rand ();
> + resolver_fn++;
> +
> + /* Use intermediate variable to get a warning for non-matching
> + * prototype. */
> + 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 (void)
> +{
> + const unsigned int ITERS = 10;
> +
> + for (int i = ITERS; i > 0; i--)
> + {
> + do_it_right_at_runtime ();
> + }
> +
> + if (resolver_fn != 1)
> + __builtin_abort ();
> +
> + if (resolved_fn != 10)
> + __builtin_abort ();
> +
> + return 0;
> +}
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Do not allow to inline ifunc resolvers (PR ipa/81128).
2017-06-27 14:57 ` Jan Hubicka
@ 2017-06-27 15:15 ` Martin Liška
2017-06-27 15:26 ` Jan Hubicka
0 siblings, 1 reply; 5+ messages in thread
From: Martin Liška @ 2017-06-27 15:15 UTC (permalink / raw)
To: Jan Hubicka; +Cc: gcc-patches, Nathan Sidwell
On 06/27/2017 04:57 PM, Jan Hubicka wrote:
>> Hello.
>>
>> Currently ifunc is interpreted as normal alias by IPA optimizations. That's problematic
>> as should not consider ifunc alias as candidate for inlining, or redirection.
>>
>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>> And survives MVC tests on x86_64-linux-gnu.
>>
>> Ready to be installed?
>
> Wasn't this supposed to go with arranging availability to be AVAIL_INTERPOSABLE
> (as the target will be interposed by linker to the corect target)
Should work, there's alternative patch:
diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index 2b6845f2cbd..626ffa1cde7 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -1846,9 +1846,14 @@ handle_alias_ifunc_attribute (bool is_alias, tree *node, tree name, tree args,
TREE_STATIC (decl) = 1;
if (!is_alias)
- /* ifuncs are also aliases, so set that attribute too. */
- DECL_ATTRIBUTES (decl)
- = tree_cons (get_identifier ("alias"), args, DECL_ATTRIBUTES (decl));
+ {
+ /* ifuncs are also aliases, so set that attribute too. */
+ DECL_ATTRIBUTES (decl)
+ = tree_cons (get_identifier ("alias"), args,
+ DECL_ATTRIBUTES (decl));
+ DECL_ATTRIBUTES (decl) = tree_cons (get_identifier ("ifunc"),
+ NULL, DECL_ATTRIBUTES (decl));
+ }
}
else
{
diff --git a/gcc/ipa-visibility.c b/gcc/ipa-visibility.c
index d5a3ae56c46..69e6e295d55 100644
--- a/gcc/ipa-visibility.c
+++ b/gcc/ipa-visibility.c
@@ -97,7 +97,8 @@ non_local_p (struct cgraph_node *node, void *data ATTRIBUTE_UNUSED)
&& !DECL_EXTERNAL (node->decl)
&& !node->externally_visible
&& !node->used_from_other_partition
- && !node->in_other_partition);
+ && !node->in_other_partition
+ && !lookup_attribute ("ifunc", DECL_ATTRIBUTES (node->decl)));
}
/* Return true when function can be marked local. */
It's questionable if local.local can be true for ifunc function? If can, one would need to
move check for ifunc aerly in:
/* Return function availability. See cgraph.h for description of individual
return values. */
enum availability
cgraph_node::get_availability (symtab_node *ref)
{
if (ref)
{
cgraph_node *cref = dyn_cast <cgraph_node *> (ref);
if (cref)
ref = cref->global.inlined_to;
}
enum availability avail;
if (!analyzed)
avail = AVAIL_NOT_AVAILABLE;
else if (local.local)
avail = AVAIL_LOCAL;
else if (global.inlined_to)
avail = AVAIL_AVAILABLE;
else if (transparent_alias)
ultimate_alias_target (&avail, ref);
else if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl)))
avail = AVAIL_INTERPOSABLE;
...
What solution do you prefer Honza?
Thanks,
Martin
>
> Honza
>> Martin
>>
>> gcc/ChangeLog:
>>
>> 2017-06-22 Martin Liska <mliska@suse.cz>
>>
>> * ipa-inline.c (can_inline_edge_p): Return false for ifunc fns.
>> * ipa-visibility.c (can_replace_by_local_alias): Likewise.
>>
>> gcc/c-family/ChangeLog:
>>
>> 2017-06-22 Martin Liska <mliska@suse.cz>
>>
>> * c-attribs.c (handle_alias_ifunc_attribute): Append ifunc alias
>> to a function declaration.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2017-06-22 Martin Liska <mliska@suse.cz>
>>
>> * gcc.target/i386/pr81128.c: New test.
>> ---
>> gcc/c-family/c-attribs.c | 11 ++++--
>> gcc/ipa-inline.c | 2 +
>> gcc/ipa-visibility.c | 3 +-
>> gcc/testsuite/gcc.target/i386/pr81128.c | 65 +++++++++++++++++++++++++++++++++
>> 4 files changed, 77 insertions(+), 4 deletions(-)
>> create mode 100644 gcc/testsuite/gcc.target/i386/pr81128.c
>>
>>
>
>> diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
>> index 2b6845f2cbd..626ffa1cde7 100644
>> --- a/gcc/c-family/c-attribs.c
>> +++ b/gcc/c-family/c-attribs.c
>> @@ -1846,9 +1846,14 @@ handle_alias_ifunc_attribute (bool is_alias, tree *node, tree name, tree args,
>> TREE_STATIC (decl) = 1;
>>
>> if (!is_alias)
>> - /* ifuncs are also aliases, so set that attribute too. */
>> - DECL_ATTRIBUTES (decl)
>> - = tree_cons (get_identifier ("alias"), args, DECL_ATTRIBUTES (decl));
>> + {
>> + /* ifuncs are also aliases, so set that attribute too. */
>> + DECL_ATTRIBUTES (decl)
>> + = tree_cons (get_identifier ("alias"), args,
>> + DECL_ATTRIBUTES (decl));
>> + DECL_ATTRIBUTES (decl) = tree_cons (get_identifier ("ifunc"),
>> + NULL, DECL_ATTRIBUTES (decl));
>> + }
>> }
>> else
>> {
>> diff --git a/gcc/ipa-inline.c b/gcc/ipa-inline.c
>> index fb20d3723cc..588fa9c41e4 100644
>> --- a/gcc/ipa-inline.c
>> +++ b/gcc/ipa-inline.c
>> @@ -370,6 +370,8 @@ can_inline_edge_p (struct cgraph_edge *e, bool report,
>> e->inline_failed = CIF_ATTRIBUTE_MISMATCH;
>> inlinable = false;
>> }
>> + else if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (e->callee->decl)))
>> + inlinable = false;
>> /* Check if caller growth allows the inlining. */
>> else if (!DECL_DISREGARD_INLINE_LIMITS (callee->decl)
>> && !disregard_limits
>> diff --git a/gcc/ipa-visibility.c b/gcc/ipa-visibility.c
>> index d5a3ae56c46..79d05b41085 100644
>> --- a/gcc/ipa-visibility.c
>> +++ b/gcc/ipa-visibility.c
>> @@ -345,7 +345,8 @@ can_replace_by_local_alias (symtab_node *node)
>>
>> return (node->get_availability () > AVAIL_INTERPOSABLE
>> && !decl_binds_to_current_def_p (node->decl)
>> - && !node->can_be_discarded_p ());
>> + && !node->can_be_discarded_p ()
>> + && !lookup_attribute ("ifunc", DECL_ATTRIBUTES (node->decl)));
>> }
>>
>> /* Return true if we can replace reference to NODE by local alias
>> diff --git a/gcc/testsuite/gcc.target/i386/pr81128.c b/gcc/testsuite/gcc.target/i386/pr81128.c
>> new file mode 100644
>> index 00000000000..90a567ad690
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/i386/pr81128.c
>> @@ -0,0 +1,65 @@
>> +/* PR ipa/81128 */
>> +/* { dg-do run } */
>> +/* { dg-options "-O3" } */
>> +/* { dg-require-ifunc "" } */
>> +
>> +
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <time.h>
>> +
>> +int resolver_fn = 0;
>> +int resolved_fn = 0;
>> +
>> +static inline void
>> +do_it_right_at_runtime_A ()
>> +{
>> + resolved_fn++;
>> +}
>> +
>> +static inline void
>> +do_it_right_at_runtime_B ()
>> +{
>> + 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")));
>> +
>> +static void (*resolve_do_it_right_at_runtime (void)) (void)
>> +{
>> + srand (time (NULL));
>> + int r = rand ();
>> + resolver_fn++;
>> +
>> + /* Use intermediate variable to get a warning for non-matching
>> + * prototype. */
>> + 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 (void)
>> +{
>> + const unsigned int ITERS = 10;
>> +
>> + for (int i = ITERS; i > 0; i--)
>> + {
>> + do_it_right_at_runtime ();
>> + }
>> +
>> + if (resolver_fn != 1)
>> + __builtin_abort ();
>> +
>> + if (resolved_fn != 10)
>> + __builtin_abort ();
>> +
>> + return 0;
>> +}
>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Do not allow to inline ifunc resolvers (PR ipa/81128).
2017-06-27 15:15 ` Martin Liška
@ 2017-06-27 15:26 ` Jan Hubicka
2017-06-28 12:46 ` Martin Liška
0 siblings, 1 reply; 5+ messages in thread
From: Jan Hubicka @ 2017-06-27 15:26 UTC (permalink / raw)
To: Martin Liška; +Cc: gcc-patches, Nathan Sidwell
> diff --git a/gcc/ipa-visibility.c b/gcc/ipa-visibility.c
> index d5a3ae56c46..69e6e295d55 100644
> --- a/gcc/ipa-visibility.c
> +++ b/gcc/ipa-visibility.c
> @@ -97,7 +97,8 @@ non_local_p (struct cgraph_node *node, void *data ATTRIBUTE_UNUSED)
> && !DECL_EXTERNAL (node->decl)
> && !node->externally_visible
> && !node->used_from_other_partition
> - && !node->in_other_partition);
> + && !node->in_other_partition
> + && !lookup_attribute ("ifunc", DECL_ATTRIBUTES (node->decl)));
> }
>
> /* Return true when function can be marked local. */
>
> It's questionable if local.local can be true for ifunc function? If can, one would need to
> move check for ifunc aerly in:
> /* Return function availability. See cgraph.h for description of individual
> return values. */
> enum availability
> cgraph_node::get_availability (symtab_node *ref)
> {
> if (ref)
> {
> cgraph_node *cref = dyn_cast <cgraph_node *> (ref);
> if (cref)
> ref = cref->global.inlined_to;
> }
> enum availability avail;
> if (!analyzed)
> avail = AVAIL_NOT_AVAILABLE;
> else if (local.local)
> avail = AVAIL_LOCAL;
> else if (global.inlined_to)
> avail = AVAIL_AVAILABLE;
> else if (transparent_alias)
> ultimate_alias_target (&avail, ref);
> else if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl)))
> avail = AVAIL_INTERPOSABLE;
>
> ...
>
> What solution do you prefer Honza?
Probably just update non_local_p to also check that availability is at least AVAIL_AVAILABLE.
Then we will have one place to collect such a side cases where !EXTERNAL function can be
interposed.
Honza
>
> Thanks,
> Martin
>
> >
> > Honza
> >> Martin
> >>
> >> gcc/ChangeLog:
> >>
> >> 2017-06-22 Martin Liska <mliska@suse.cz>
> >>
> >> * ipa-inline.c (can_inline_edge_p): Return false for ifunc fns.
> >> * ipa-visibility.c (can_replace_by_local_alias): Likewise.
> >>
> >> gcc/c-family/ChangeLog:
> >>
> >> 2017-06-22 Martin Liska <mliska@suse.cz>
> >>
> >> * c-attribs.c (handle_alias_ifunc_attribute): Append ifunc alias
> >> to a function declaration.
> >>
> >> gcc/testsuite/ChangeLog:
> >>
> >> 2017-06-22 Martin Liska <mliska@suse.cz>
> >>
> >> * gcc.target/i386/pr81128.c: New test.
> >> ---
> >> gcc/c-family/c-attribs.c | 11 ++++--
> >> gcc/ipa-inline.c | 2 +
> >> gcc/ipa-visibility.c | 3 +-
> >> gcc/testsuite/gcc.target/i386/pr81128.c | 65 +++++++++++++++++++++++++++++++++
> >> 4 files changed, 77 insertions(+), 4 deletions(-)
> >> create mode 100644 gcc/testsuite/gcc.target/i386/pr81128.c
> >>
> >>
> >
> >> diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
> >> index 2b6845f2cbd..626ffa1cde7 100644
> >> --- a/gcc/c-family/c-attribs.c
> >> +++ b/gcc/c-family/c-attribs.c
> >> @@ -1846,9 +1846,14 @@ handle_alias_ifunc_attribute (bool is_alias, tree *node, tree name, tree args,
> >> TREE_STATIC (decl) = 1;
> >>
> >> if (!is_alias)
> >> - /* ifuncs are also aliases, so set that attribute too. */
> >> - DECL_ATTRIBUTES (decl)
> >> - = tree_cons (get_identifier ("alias"), args, DECL_ATTRIBUTES (decl));
> >> + {
> >> + /* ifuncs are also aliases, so set that attribute too. */
> >> + DECL_ATTRIBUTES (decl)
> >> + = tree_cons (get_identifier ("alias"), args,
> >> + DECL_ATTRIBUTES (decl));
> >> + DECL_ATTRIBUTES (decl) = tree_cons (get_identifier ("ifunc"),
> >> + NULL, DECL_ATTRIBUTES (decl));
> >> + }
> >> }
> >> else
> >> {
> >> diff --git a/gcc/ipa-inline.c b/gcc/ipa-inline.c
> >> index fb20d3723cc..588fa9c41e4 100644
> >> --- a/gcc/ipa-inline.c
> >> +++ b/gcc/ipa-inline.c
> >> @@ -370,6 +370,8 @@ can_inline_edge_p (struct cgraph_edge *e, bool report,
> >> e->inline_failed = CIF_ATTRIBUTE_MISMATCH;
> >> inlinable = false;
> >> }
> >> + else if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (e->callee->decl)))
> >> + inlinable = false;
> >> /* Check if caller growth allows the inlining. */
> >> else if (!DECL_DISREGARD_INLINE_LIMITS (callee->decl)
> >> && !disregard_limits
> >> diff --git a/gcc/ipa-visibility.c b/gcc/ipa-visibility.c
> >> index d5a3ae56c46..79d05b41085 100644
> >> --- a/gcc/ipa-visibility.c
> >> +++ b/gcc/ipa-visibility.c
> >> @@ -345,7 +345,8 @@ can_replace_by_local_alias (symtab_node *node)
> >>
> >> return (node->get_availability () > AVAIL_INTERPOSABLE
> >> && !decl_binds_to_current_def_p (node->decl)
> >> - && !node->can_be_discarded_p ());
> >> + && !node->can_be_discarded_p ()
> >> + && !lookup_attribute ("ifunc", DECL_ATTRIBUTES (node->decl)));
> >> }
> >>
> >> /* Return true if we can replace reference to NODE by local alias
> >> diff --git a/gcc/testsuite/gcc.target/i386/pr81128.c b/gcc/testsuite/gcc.target/i386/pr81128.c
> >> new file mode 100644
> >> index 00000000000..90a567ad690
> >> --- /dev/null
> >> +++ b/gcc/testsuite/gcc.target/i386/pr81128.c
> >> @@ -0,0 +1,65 @@
> >> +/* PR ipa/81128 */
> >> +/* { dg-do run } */
> >> +/* { dg-options "-O3" } */
> >> +/* { dg-require-ifunc "" } */
> >> +
> >> +
> >> +#include <stdio.h>
> >> +#include <stdlib.h>
> >> +#include <time.h>
> >> +
> >> +int resolver_fn = 0;
> >> +int resolved_fn = 0;
> >> +
> >> +static inline void
> >> +do_it_right_at_runtime_A ()
> >> +{
> >> + resolved_fn++;
> >> +}
> >> +
> >> +static inline void
> >> +do_it_right_at_runtime_B ()
> >> +{
> >> + 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")));
> >> +
> >> +static void (*resolve_do_it_right_at_runtime (void)) (void)
> >> +{
> >> + srand (time (NULL));
> >> + int r = rand ();
> >> + resolver_fn++;
> >> +
> >> + /* Use intermediate variable to get a warning for non-matching
> >> + * prototype. */
> >> + 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 (void)
> >> +{
> >> + const unsigned int ITERS = 10;
> >> +
> >> + for (int i = ITERS; i > 0; i--)
> >> + {
> >> + do_it_right_at_runtime ();
> >> + }
> >> +
> >> + if (resolver_fn != 1)
> >> + __builtin_abort ();
> >> +
> >> + if (resolved_fn != 10)
> >> + __builtin_abort ();
> >> +
> >> + return 0;
> >> +}
> >>
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Do not allow to inline ifunc resolvers (PR ipa/81128).
2017-06-27 15:26 ` Jan Hubicka
@ 2017-06-28 12:46 ` Martin Liška
0 siblings, 0 replies; 5+ messages in thread
From: Martin Liška @ 2017-06-28 12:46 UTC (permalink / raw)
To: Jan Hubicka; +Cc: gcc-patches, Nathan Sidwell
[-- Attachment #1: Type: text/plain, Size: 6699 bytes --]
On 06/27/2017 05:26 PM, Jan Hubicka wrote:
>> diff --git a/gcc/ipa-visibility.c b/gcc/ipa-visibility.c
>> index d5a3ae56c46..69e6e295d55 100644
>> --- a/gcc/ipa-visibility.c
>> +++ b/gcc/ipa-visibility.c
>> @@ -97,7 +97,8 @@ non_local_p (struct cgraph_node *node, void *data ATTRIBUTE_UNUSED)
>> && !DECL_EXTERNAL (node->decl)
>> && !node->externally_visible
>> && !node->used_from_other_partition
>> - && !node->in_other_partition);
>> + && !node->in_other_partition
>> + && !lookup_attribute ("ifunc", DECL_ATTRIBUTES (node->decl)));
>> }
>>
>> /* Return true when function can be marked local. */
>>
>> It's questionable if local.local can be true for ifunc function? If can, one would need to
>> move check for ifunc aerly in:
>> /* Return function availability. See cgraph.h for description of individual
>> return values. */
>> enum availability
>> cgraph_node::get_availability (symtab_node *ref)
>> {
>> if (ref)
>> {
>> cgraph_node *cref = dyn_cast <cgraph_node *> (ref);
>> if (cref)
>> ref = cref->global.inlined_to;
>> }
>> enum availability avail;
>> if (!analyzed)
>> avail = AVAIL_NOT_AVAILABLE;
>> else if (local.local)
>> avail = AVAIL_LOCAL;
>> else if (global.inlined_to)
>> avail = AVAIL_AVAILABLE;
>> else if (transparent_alias)
>> ultimate_alias_target (&avail, ref);
>> else if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl)))
>> avail = AVAIL_INTERPOSABLE;
>>
>> ...
>>
>> What solution do you prefer Honza?
>
> Probably just update non_local_p to also check that availability is at least AVAIL_AVAILABLE.
> Then we will have one place to collect such a side cases where !EXTERNAL function can be
> interposed.
Good, this is tested version of patch I'm going to install.
Martin
>
> Honza
>>
>> Thanks,
>> Martin
>>
>>>
>>> Honza
>>>> Martin
>>>>
>>>> gcc/ChangeLog:
>>>>
>>>> 2017-06-22 Martin Liska <mliska@suse.cz>
>>>>
>>>> * ipa-inline.c (can_inline_edge_p): Return false for ifunc fns.
>>>> * ipa-visibility.c (can_replace_by_local_alias): Likewise.
>>>>
>>>> gcc/c-family/ChangeLog:
>>>>
>>>> 2017-06-22 Martin Liska <mliska@suse.cz>
>>>>
>>>> * c-attribs.c (handle_alias_ifunc_attribute): Append ifunc alias
>>>> to a function declaration.
>>>>
>>>> gcc/testsuite/ChangeLog:
>>>>
>>>> 2017-06-22 Martin Liska <mliska@suse.cz>
>>>>
>>>> * gcc.target/i386/pr81128.c: New test.
>>>> ---
>>>> gcc/c-family/c-attribs.c | 11 ++++--
>>>> gcc/ipa-inline.c | 2 +
>>>> gcc/ipa-visibility.c | 3 +-
>>>> gcc/testsuite/gcc.target/i386/pr81128.c | 65 +++++++++++++++++++++++++++++++++
>>>> 4 files changed, 77 insertions(+), 4 deletions(-)
>>>> create mode 100644 gcc/testsuite/gcc.target/i386/pr81128.c
>>>>
>>>>
>>>
>>>> diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
>>>> index 2b6845f2cbd..626ffa1cde7 100644
>>>> --- a/gcc/c-family/c-attribs.c
>>>> +++ b/gcc/c-family/c-attribs.c
>>>> @@ -1846,9 +1846,14 @@ handle_alias_ifunc_attribute (bool is_alias, tree *node, tree name, tree args,
>>>> TREE_STATIC (decl) = 1;
>>>>
>>>> if (!is_alias)
>>>> - /* ifuncs are also aliases, so set that attribute too. */
>>>> - DECL_ATTRIBUTES (decl)
>>>> - = tree_cons (get_identifier ("alias"), args, DECL_ATTRIBUTES (decl));
>>>> + {
>>>> + /* ifuncs are also aliases, so set that attribute too. */
>>>> + DECL_ATTRIBUTES (decl)
>>>> + = tree_cons (get_identifier ("alias"), args,
>>>> + DECL_ATTRIBUTES (decl));
>>>> + DECL_ATTRIBUTES (decl) = tree_cons (get_identifier ("ifunc"),
>>>> + NULL, DECL_ATTRIBUTES (decl));
>>>> + }
>>>> }
>>>> else
>>>> {
>>>> diff --git a/gcc/ipa-inline.c b/gcc/ipa-inline.c
>>>> index fb20d3723cc..588fa9c41e4 100644
>>>> --- a/gcc/ipa-inline.c
>>>> +++ b/gcc/ipa-inline.c
>>>> @@ -370,6 +370,8 @@ can_inline_edge_p (struct cgraph_edge *e, bool report,
>>>> e->inline_failed = CIF_ATTRIBUTE_MISMATCH;
>>>> inlinable = false;
>>>> }
>>>> + else if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (e->callee->decl)))
>>>> + inlinable = false;
>>>> /* Check if caller growth allows the inlining. */
>>>> else if (!DECL_DISREGARD_INLINE_LIMITS (callee->decl)
>>>> && !disregard_limits
>>>> diff --git a/gcc/ipa-visibility.c b/gcc/ipa-visibility.c
>>>> index d5a3ae56c46..79d05b41085 100644
>>>> --- a/gcc/ipa-visibility.c
>>>> +++ b/gcc/ipa-visibility.c
>>>> @@ -345,7 +345,8 @@ can_replace_by_local_alias (symtab_node *node)
>>>>
>>>> return (node->get_availability () > AVAIL_INTERPOSABLE
>>>> && !decl_binds_to_current_def_p (node->decl)
>>>> - && !node->can_be_discarded_p ());
>>>> + && !node->can_be_discarded_p ()
>>>> + && !lookup_attribute ("ifunc", DECL_ATTRIBUTES (node->decl)));
>>>> }
>>>>
>>>> /* Return true if we can replace reference to NODE by local alias
>>>> diff --git a/gcc/testsuite/gcc.target/i386/pr81128.c b/gcc/testsuite/gcc.target/i386/pr81128.c
>>>> new file mode 100644
>>>> index 00000000000..90a567ad690
>>>> --- /dev/null
>>>> +++ b/gcc/testsuite/gcc.target/i386/pr81128.c
>>>> @@ -0,0 +1,65 @@
>>>> +/* PR ipa/81128 */
>>>> +/* { dg-do run } */
>>>> +/* { dg-options "-O3" } */
>>>> +/* { dg-require-ifunc "" } */
>>>> +
>>>> +
>>>> +#include <stdio.h>
>>>> +#include <stdlib.h>
>>>> +#include <time.h>
>>>> +
>>>> +int resolver_fn = 0;
>>>> +int resolved_fn = 0;
>>>> +
>>>> +static inline void
>>>> +do_it_right_at_runtime_A ()
>>>> +{
>>>> + resolved_fn++;
>>>> +}
>>>> +
>>>> +static inline void
>>>> +do_it_right_at_runtime_B ()
>>>> +{
>>>> + 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")));
>>>> +
>>>> +static void (*resolve_do_it_right_at_runtime (void)) (void)
>>>> +{
>>>> + srand (time (NULL));
>>>> + int r = rand ();
>>>> + resolver_fn++;
>>>> +
>>>> + /* Use intermediate variable to get a warning for non-matching
>>>> + * prototype. */
>>>> + 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 (void)
>>>> +{
>>>> + const unsigned int ITERS = 10;
>>>> +
>>>> + for (int i = ITERS; i > 0; i--)
>>>> + {
>>>> + do_it_right_at_runtime ();
>>>> + }
>>>> +
>>>> + if (resolver_fn != 1)
>>>> + __builtin_abort ();
>>>> +
>>>> + if (resolved_fn != 10)
>>>> + __builtin_abort ();
>>>> +
>>>> + return 0;
>>>> +}
>>>>
>>>
[-- Attachment #2: 0001-Do-not-allow-to-inline-ifunc-resolvers-PR-ipa-81128.patch --]
[-- Type: text/x-patch, Size: 3617 bytes --]
From f1fcd618d424fe617cd75190d89e85941e9d7b98 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Thu, 22 Jun 2017 09:19:31 +0200
Subject: [PATCH] Do not allow to inline ifunc resolvers (PR ipa/81128).
gcc/ChangeLog:
2017-06-22 Martin Liska <mliska@suse.cz>
* ipa-visibility.c (non_local_p): Handle visibility.
gcc/c-family/ChangeLog:
2017-06-22 Martin Liska <mliska@suse.cz>
* c-attribs.c (handle_alias_ifunc_attribute): Append ifunc alias
to a function declaration.
gcc/testsuite/ChangeLog:
2017-06-22 Martin Liska <mliska@suse.cz>
* gcc.target/i386/pr81128.c: New test.
---
gcc/c-family/c-attribs.c | 11 ++++--
gcc/ipa-visibility.c | 3 +-
gcc/testsuite/gcc.target/i386/pr81128.c | 65 +++++++++++++++++++++++++++++++++
3 files changed, 75 insertions(+), 4 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/i386/pr81128.c
diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index 2b6845f2cbd..626ffa1cde7 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -1846,9 +1846,14 @@ handle_alias_ifunc_attribute (bool is_alias, tree *node, tree name, tree args,
TREE_STATIC (decl) = 1;
if (!is_alias)
- /* ifuncs are also aliases, so set that attribute too. */
- DECL_ATTRIBUTES (decl)
- = tree_cons (get_identifier ("alias"), args, DECL_ATTRIBUTES (decl));
+ {
+ /* ifuncs are also aliases, so set that attribute too. */
+ DECL_ATTRIBUTES (decl)
+ = tree_cons (get_identifier ("alias"), args,
+ DECL_ATTRIBUTES (decl));
+ DECL_ATTRIBUTES (decl) = tree_cons (get_identifier ("ifunc"),
+ NULL, DECL_ATTRIBUTES (decl));
+ }
}
else
{
diff --git a/gcc/ipa-visibility.c b/gcc/ipa-visibility.c
index d5a3ae56c46..da4a22e7329 100644
--- a/gcc/ipa-visibility.c
+++ b/gcc/ipa-visibility.c
@@ -97,7 +97,8 @@ non_local_p (struct cgraph_node *node, void *data ATTRIBUTE_UNUSED)
&& !DECL_EXTERNAL (node->decl)
&& !node->externally_visible
&& !node->used_from_other_partition
- && !node->in_other_partition);
+ && !node->in_other_partition
+ && node->get_availability () >= AVAIL_AVAILABLE);
}
/* Return true when function can be marked local. */
diff --git a/gcc/testsuite/gcc.target/i386/pr81128.c b/gcc/testsuite/gcc.target/i386/pr81128.c
new file mode 100644
index 00000000000..90a567ad690
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr81128.c
@@ -0,0 +1,65 @@
+/* PR ipa/81128 */
+/* { dg-do run } */
+/* { dg-options "-O3" } */
+/* { dg-require-ifunc "" } */
+
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <time.h>
+
+int resolver_fn = 0;
+int resolved_fn = 0;
+
+static inline void
+do_it_right_at_runtime_A ()
+{
+ resolved_fn++;
+}
+
+static inline void
+do_it_right_at_runtime_B ()
+{
+ 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")));
+
+static void (*resolve_do_it_right_at_runtime (void)) (void)
+{
+ srand (time (NULL));
+ int r = rand ();
+ resolver_fn++;
+
+ /* Use intermediate variable to get a warning for non-matching
+ * prototype. */
+ 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 (void)
+{
+ const unsigned int ITERS = 10;
+
+ for (int i = ITERS; i > 0; i--)
+ {
+ do_it_right_at_runtime ();
+ }
+
+ if (resolver_fn != 1)
+ __builtin_abort ();
+
+ if (resolved_fn != 10)
+ __builtin_abort ();
+
+ return 0;
+}
--
2.13.1
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-06-28 12:46 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-27 12:50 [PATCH] Do not allow to inline ifunc resolvers (PR ipa/81128) Martin Liška
2017-06-27 14:57 ` Jan Hubicka
2017-06-27 15:15 ` Martin Liška
2017-06-27 15:26 ` Jan Hubicka
2017-06-28 12:46 ` Martin Liška
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).