Re: [RFC PATCH] Implementing ifunc target hook

[Maxim Kuvyrkov <maxim@codesourcery.com>]

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