Hi Adhemerval,

thanks for reviewing. I've split the too long lines and added a comment 
block for libc_ifunc{_redirected,_hidden} macros in include/libc-symbols.h.

Yes you are right, review for patch 9 is still missing. I've tested the 
s390 part on s390x/s390. If patch 9 is okay, I'll commit the patch series.

Thanks.
Stefan

On 09/29/2016 08:38 PM, Adhemerval Zanella wrote:
> Patch LGTM and a powerpc32/power7 (to actually uses ifunc) and a build/run
> for powerpc64le seems ok.
>
> I see with recent Andreas ack of 4/9 that only 3/9 (s390 bits) and
> 9/9 (siglongjmp, longjmp in libpthread) seems to be the impending
> bits.  I plan to check on 9/9, but I won't be able to proper review
> 3/9 since I do not have access to a s390 machine anymore.
>
> Some comments below:
>
> On 24/08/2016 07:04, Stefan Liebler wrote:
>> diff --git a/include/libc-symbols.h b/include/libc-symbols.h
>> index c2b499a..44e5253 100644
>> --- a/include/libc-symbols.h
>> +++ b/include/libc-symbols.h
>> @@ -722,27 +722,64 @@ for linking")
>>  # define compat_data_section .section ".data.compat", "aw";
>>  #endif
>>
>> -/* Marker used for indirection function symbols.  */
>> -#define libc_ifunc(name, expr)						\
>> -  extern void *name##_ifunc (void) __asm__ (#name);			\
>> -  void *name##_ifunc (void)						\
>> +/* Helper / base  macros for indirect function symbols.  */
>> +#define __ifunc_resolver(type_name, name, expr, arg, init, classifier)	\
>> +  classifier void *name##_ifunc (arg)					\
>>    {									\
>> -    INIT_ARCH ();							\
>> -    __typeof (name) *res = expr;					\
>> +    init ();								\
>> +    __typeof (type_name) *res = expr;					\
>>      return res;								\
>> -  }									\
>> -  __asm__ (".type " #name ", %gnu_indirect_function");
>> +  }
>> +
>> +#ifdef HAVE_GCC_IFUNC
>> +# define __ifunc(type_name, name, expr, arg, init)			\
>> +  extern __typeof (type_name) name __attribute__			\
>> +			      ((ifunc (#name "_ifunc")));		\
>> +  __ifunc_resolver (type_name, name, expr, arg, init, static)
>> +
>> +# define __ifunc_hidden(type_name, name, expr, arg, init)	\
>> +  __ifunc (type_name, name, expr, arg, init)
>> +#else
>> +/* Gcc does not support __attribute__ ((ifunc (...))).  Use the old behaviour
>> +   as fallback.  But keep in mind that the debug information for the ifunc
>> +   resolver functions is not correct.  It contains the ifunc'ed function as
>> +   DW_AT_linkage_name.  E.g. lldb uses this field and an inferior function
>> +   call of the ifunc'ed function will fail due to "no matching function for call
>
> Line too long.
>
>> +   to ..." because the ifunc'ed function and the resolver function have
>> +   different signatures.  (Gcc support is disabled at least on a ppc64le
>> +   Ubuntu 14.04 system.)  */
>> +
>> +# define __ifunc(type_name, name, expr, arg, init)			\
>> +  extern __typeof (type_name) name;					\
>> +  void *name##_ifunc (arg) __asm__ (#name);				\
>> +  __ifunc_resolver (type_name, name, expr, arg, init,)			\
>> + __asm__ (".type " #name ", %gnu_indirect_function");
>> +
>> +# define __ifunc_hidden(type_name, name, expr, arg, init)		\
>> +  extern __typeof (type_name) __libc_##name;				\
>> +  __ifunc (type_name, __libc_##name, expr, arg, init)			\
>> +  strong_alias (__libc_##name, name);
>> +#endif /* !HAVE_GCC_IFUNC  */
>> +
>> +/* Use libc_ifunc if your ifunc'ed function has no internal symbol.  */
>> +#define libc_ifunc(name, expr) __ifunc (name, name, expr, void, INIT_ARCH)
>
> I think it will be valuable to add a comment like the one on same
> file at line 341 with a usage example on how to use
> libc_ifunc{_redirected,_hidden} and difference between each usage and
> the possible requirements (such as name redirection).
>
>> +
>> +/* Use libc_ifunc_redirected if your ifunc'ed function has an internal symbol
>> +   which should be a dedicated fallback function instead of ifunc'ed.
>> +   You have to redirect the function in the header file and use it as
>> +   redirected_name.  */
>> +#define libc_ifunc_redirected(redirected_name, name, expr)	\
>> +  __ifunc (redirected_name, name, expr, void, INIT_ARCH)
>> +
>> +/* Use libc_ifunc_hidden if your ifunc'ed function has an internal symbol
>> +   which should be the ifunc'ed function'.  */
>> +#define libc_ifunc_hidden(redirected_name, name, expr)			\
>> +  __ifunc_hidden (redirected_name, name, expr, void, INIT_ARCH)
>>
>>  /* The body of the function is supposed to use __get_cpu_features
>>     which will, if necessary, initialize the data first.  */
>> -#define libm_ifunc(name, expr)						\
>> -  extern void *name##_ifunc (void) __asm__ (#name);			\
>> -  void *name##_ifunc (void)						\
>> -  {									\
>> -    __typeof (name) *res = expr;					\
>> -    return res;								\
>> -  }									\
>> -  __asm__ (".type " #name ", %gnu_indirect_function");
>> +#define libm_ifunc_init()
>> +#define libm_ifunc(name, expr) __ifunc (name, name, expr, void, libm_ifunc_init)
>
> Line too long.
>