Re: [RFC PATCH] Implementing ifunc target hook

[Alexander Ivchenko <aivchenk at gmail dot com>]

Hello Maxim,

>>
>>>> --- a/gcc/config/i386/i386.c
>>>> +++ b/gcc/config/i386/i386.c
>>>> @@ -29146,7 +29146,7 @@ make_name (tree decl, const char *suffix, bool make_unique)
>>>>   return global_var_name;
>>>> }
>>>>
>>>> -#if defined (ASM_OUTPUT_TYPE_DIRECTIVE) && HAVE_GNU_INDIRECT_FUNCTION
>>>> +#if defined (ASM_OUTPUT_TYPE_DIRECTIVE)
>>>>
>>>> /* Make a dispatcher declaration for the multi-versioned function DECL.
>>>>    Calls to DECL function will be replaced with calls to the dispatcher
>>>> @@ -29213,7 +29213,7 @@ ix86_get_function_versions_dispatcher (void *decl)
>>>>
>>>>   tree dispatch_decl = NULL;
>>>>
>>>> -#if defined (ASM_OUTPUT_TYPE_DIRECTIVE) && HAVE_GNU_INDIRECT_FUNCTION
>>>> +#if defined (ASM_OUTPUT_TYPE_DIRECTIVE)
>>>>   struct cgraph_function_version_info *it_v = NULL;
>>>>   struct cgraph_node *dispatcher_node = NULL;
>>>>   struct cgraph_function_version_info *dispatcher_version_info = NULL;
>>>
>>> It seems you can move these variables inside the 'if (TARGET_HAS_IFUNC)' clause below and make >the code cleaner, no?
>>
>> All variable declarations should be at the beginning of the routine.
>> Or is it changed right now?
>
>The variable declarations should be at the beginning of a block.  Since you are adding a new block below (under if (TARGET_HAS_IFUNC) { <block> }), and these variables are used only within that block, it would be cleaner to move them there.

Done, thank you.

>> diff --git a/gcc/config/host-linux.c b/gcc/config/host-linux.c
>> index b535758..c72db79 100644
>> --- a/gcc/config/host-linux.c
>> +++ b/gcc/config/host-linux.c
>>
....
>>
>> +/* Android does not support GNU indirect functions.  */
>> +
>> +bool
>> +linux_has_ifunc (void)
>> +{
>> +  return TARGET_ANDROID ? false : HAVE_GNU_INDIRECT_FUNCTION;
>> +}
>> +
>
>OK, so you defined new *target* hook has_ifunc[_p].  Placing one of the hook's implementation is into a host-specific file seems like a strange idea, but maybe I'm missing something.
>
>It seems you want to add a new file config/linux-android.c (with implementation for symbols used in config/linux-android.h) and add it Android-flavoured linux targets in config.gcc:
>  # Add Android userspace support to Linux targets.
.  case $target in
>    *linux*)
>      tm_file="$tm_file linux-android.h"
>      extra_options="$extra_options linux-android.opt"
>+       extra_objs="$extra_objs linux-android.o"
>      ;;
>  esac
>

I agree, that would be much better, I added gcc/config/linux-android.c
and also gcc/config/t-linux-android for the rule, thanks.

>> diff --git a/gcc/config/linux-protos.h b/gcc/config/linux-protos.h
>> new file mode 100644
>> index 0000000..ea3d77d
>> --- /dev/null
>> +++ b/gcc/config/linux-protos.h
>> @@ -0,0 +1,21 @@
>> +/* Prototypes.
>> +   Copyright (C) 2001, 2002, 2003, 2004, 2005, 2007, 2008, 2010, 2011, 2012
>> +   Free Software Foundation, Inc.
>> +
>> +This file is part of GCC.
>> +
>> +GCC is free software; you can redistribute it and/or modify
>> +it under the terms of the GNU General Public License as published by
>> +the Free Software Foundation; either version 3, or (at your option)
>> +any later version.
>> +
>> +GCC is distributed in the hope that it will be useful,
>> +but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +GNU General Public License for more details.
>> +
>> +You should have received a copy of the GNU General Public License
>> +along with GCC; see the file COPYING3.  If not see
>> +<http://www.gnu.org/licenses/>.  */
>> +
>> +extern bool linux_has_ifunc (void);
>
>Config/linux-android.h is a better place for this declaration.

That wouldn't help, I got the following error:

In file included from ../../.././gcc/tm.h:24:0,
                 from [..]/src/gcc/libgcc/generic-morestack-thread.c:29:
[..]/src/gcc/libgcc/../gcc/config/linux-android.h:62:1: error: unknown
type name ‘bool’
 extern bool linux_android_has_ifunc_p (void);
 ^

Anyway, linux-protos.h looks to me as a good thing to have (e.g. for
libc_has_function hook, that is
supposed to be commited in a near future) for declaring linux (and
Android) specific versions of hooks..

>> +/* True if target supports indirect functions.  */
>> +DEFHOOK
>> +(has_ifunc,
>
>The convention is to add "_p" for functions that behave like boolean predicates, i.e., "has_ifunc_p".

Done.

>> + "It returns true if the target supports GNU indirect functions.\n\
>> +The support includes the assembler, linker and dynamic linker.\n\
>> +The default value of this hook is defined as for the host machine.",
>
>Are your sure the last sentence is correct?  It seems the default value for this hook is based on which libc is being used.  Maybe it would be more accurate to say "The default value of this hook is based on target's libc."?

Well yes, you are right that the default value depends on version of
libc, but this version
is checked on the configure time for the host machine
(HAVE_GNU_INDIRECT_FUNCTION), so
may be the correct sentence would be "The default value of this hook
is based on host's libc."?

thanks,
Alexander



2013/1/3 Maxim Kuvyrkov <maxim.kuvyrkov@gmail.com>:
> On 29/12/2012, at 1:30 AM, Alexander Ivchenko wrote:
>
>> Joseph, Maxim, thank you for your input. I converted this macro into
>> a target hook as you said. I had to add gcc/config/linux-protos.h in order
>> to declare linux (that works for android) version of this hook - otherwise
>> I don't know where to declare it..
>
> See notes below on this.  Once you added a new target hook you can call it via targetm.<hook>(), so references to TARGET_HAS_IFUNC should be replaced with calls to targetm.has_ifunc_p().
>
> Sorry, but it will take another iteration or two to make this patch just right.  GCC config machinery is tricky, and placing things in the right places is non-trivial.
>
>>
>>>> --- a/gcc/config/i386/i386.c
>>>> +++ b/gcc/config/i386/i386.c
>>>> @@ -29146,7 +29146,7 @@ make_name (tree decl, const char *suffix, bool make_unique)
>>>>   return global_var_name;
>>>> }
>>>>
>>>> -#if defined (ASM_OUTPUT_TYPE_DIRECTIVE) && HAVE_GNU_INDIRECT_FUNCTION
>>>> +#if defined (ASM_OUTPUT_TYPE_DIRECTIVE)
>>>>
>>>> /* Make a dispatcher declaration for the multi-versioned function DECL.
>>>>    Calls to DECL function will be replaced with calls to the dispatcher
>>>> @@ -29213,7 +29213,7 @@ ix86_get_function_versions_dispatcher (void *decl)
>>>>
>>>>   tree dispatch_decl = NULL;
>>>>
>>>> -#if defined (ASM_OUTPUT_TYPE_DIRECTIVE) && HAVE_GNU_INDIRECT_FUNCTION
>>>> +#if defined (ASM_OUTPUT_TYPE_DIRECTIVE)
>>>>   struct cgraph_function_version_info *it_v = NULL;
>>>>   struct cgraph_node *dispatcher_node = NULL;
>>>>   struct cgraph_function_version_info *dispatcher_version_info = NULL;
>>>
>>> It seems you can move these variables inside the 'if (TARGET_HAS_IFUNC)' clause below and make >the code cleaner, no?
>>
>> All variable declarations should be at the beginning of the routine.
>> Or is it changed right now?
>
> The variable declarations should be at the beginning of a block.  Since you are adding a new block below (under if (TARGET_HAS_IFUNC) { <block> }), and these variables are used only within that block, it would be cleaner to move them there.
>
>>
>>>> @@ -29263,24 +29263,33 @@ ix86_get_function_versions_dispatcher (void *decl)
>>>>
>>>>   default_node = default_version_info->this_node;
>>>>
>>>> -#if defined (ASM_OUTPUT_TYPE_DIRECTIVE) && HAVE_GNU_INDIRECT_FUNCTION
>>>> -  /* Right now, the dispatching is done via ifunc.  */
>>>> -  dispatch_decl = make_dispatcher_decl (default_node->symbol.decl);
>>>> -
>>>> -  dispatcher_node = cgraph_get_create_node (dispatch_decl);
>>>> -  gcc_assert (dispatcher_node != NULL);
>>>> -  dispatcher_node->dispatcher_function = 1;
>>>> -  dispatcher_version_info
>>>> -    = insert_new_cgraph_node_version (dispatcher_node);
>>>> -  dispatcher_version_info->next = default_version_info;
>>>> -  dispatcher_node->local.finalized = 1;
>>>> -
>>>> -  /* Set the dispatcher for all the versions.  */
>>>> -  it_v = default_version_info;
>>>> -  while (it_v->next != NULL)
>>>> +#if defined (ASM_OUTPUT_TYPE_DIRECTIVE)
>>>> +  if (TARGET_HAS_IFUNC)
>>>> +    {
>>>> +      /* Right now, the dispatching is done via ifunc.  */
>>>> +      dispatch_decl = make_dispatcher_decl (default_node->symbol.decl);
>>>> +
>>>> +      dispatcher_node = cgraph_get_create_node (dispatch_decl);
>>>> +      gcc_assert (dispatcher_node != NULL);
>>>> +      dispatcher_node->dispatcher_function = 1;
>>>> +      dispatcher_version_info
>>>> +     = insert_new_cgraph_node_version (dispatcher_node);
>>>> +      dispatcher_version_info->next = default_version_info;
>>>> +      dispatcher_node->local.finalized = 1;
>>>> +
>>>> +      /* Set the dispatcher for all the versions.  */
>>>> +      it_v = default_version_info;
>>>> +      while (it_v->next != NULL)
>>>> +     {
>>>> +       it_v->dispatcher_resolver = dispatch_decl;
>>>> +       it_v = it_v->next;
>>>> +     }
>>>> +    }
>>>> +  else
>>>>     {
>>>> -      it_v->dispatcher_resolver = dispatch_decl;
>>>> -      it_v = it_v->next;
>>>> +      error_at (DECL_SOURCE_LOCATION (default_node->symbol.decl),
>>>> +             "multiversioning needs ifunc which is not supported "
>>>> +             "on this target");
>>>>     }
>>>
>>> This looks wrong.  Before the patch this code would be ignored if !HAVE_GNU_INDIRECT_FUNCTION and after the patch it will produce an error.  Removing the else-clause should fix this.
>>
>> Mmm, I believe that we want this code not to be ignored, because we
>> want it to produces an error
>> if the target does not support ifuncs and we are compiling something
>> that uses ifuncs.. May be I
>> missed something..
>
> You are right, I read the patch wrong.
>
>> index db56819..e535218 100644
>> --- a/gcc/config.gcc
>> +++ b/gcc/config.gcc
>> @@ -637,6 +637,11 @@ case ${target} in
>>        native_system_header_dir=/include
>>        ;;
>>    esac
>> +  case $target in
>> +    *linux*)
>> +      tm_p_file="${tm_p_file} linux-protos.h"
>> +      ;;
>> +  esac
>>    # glibc / uclibc / bionic switch.
>>    # uclibc and bionic aren't usable for GNU/Hurd and neither for GNU/k*BSD.
>>    case $target in
>
> Should not be necessary.  Put the hook override into config/linux-android.h:
> #undef TARGET_HAS_IFUNC_P
> #define TARGET_HAS_IFUNC_P linux_android_has_ifunc_p
>
>> diff --git a/gcc/config/host-linux.c b/gcc/config/host-linux.c
>> index b535758..c72db79 100644
>> --- a/gcc/config/host-linux.c
>> +++ b/gcc/config/host-linux.c
>>
> ...
>>
>> +/* Android does not support GNU indirect functions.  */
>> +
>> +bool
>> +linux_has_ifunc (void)
>> +{
>> +  return TARGET_ANDROID ? false : HAVE_GNU_INDIRECT_FUNCTION;
>> +}
>> +
>
> OK, so you defined new *target* hook has_ifunc[_p].  Placing one of the hook's implementation is into a host-specific file seems like a strange idea, but maybe I'm missing something.
>
> It seems you want to add a new file config/linux-android.c (with implementation for symbols used in config/linux-android.h) and add it Android-flavoured linux targets in config.gcc:
>   # Add Android userspace support to Linux targets.
>   case $target in
>     *linux*)
>       tm_file="$tm_file linux-android.h"
>       extra_options="$extra_options linux-android.opt"
> +       extra_objs="$extra_objs linux-android.o"
>       ;;
>   esac
>
>
>> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
>> index b466a4f..c422fd9 100644
>> --- a/gcc/config/i386/i386.c
>> +++ b/gcc/config/i386/i386.c
>> @@ -29146,7 +29146,7 @@ make_name (tree decl, const char *suffix, bool make_unique)
>>    return global_var_name;
>>  }
>>
>> -#if defined (ASM_OUTPUT_TYPE_DIRECTIVE) && HAVE_GNU_INDIRECT_FUNCTION
>> +#if defined (ASM_OUTPUT_TYPE_DIRECTIVE)
>>
>>  /* Make a dispatcher declaration for the multi-versioned function DECL.
>>     Calls to DECL function will be replaced with calls to the dispatcher
>> @@ -29213,7 +29213,7 @@ ix86_get_function_versions_dispatcher (void *decl)
>>
>>    tree dispatch_decl = NULL;
>>
>> -#if defined (ASM_OUTPUT_TYPE_DIRECTIVE) && HAVE_GNU_INDIRECT_FUNCTION
>> +#if defined (ASM_OUTPUT_TYPE_DIRECTIVE)
>>    struct cgraph_function_version_info *it_v = NULL;
>>    struct cgraph_node *dispatcher_node = NULL;
>>    struct cgraph_function_version_info *dispatcher_version_info = NULL;
>> @@ -29263,24 +29263,33 @@ ix86_get_function_versions_dispatcher (void *decl)
>>
>>    default_node = default_version_info->this_node;
>>
>> -#if defined (ASM_OUTPUT_TYPE_DIRECTIVE) && HAVE_GNU_INDIRECT_FUNCTION
>> -  /* Right now, the dispatching is done via ifunc.  */
>> -  dispatch_decl = make_dispatcher_decl (default_node->symbol.decl);
>> -
>> -  dispatcher_node = cgraph_get_create_node (dispatch_decl);
>> -  gcc_assert (dispatcher_node != NULL);
>> -  dispatcher_node->dispatcher_function = 1;
>> -  dispatcher_version_info
>> -    = insert_new_cgraph_node_version (dispatcher_node);
>> -  dispatcher_version_info->next = default_version_info;
>> -  dispatcher_node->local.finalized = 1;
>> -
>> -  /* Set the dispatcher for all the versions.  */
>> -  it_v = default_version_info;
>> -  while (it_v->next != NULL)
>> +#if defined (ASM_OUTPUT_TYPE_DIRECTIVE)
>> +  if (targetm.has_ifunc ())
>> +    {
>> +      /* Right now, the dispatching is done via ifunc.  */
>> +      dispatch_decl = make_dispatcher_decl (default_node->symbol.decl);
>> +
>> +      dispatcher_node = cgraph_get_create_node (dispatch_decl);
>> +      gcc_assert (dispatcher_node != NULL);
>> +      dispatcher_node->dispatcher_function = 1;
>> +      dispatcher_version_info
>> +     = insert_new_cgraph_node_version (dispatcher_node);
>> +      dispatcher_version_info->next = default_version_info;
>> +      dispatcher_node->local.finalized = 1;
>> +
>> +      /* Set the dispatcher for all the versions.  */
>> +      it_v = default_version_info;
>> +      while (it_v->next != NULL)
>> +     {
>> +       it_v->dispatcher_resolver = dispatch_decl;
>> +       it_v = it_v->next;
>> +     }
>> +    }
>> +  else
>>      {
>> -      it_v->dispatcher_resolver = dispatch_decl;
>> -      it_v = it_v->next;
>> +      error_at (DECL_SOURCE_LOCATION (default_node->symbol.decl),
>> +             "multiversioning needs ifunc which is not supported "
>> +             "on this target");
>>      }
>>  #else
>>    error_at (DECL_SOURCE_LOCATION (default_node->symbol.decl),
>
> Would you please use same trick for arranging #if-else-#endif as you did for varasm.c ?
>
>> diff --git a/gcc/config/linux-protos.h b/gcc/config/linux-protos.h
>> new file mode 100644
>> index 0000000..ea3d77d
>> --- /dev/null
>> +++ b/gcc/config/linux-protos.h
>> @@ -0,0 +1,21 @@
>> +/* Prototypes.
>> +   Copyright (C) 2001, 2002, 2003, 2004, 2005, 2007, 2008, 2010, 2011, 2012
>> +   Free Software Foundation, Inc.
>> +
>> +This file is part of GCC.
>> +
>> +GCC is free software; you can redistribute it and/or modify
>> +it under the terms of the GNU General Public License as published by
>> +the Free Software Foundation; either version 3, or (at your option)
>> +any later version.
>> +
>> +GCC is distributed in the hope that it will be useful,
>> +but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +GNU General Public License for more details.
>> +
>> +You should have received a copy of the GNU General Public License
>> +along with GCC; see the file COPYING3.  If not see
>> +<http://www.gnu.org/licenses/>.  */
>> +
>> +extern bool linux_has_ifunc (void);
>
> Config/linux-android.h is a better place for this declaration.
>
>> diff --git a/gcc/config/linux.h b/gcc/config/linux.h
>> index fb459e6..bdfbc0d 100644
>> --- a/gcc/config/linux.h
>> +++ b/gcc/config/linux.h
>> @@ -108,3 +108,6 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
>>  /* Whether we have Bionic libc runtime */
>>  #undef TARGET_HAS_BIONIC
>>  #define TARGET_HAS_BIONIC (OPTION_BIONIC)
>> +
>> +#undef TARGET_HAS_IFUNC
>> +#define TARGET_HAS_IFUNC linux_has_ifunc
>
> As noted above, please move the hook override to linux-android.h .  The override should be effective only for Linux targets that support Android, otherwise compilation may fail due to undefined TARGET_ANDROID used in linux_has_ifunc.
>
>
>> diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
>> index 75aa867..2d7121f 100644
>> --- a/gcc/doc/tm.texi
>> +++ b/gcc/doc/tm.texi
>> @@ -11343,3 +11343,9 @@ memory model bits are allowed.
>>  @deftypevr {Target Hook} {unsigned char} TARGET_ATOMIC_TEST_AND_SET_TRUEVAL
>>  This value should be set if the result written by @code{atomic_test_and_set} is not exactly 1, i.e. the @code{bool} @code{true}.
>>  @end deftypevr
>> +
>> +@deftypefn {Target Hook} bool TARGET_HAS_IFUNC (void)
>> +It returns true if the target supports GNU indirect functions.
>> +The support includes the assembler, linker and dynamic linker.
>> +The default value of this hook is defined as for the host machine.
>> +@end deftypefn
>> diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
>> index 95fab18..40dba77 100644
>> --- a/gcc/doc/tm.texi.in
>> +++ b/gcc/doc/tm.texi.in
>> @@ -11179,3 +11179,5 @@ memory model bits are allowed.
>>  @end deftypefn
>>
>>  @hook TARGET_ATOMIC_TEST_AND_SET_TRUEVAL
>> +
>> +@hook TARGET_HAS_IFUNC
>> diff --git a/gcc/target.def b/gcc/target.def
>> index c5bbfae..627ae2d 100644
>> --- a/gcc/target.def
>> +++ b/gcc/target.def
>> @@ -1520,6 +1520,15 @@ DEFHOOK
>>   bool, (const_rtx x),
>>   default_use_anchors_for_symbol_p)
>>
>> +/* True if target supports indirect functions.  */
>> +DEFHOOK
>> +(has_ifunc,
>
> The convention is to add "_p" for functions that behave like boolean predicates, i.e., "has_ifunc_p".
>
>> + "It returns true if the target supports GNU indirect functions.\n\
>> +The support includes the assembler, linker and dynamic linker.\n\
>> +The default value of this hook is defined as for the host machine.",
>
> Are your sure the last sentence is correct?  It seems the default value for this hook is based on which libc is being used.  Maybe it would be more accurate to say "The default value of this hook is based on target's libc."?
>
>> + bool, (void),
>> + default_have_ifunc)
>> +
>>  /* True if it is OK to do sibling call optimization for the specified
>>     call expression EXP.  DECL will be the called function, or NULL if
>>     this is an indirect call.  */
>> diff --git a/gcc/targhooks.c b/gcc/targhooks.c
>> index 954cdb9..ea0f9e2 100644
>> --- a/gcc/targhooks.c
>> +++ b/gcc/targhooks.c
>> @@ -451,6 +451,14 @@ default_fixed_point_supported_p (void)
>>    return ENABLE_FIXED_POINT;
>>  }
>>
>> +/* True if the target supports GNU indirect functions.  */
>> +
>> +bool
>> +default_have_ifunc (void)
>> +{
>> +  return HAVE_GNU_INDIRECT_FUNCTION;
>> +}
>> +
>>  /* NULL if INSN insn is valid within a low-overhead loop, otherwise returns
>>     an error message.
>>
>> diff --git a/gcc/targhooks.h b/gcc/targhooks.h
>> index bd7d4bc..8622d58 100644
>> --- a/gcc/targhooks.h
>> +++ b/gcc/targhooks.h
>> @@ -73,6 +73,8 @@ extern bool targhook_float_words_big_endian (void);
>>  extern bool default_decimal_float_supported_p (void);
>>  extern bool default_fixed_point_supported_p (void);
>>
>> +extern bool default_have_ifunc (void);
>> +
>>  extern const char * default_invalid_within_doloop (const_rtx);
>>
>>  extern tree default_builtin_vectorized_function (tree, tree, tree);
>> diff --git a/gcc/varasm.c b/gcc/varasm.c
>> index 7d083fd..dd677a4 100644
>> --- a/gcc/varasm.c
>> +++ b/gcc/varasm.c
>> @@ -5489,14 +5489,15 @@ do_assemble_alias (tree decl, tree target)
>>      }
>>    if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (decl)))
>>      {
>> -#if defined (ASM_OUTPUT_TYPE_DIRECTIVE) && HAVE_GNU_INDIRECT_FUNCTION
>> -      ASM_OUTPUT_TYPE_DIRECTIVE
>> -     (asm_out_file, IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl)),
>> -      IFUNC_ASM_TYPE);
>> -#else
>> -      error_at (DECL_SOURCE_LOCATION (decl),
>> -             "ifunc is not supported in this configuration");
>> +#if defined (ASM_OUTPUT_TYPE_DIRECTIVE)
>> +      if (targetm.has_ifunc ())
>> +     ASM_OUTPUT_TYPE_DIRECTIVE
>> +       (asm_out_file, IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl)),
>> +        IFUNC_ASM_TYPE);
>> +      else
>>  #endif
>> +     error_at (DECL_SOURCE_LOCATION (decl),
>> +               "ifunc is not supported on this target");
>>      }
>
> I like how you arranged #if-else-#endif to have a single error path.
>
> Thank you,
>
> --
> Maxim Kuvyrkov
>
>
>
>

Attachment: disable_ifunc_for_android_05.patch
Description: Binary data