On 27/12/2012, at 1:15 AM, Alexander Ivchenko wrote:
> Hi,
>
> Currently Android dynamic loader does not support indirect functions
> (And I don't think that
> it will someday). But there is no way for us to specify that for gcc,
> and for example, tests like
> gcc.dg/attr-ifunc-* are failing on android right now.
> The attached patch is indended to add the target hook for indicating
> the support of ifunc on target.
The idea behind the patch looks OK, but implementation needs a bit of tweaking.
As Joseph mentioned, you need to convert this macro into a target hook. GCC is
making a gradual transition away from target macros to target hook functions,
and all new hooks should be added as functions, unless there is a compelling
argument to make it a macro. For an example of adding a target hook function
see rev 194608 (among many others).
> --- 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?
> @@ -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.
> #else
> error_at (DECL_SOURCE_LOCATION (default_node->symbol.decl),
> diff --git a/gcc/config/linux-android.h b/gcc/config/linux-android.h
> index e74e261..f6f44f1 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
> +#define TARGET_HAS_IFUNC (!TARGET_ANDROID)
This initialization should be moved to targethooks.c ...
> diff --git a/gcc/defaults.h b/gcc/defaults.h
> index 76909ab..2180a47 100644
> --- a/gcc/defaults.h
> +++ b/gcc/defaults.h
> @@ -111,6 +111,10 @@ see the files COPYING3 and COPYING.RUNTIME respectively.
> If not, see
> #endif
> #endif
>
> +#ifndef TARGET_HAS_IFUNC
> +#define TARGET_HAS_IFUNC HAVE_GNU_INDIRECT_FUNCTION
> +#endif
... and this one too.
> +
> #ifndef IFUNC_ASM_TYPE
> #define IFUNC_ASM_TYPE "gnu_indirect_function"
> #endif
> diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
> index 75aa867..1c8bc51 100644
> --- a/gcc/doc/tm.texi
> +++ b/gcc/doc/tm.texi
> @@ -5847,6 +5847,12 @@ If @code{ASM_OUTPUT_DEF} is not available, the hook's
> default definition
> is @code{NULL}, which disables the use of section anchors altogether.
> @end deftypefn
>
> +@defmac TARGET_HAS_IFUNC
> +When this macro is nonzero, GCC will assume that 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 defmac
> +
> @deftypefn {Target Hook} bool TARGET_USE_ANCHORS_FOR_SYMBOL_P (const_rtx
> @var{x})
> Return true if GCC should attempt to use anchors to access @code{SYMBOL_REF}
> @var{x}. You can assume @samp{SYMBOL_REF_HAS_BLOCK_INFO_P (@var{x})} and
> diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
> index 95fab18..e1646b8 100644
> --- a/gcc/doc/tm.texi.in
> +++ b/gcc/doc/tm.texi.in
> @@ -5751,6 +5751,12 @@ If @code{ASM_OUTPUT_DEF} is not available, the hook's
> default definition
> is @code{NULL}, which disables the use of section anchors altogether.
> @end deftypefn
>
> +@defmac TARGET_HAS_IFUNC
> +When this macro is nonzero, GCC will assume that 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 defmac
> +
> @hook TARGET_USE_ANCHORS_FOR_SYMBOL_P
> Return true if GCC should attempt to use anchors to access @code{SYMBOL_REF}
> @var{x}. You can assume @samp{SYMBOL_REF_HAS_BLOCK_INFO_P (@var{x})} and
> diff --git a/gcc/varasm.c b/gcc/varasm.c
> index 7d083fd..14be8fb 100644
> --- a/gcc/varasm.c
> +++ b/gcc/varasm.c
> @@ -5489,10 +5489,14 @@ 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);
> +#if defined (ASM_OUTPUT_TYPE_DIRECTIVE)
> + if (TARGET_HAS_IFUNC)
> + 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 on this target");
> #else
> error_at (DECL_SOURCE_LOCATION (decl),
> "ifunc is not supported in this configuration");
Please use same error message in both error cases. It seems you can just
convert '#else' preprocessor clause into 'else' code clause without touching
the error_at() statement beyond indentation updates.
Thanks,
--
Maxim Kuvyrkov
CodeSourcery / Mentor Graphics
