public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Alexander Ivchenko <aivchenk@gmail.com>
To: Maxim Kuvyrkov <maxim.kuvyrkov@gmail.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>,
	"Joseph S. Myers" <joseph@codesourcery.com>,
		"H.J. Lu" <hjl.tools@gmail.com>
Subject: Re: [RFC PATCH] Implementing ifunc target hook
Date: Wed, 09 Jan 2013 11:24:00 -0000	[thread overview]
Message-ID: <CACysShi_LqUWwszH+FGJ8do_c94atE_1-tZG_3gEZ-YRQYHX9A@mail.gmail.com> (raw)
In-Reply-To: <D56AE797-1CD6-4C96-8076-CE0B13D15C77@gmail.com>

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

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 #2: disable_ifunc_for_android_05.patch --]
[-- Type: application/octet-stream, Size: 12771 bytes --]

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index d625bb1..abce372 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,21 @@
+2012-12-28  Alexander Ivchenko  <alexander.ivchenko@intel.com>
+
+	* gcc/target.def (TARGET_HAS_IFUNC_P): New target hook.
+	* gcc/doc/tm.texi.in (TARGET_HAS_IFUNC_P): New.
+	* gcc/doc/tm.texi: Regenerate.
+	* gcc/targhooks.h (default_have_ifunc_p): New.
+	* gcc/targhooks.c (default_have_ifunc_p): Ditto.
+	* gcc/config/linux-protos.h: New file.
+	* gcc/config/linux-androids.h (TARGET_HAS_IFUNC_P): Using version of
+	this hook for linux which disables support of indirect functions in
+	android.
+	* gcc/config/linux-android.c: New file.
+	* gcc/config/t-linux-android.c: Ditto.
+	* gcc/config.gcc: Added new object file linux-android.o.
+	* gcc/config/i386/i386.c (ix86_get_function_versions_dispatcher):
+	Using TARGET_HAS_IFUNC hook instead of HAVE_GNU_INDIRECT_FUNCTION.
+	* gcc/varasm.c (do_assemble_alias): Likewise.
+
 2012-12-22  Jan Hubicka  <jh@suse.cz>
 
 	PR lto/54728
diff --git a/gcc/config.gcc b/gcc/config.gcc
index db56819..bbf65c2 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
@@ -662,8 +667,10 @@ case ${target} in
   # Add Android userspace support to Linux targets.
   case $target in
     *linux*)
+      tmake_file="${tmake_file} t-linux-android"
       tm_file="$tm_file linux-android.h"
       extra_options="$extra_options linux-android.opt"
+      extra_objs="$extra_objs linux-android.o"
       ;;
   esac
   # Enable compilation for Android by default for *android* targets.
diff --git a/gcc/config/host-linux.c b/gcc/config/host-linux.c
index b535758..ae5d211 100644
--- a/gcc/config/host-linux.c
+++ b/gcc/config/host-linux.c
@@ -23,7 +23,6 @@
 #include "hosthooks.h"
 #include "hosthooks-def.h"
 
-
 /* Linux has a feature called exec-shield-randomize that perturbs the
    address of non-fixed mapped segments by a (relatively) small amount.
    The feature is intended to make it harder to attack the system with
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index b466a4f..98e2998 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,12 +29213,6 @@ ix86_get_function_versions_dispatcher (void *decl)
 
   tree dispatch_decl = NULL;
 
-#if defined (ASM_OUTPUT_TYPE_DIRECTIVE) && HAVE_GNU_INDIRECT_FUNCTION
-  struct cgraph_function_version_info *it_v = NULL;
-  struct cgraph_node *dispatcher_node = NULL;
-  struct cgraph_function_version_info *dispatcher_version_info = NULL;
-#endif
-
   struct cgraph_function_version_info *default_version_info = NULL;
  
   gcc_assert (fn != NULL && DECL_FUNCTION_VERSIONED (fn));
@@ -29263,30 +29257,40 @@ 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_p ())
     {
-      it_v->dispatcher_resolver = dispatch_decl;
-      it_v = it_v->next;
+      struct cgraph_function_version_info *it_v = NULL;
+      struct cgraph_node *dispatcher_node = NULL;
+      struct cgraph_function_version_info *dispatcher_version_info = NULL;
+
+      /* 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
-  error_at (DECL_SOURCE_LOCATION (default_node->symbol.decl),
-	    "multiversioning needs ifunc which is not supported "
-	    "in this configuration");
+  else
 #endif
+    {
+      error_at (DECL_SOURCE_LOCATION (default_node->symbol.decl),
+		"multiversioning needs ifunc which is not supported "
+		"on this target");
+    }
+
   return dispatch_decl;
 }
 
diff --git a/gcc/config/linux-android.c b/gcc/config/linux-android.c
new file mode 100644
index 0000000..47cc50a
--- /dev/null
+++ b/gcc/config/linux-android.c
@@ -0,0 +1,33 @@
+/* Functions for Linux Android as target machine for GNU C compiler.
+   Copyright (C) 2001, 2002, 2003, 2004, 2005, 2007, 2008, 2010, 2011,
+   2012, 2013.
+   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/>.  */
+
+#include "config.h"
+#include "system.h"
+#include "coretypes.h"
+#include "tm.h"
+
+/* Android does not support GNU indirect functions.  */
+
+bool
+linux_android_has_ifunc_p (void)
+{
+  return TARGET_ANDROID ? false : HAVE_GNU_INDIRECT_FUNCTION;
+}
diff --git a/gcc/config/linux-android.h b/gcc/config/linux-android.h
index e74e261..d6a2bb8 100644
--- a/gcc/config/linux-android.h
+++ b/gcc/config/linux-android.h
@@ -58,3 +58,6 @@
 
 #define ANDROID_ENDFILE_SPEC \
   "%{shared: crtend_so%O%s;: crtend_android%O%s}"
+
+#undef TARGET_HAS_IFUNC_P
+#define TARGET_HAS_IFUNC_P linux_android_has_ifunc_p
diff --git a/gcc/config/linux-protos.h b/gcc/config/linux-protos.h
new file mode 100644
index 0000000..aae1d28
--- /dev/null
+++ b/gcc/config/linux-protos.h
@@ -0,0 +1,22 @@
+/* Prototypes.
+   Copyright (C) 2001, 2002, 2003, 2004, 2005, 2007, 2008, 2010, 2011,
+   2012, 2013
+   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_android_has_ifunc_p (void);
diff --git a/gcc/config/t-linux-android b/gcc/config/t-linux-android
new file mode 100644
index 0000000..6f9b033
--- /dev/null
+++ b/gcc/config/t-linux-android
@@ -0,0 +1,23 @@
+# Copyright (C) 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2013
+# 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/>.
+
+linux-android.o: $(srcdir)/config/linux-android.c $(CONFIG_H) $(SYSTEM_H) \
+  coretypes.h $(TM_H) $(TM_P_H)
+	$(COMPILER) -c $(ALL_COMPILERFLAGS) $(ALL_CPPFLAGS) $(INCLUDES) \
+		$(srcdir)/config/linux-android.c
diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index 75aa867..48cc507 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_P (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 based on host's libc.
+@end deftypefn
diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
index 95fab18..432cdcf 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_P
diff --git a/gcc/target.def b/gcc/target.def
index c5bbfae..f8df671 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_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 based on host's libc.",
+ bool, (void),
+ default_have_ifunc_p)
+
 /* 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..2a8caec 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_p (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..6cb99ba 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_p (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..d9b7bb7 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_p ())
+	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");
     }
 
 # ifdef ASM_OUTPUT_DEF_FROM_DECLS

  reply	other threads:[~2013-01-09 11:24 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-12-26 12:15 Alexander Ivchenko
2012-12-26 17:06 ` Joseph S. Myers
2012-12-27 22:59 ` Maxim Kuvyrkov
2012-12-28 12:30   ` Alexander Ivchenko
2013-01-02 23:08     ` Maxim Kuvyrkov
2013-01-09 11:24       ` Alexander Ivchenko [this message]
2013-01-10 23:24         ` Maxim Kuvyrkov
2013-01-14 15:55           ` Alexander Ivchenko
2013-01-15  4:13             ` Maxim Kuvyrkov
2013-03-26 15:15               ` Alexander Ivchenko
2013-03-26 20:59                 ` Maxim Kuvyrkov
2013-03-27  9:56                   ` Kirill Yukhin
2013-04-02 13:49                     ` Jakub Jelinek
2013-04-02 14:54                       ` Alexander Ivchenko
2013-04-02 15:04                         ` Jakub Jelinek
2013-04-02 15:06                           ` Kirill Yukhin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CACysShi_LqUWwszH+FGJ8do_c94atE_1-tZG_3gEZ-YRQYHX9A@mail.gmail.com \
    --to=aivchenk@gmail.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=hjl.tools@gmail.com \
    --cc=joseph@codesourcery.com \
    --cc=maxim.kuvyrkov@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).