[PATCH] have ifunc resolver's return type match target

[PATCH] have ifunc resolver's return type match target

[Martin Sebor]

The following GCC patch has been submitted for review.  It
helps detect mismatches between the type of an ifunc or alias
declaration and the type of the resolver or alias.

   https://gcc.gnu.org/ml/gcc-patches/2017-08/msg01103.html

To let Glibc take advantage of this type checking and avoid
warnings when using the patched GCC when the change above is
committed, the patch below adjusts the Glibc __ifunc_resolver
macro to declare the ifunc resolver so that its return type
matches that of the target.  (I was going to wait to submit it
until after the GCC patch has been accepted but per Joseph's
suggestion I'm posting it here ahead of time.)

The patch has been tested on its own with the system GCC 6.3
and with the patched GCC on x86_64-linux with no regressions.

Martin

diff --git a/include/libc-symbols.h b/include/libc-symbols.h
index fe3ab81..5413e56 100644
--- a/include/libc-symbols.h
+++ b/include/libc-symbols.h
@@ -790,7 +790,8 @@ for linking")

  /* Helper / base  macros for indirect function symbols.  */
  #define __ifunc_resolver(type_name, name, expr, arg, init, classifier) \
-  classifier inhibit_stack_protector void *name##_ifunc (arg) 
              \
+  classifier inhibit_stack_protector                                   \
+  __typeof (type_name) *name##_ifunc (arg)                             \
    {                                                                    \
      init ();                                                           \
      __typeof (type_name) *res = expr;                                  \

Re: [PATCH] have ifunc resolver's return type match target

[Joseph Myers]

On Sun, 20 Aug 2017, Martin Sebor wrote:

> To let Glibc take advantage of this type checking and avoid
> warnings when using the patched GCC when the change above is
> committed, the patch below adjusts the Glibc __ifunc_resolver
> macro to declare the ifunc resolver so that its return type
> matches that of the target.  (I was going to wait to submit it
> until after the GCC patch has been accepted but per Joseph's
> suggestion I'm posting it here ahead of time.)
> 
> The patch has been tested on its own with the system GCC 6.3
> and with the patched GCC on x86_64-linux with no regressions.

OK (with a ChangeLog entry, of course).

-- 
Joseph S. Myers
joseph@codesourcery.com

[PATCH] Fix remaining return type of ifunc resolver declaration

[Gabriel F. T. Gomes]

Since Martin Sebor's commit

commit ee4e992ebe5f9712faedeefe8958b67d61eaa0f2
Author: Martin Sebor <msebor@redhat.com>
Date:   Tue Aug 22 09:35:23 2017 -0600

    Declare ifunc resolver to return a pointer to the same type as the target
    function to help GCC detect incompatibilities between the two when it's
    enhanced to do so.

builds for powerpc64le fail in the declaration of some ifunc resolvers,
because the ifunc is declared with unmatching return types.  One of the
declarations comes from the __ifunc_resolver macro, which was patched by
the aforementioned commit:

    /* Helper / base  macros for indirect function symbols.  */
    #define __ifunc_resolver(type_name, name, expr, arg, init, classifier) \
      classifier inhibit_stack_protector                                   \
      __typeof (type_name) *name##_ifunc (arg)                             \

whereas the other comes from the unpatched __ifunc macro when
HAVE_GCC_IFUNC is not defined:

    # define __ifunc(type_name, name, expr, arg, init)                     \
      extern __typeof (type_name) name;                                    \
      void *name##_ifunc (arg) __asm__ (#name);                            \

This patch changes the return type of the ifunc resolver in the __ifunc
macro, so that it matches the return type of the target function,
similarly to what the aforementioned commit does.

Tested for powerpc64le and s390x with unpatched GCC.
---
 include/libc-symbols.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/libc-symbols.h b/include/libc-symbols.h
index 42fc41a1a5..65738caaa7 100644
--- a/include/libc-symbols.h
+++ b/include/libc-symbols.h
@@ -831,7 +831,7 @@ for linking")
 
 # define __ifunc(type_name, name, expr, arg, init)			\
   extern __typeof (type_name) name;					\
-  void *name##_ifunc (arg) __asm__ (#name);				\
+  extern __typeof (type_name) *name##_ifunc (arg) __asm__ (#name);	\
   __ifunc_resolver (type_name, name, expr, arg, init,)			\
  __asm__ (".type " #name ", %gnu_indirect_function");
 
-- 
2.13.5

Re: [PATCH] Fix remaining return type of ifunc resolver declaration

[Martin Sebor]

On 08/22/2017 12:02 PM, Gabriel F. T. Gomes wrote:
> Since Martin Sebor's commit
>
> commit ee4e992ebe5f9712faedeefe8958b67d61eaa0f2
> Author: Martin Sebor <msebor@redhat.com>
> Date:   Tue Aug 22 09:35:23 2017 -0600
>
>     Declare ifunc resolver to return a pointer to the same type as the target
>     function to help GCC detect incompatibilities between the two when it's
>     enhanced to do so.
>
> builds for powerpc64le fail in the declaration of some ifunc resolvers,
> because the ifunc is declared with unmatching return types.  One of the
> declarations comes from the __ifunc_resolver macro, which was patched by
> the aforementioned commit:
>
>     /* Helper / base  macros for indirect function symbols.  */
>     #define __ifunc_resolver(type_name, name, expr, arg, init, classifier) \
>       classifier inhibit_stack_protector                                   \
>       __typeof (type_name) *name##_ifunc (arg)                             \
>
> whereas the other comes from the unpatched __ifunc macro when
> HAVE_GCC_IFUNC is not defined:
>
>     # define __ifunc(type_name, name, expr, arg, init)                     \
>       extern __typeof (type_name) name;                                    \
>       void *name##_ifunc (arg) __asm__ (#name);                            \
>
> This patch changes the return type of the ifunc resolver in the __ifunc
> macro, so that it matches the return type of the target function,
> similarly to what the aforementioned commit does.

Sorry about that and thanks for patching it up.  I didn't think
to check all the other __ifunc macros in the file for their uses
of __ifunc_resolver.

Martin

>
> Tested for powerpc64le and s390x with unpatched GCC.
> ---
>  include/libc-symbols.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/libc-symbols.h b/include/libc-symbols.h
> index 42fc41a1a5..65738caaa7 100644
> --- a/include/libc-symbols.h
> +++ b/include/libc-symbols.h
> @@ -831,7 +831,7 @@ for linking")
>
>  # define __ifunc(type_name, name, expr, arg, init)			\
>    extern __typeof (type_name) name;					\
> -  void *name##_ifunc (arg) __asm__ (#name);				\
> +  extern __typeof (type_name) *name##_ifunc (arg) __asm__ (#name);	\
>    __ifunc_resolver (type_name, name, expr, arg, init,)			\
>   __asm__ (".type " #name ", %gnu_indirect_function");
>
>

Re: [PATCH] Fix remaining return type of ifunc resolver declaration

[Joseph Myers]

On Tue, 22 Aug 2017, Gabriel F. T. Gomes wrote:

> This patch changes the return type of the ifunc resolver in the __ifunc
> macro, so that it matches the return type of the target function,
> similarly to what the aforementioned commit does.

Thanks, please commit (with ChangeLog entry).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Fix remaining return type of ifunc resolver declaration

[Gabriel F. T. Gomes]

On Tue, 22 Aug 2017 20:13:45 +0000
Joseph Myers <joseph@codesourcery.com> wrote:

>On Tue, 22 Aug 2017, Gabriel F. T. Gomes wrote:
>
>> This patch changes the return type of the ifunc resolver in the __ifunc
>> macro, so that it matches the return type of the target function,
>> similarly to what the aforementioned commit does.  
>
>Thanks, please commit (with ChangeLog entry).

Thank you, Martin and Joseph.  I've committed this patch with the
ChangeLog entry.

Re: [PATCH] have ifunc resolver's return type match target

[Florian Weimer]

On 08/21/2017 12:30 AM, Martin Sebor wrote:
> The following GCC patch has been submitted for review.  It
> helps detect mismatches between the type of an ifunc or alias
> declaration and the type of the resolver or alias.
> 
>   https://gcc.gnu.org/ml/gcc-patches/2017-08/msg01103.html
> 
> To let Glibc take advantage of this type checking and avoid
> warnings when using the patched GCC when the change above is
> committed, the patch below adjusts the Glibc __ifunc_resolver
> macro to declare the ifunc resolver so that its return type
> matches that of the target.  (I was going to wait to submit it
> until after the GCC patch has been accepted but per Joseph's
> suggestion I'm posting it here ahead of time.)

Do we have to backport both patches to older releases, too, so that they
keep building with a newer GCC?

Thanks,
Florian

Re: [PATCH] have ifunc resolver's return type match target

[Martin Sebor]

On 08/23/2017 03:28 AM, Florian Weimer wrote:
> On 08/21/2017 12:30 AM, Martin Sebor wrote:
>> The following GCC patch has been submitted for review.  It
>> helps detect mismatches between the type of an ifunc or alias
>> declaration and the type of the resolver or alias.
>>
>>   https://gcc.gnu.org/ml/gcc-patches/2017-08/msg01103.html
>>
>> To let Glibc take advantage of this type checking and avoid
>> warnings when using the patched GCC when the change above is
>> committed, the patch below adjusts the Glibc __ifunc_resolver
>> macro to declare the ifunc resolver so that its return type
>> matches that of the target.  (I was going to wait to submit it
>> until after the GCC patch has been accepted but per Joseph's
>> suggestion I'm posting it here ahead of time.)
>
> Do we have to backport both patches to older releases, too, so that they
> keep building with a newer GCC?

It would make sense to me if that's how Glibc usually deals with
these sorts of things (i.e., changing code to avoid new warnings).
The other (obvious) alternative is for people to suppress the
warnings when using the new compiler.

Martin

Re: [PATCH] have ifunc resolver's return type match target

[Florian Weimer]

On 08/23/2017 04:48 PM, Martin Sebor wrote:
> On 08/23/2017 03:28 AM, Florian Weimer wrote:
>> On 08/21/2017 12:30 AM, Martin Sebor wrote:
>>> The following GCC patch has been submitted for review.  It
>>> helps detect mismatches between the type of an ifunc or alias
>>> declaration and the type of the resolver or alias.
>>>
>>>   https://gcc.gnu.org/ml/gcc-patches/2017-08/msg01103.html
>>>
>>> To let Glibc take advantage of this type checking and avoid
>>> warnings when using the patched GCC when the change above is
>>> committed, the patch below adjusts the Glibc __ifunc_resolver
>>> macro to declare the ifunc resolver so that its return type
>>> matches that of the target.  (I was going to wait to submit it
>>> until after the GCC patch has been accepted but per Joseph's
>>> suggestion I'm posting it here ahead of time.)
>>
>> Do we have to backport both patches to older releases, too, so that they
>> keep building with a newer GCC?
> 
> It would make sense to me if that's how Glibc usually deals with
> these sorts of things (i.e., changing code to avoid new warnings).
> The other (obvious) alternative is for people to suppress the
> warnings when using the new compiler.

Okay, I backported a few toolchain enablement patches to the 2.26 branch.

Thanks,
Florian