public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [Android] Enable ifuncs on Android
@ 2014-11-12 10:03 Alexander Ivchenko
  2014-11-13 17:42 ` Jeff Law
  0 siblings, 1 reply; 15+ messages in thread
From: Alexander Ivchenko @ 2014-11-12 10:03 UTC (permalink / raw)
  To: GCC Patches; +Cc: enh, Andrew Hsieh

Hi,

Bionic - Android libc - supports indirect functions right now, but
they are disabled in gcc;

We cannot do the configure-time check for that, because there is only
one version of each compiler shipped in ndk for all Android platforms.
On the other hand, having different runtime targets like -mandroid-19,
-mandroid-20 would be a nightmare. But, keeping in mind that the last
version of Android is a priority, I think that enabling ifuncs
unconditionally would be the right thing.

Is the patch ok? Bootstrapped/regtested on x86_64-unknown-linux-gnu +
checked that the behavior of i686-linux-android is correct.


diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index eac19cf..9932323 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,8 @@
+2014-11-11  Alexander Ivchenko  <alexander.ivchenko@intel.com>
+
+       * config/linux.c (linux_has_ifunc_p): Remove.
+       * config/linux.h (TARGET_HAS_IFUNC_P): Use the default version.
+
 2014-11-11  Uros Bizjak  <ubizjak@gmail.com>

        * sreal.c (sreal::to_int): Use INTTYPE_MAXIMUM (int64_t)
diff --git a/gcc/config/linux.c b/gcc/config/linux.c
index 6242e11..15df213 100644
--- a/gcc/config/linux.c
+++ b/gcc/config/linux.c
@@ -23,14 +23,6 @@ along with GCC; see the file COPYING3.  If not see
 #include "tm.h"
 #include "linux-protos.h"

-/* Android does not support GNU indirect functions.  */
-
-bool
-linux_has_ifunc_p (void)
-{
-  return OPTION_BIONIC ? false : HAVE_GNU_INDIRECT_FUNCTION;
-}
-
 bool
 linux_libc_has_function (enum function_class fn_class)
 {
diff --git a/gcc/config/linux.h b/gcc/config/linux.h
index d38ef81..6ccacff 100644
--- a/gcc/config/linux.h
+++ b/gcc/config/linux.h
@@ -117,10 +117,6 @@ see the files COPYING3 and COPYING.RUNTIME
respectively.  If not, see

 #else /* !uClinux, i.e., normal Linux */

-/* IFUNCs are supported by glibc, but not by uClibc or Bionic.  */
-# undef TARGET_HAS_IFUNC_P
-# define TARGET_HAS_IFUNC_P linux_has_ifunc_p
-
 /* Determine what functions are present at the runtime;
    this includes full c99 runtime and sincos.  */
 # undef TARGET_LIBC_HAS_FUNCTION


--Alexander

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Android] Enable ifuncs on Android
  2014-11-12 10:03 [Android] Enable ifuncs on Android Alexander Ivchenko
@ 2014-11-13 17:42 ` Jeff Law
  2014-11-13 18:05   ` enh
                     ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Jeff Law @ 2014-11-13 17:42 UTC (permalink / raw)
  To: Alexander Ivchenko, GCC Patches; +Cc: enh, Andrew Hsieh

On 11/12/14 03:02, Alexander Ivchenko wrote:
> Hi,
>
> Bionic - Android libc - supports indirect functions right now, but
> they are disabled in gcc;
>
> We cannot do the configure-time check for that, because there is only
> one version of each compiler shipped in ndk for all Android platforms.
> On the other hand, having different runtime targets like -mandroid-19,
> -mandroid-20 would be a nightmare. But, keeping in mind that the last
> version of Android is a priority, I think that enabling ifuncs
> unconditionally would be the right thing.
>
> Is the patch ok? Bootstrapped/regtested on x86_64-unknown-linux-gnu +
> checked that the behavior of i686-linux-android is correct.
>
>
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index eac19cf..9932323 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,8 @@
> +2014-11-11  Alexander Ivchenko  <alexander.ivchenko@intel.com>
> +
> +       * config/linux.c (linux_has_ifunc_p): Remove.
> +       * config/linux.h (TARGET_HAS_IFUNC_P): Use the default version.
This feels like a bad idea to me simply because a new compiler with an 
old runtime will generate code that fails, right?

If you can't do a configure-time test, then the way to go is either a 
compile-time option, or to use a different target.  If there's some 
minimum version of android that has this capability, then this isn't 
terribly hard.   You may not even need a config file for this since you 
could define LIBC_BIONIC_USE_IFUNCS or something like that when 
configured for a suitably new android version.

jeff

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Android] Enable ifuncs on Android
  2014-11-13 17:42 ` Jeff Law
@ 2014-11-13 18:05   ` enh
       [not found]   ` <CAJgzZopFPL=5y8ts36Jd_+tH1UOz+2SG7BLZkRj5JnihuACOUw@mail.gmail.com>
  2014-11-14  4:59   ` H.J. Lu
  2 siblings, 0 replies; 15+ messages in thread
From: enh @ 2014-11-13 18:05 UTC (permalink / raw)
  To: Jeff Law; +Cc: Alexander Ivchenko, GCC Patches, Andrew Hsieh

On Thu, Nov 13, 2014 at 9:36 AM, Jeff Law <law@redhat.com> wrote:
> On 11/12/14 03:02, Alexander Ivchenko wrote:
>>
>> Hi,
>>
>> Bionic - Android libc - supports indirect functions right now, but
>> they are disabled in gcc;
>>
>> We cannot do the configure-time check for that, because there is only
>> one version of each compiler shipped in ndk for all Android platforms.
>> On the other hand, having different runtime targets like -mandroid-19,
>> -mandroid-20 would be a nightmare. But, keeping in mind that the last
>> version of Android is a priority, I think that enabling ifuncs
>> unconditionally would be the right thing.
>>
>> Is the patch ok? Bootstrapped/regtested on x86_64-unknown-linux-gnu +
>> checked that the behavior of i686-linux-android is correct.
>>
>>
>> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
>> index eac19cf..9932323 100644
>> --- a/gcc/ChangeLog
>> +++ b/gcc/ChangeLog
>> @@ -1,3 +1,8 @@
>> +2014-11-11  Alexander Ivchenko  <alexander.ivchenko@intel.com>
>> +
>> +       * config/linux.c (linux_has_ifunc_p): Remove.
>> +       * config/linux.h (TARGET_HAS_IFUNC_P): Use the default version.
>
> This feels like a bad idea to me simply because a new compiler with an old
> runtime will generate code that fails, right?

yes, but that's already true of PIE or gnu-style hash or...

> If you can't do a configure-time test, then the way to go is either a
> compile-time option, or to use a different target.  If there's some minimum
> version of android that has this capability, then this isn't terribly hard.
> You may not even need a config file for this since you could define
> LIBC_BIONIC_USE_IFUNCS or something like that when configured for a suitably
> new android version.

this won't make any difference to developers, though. they get their
prebuilt compilers from us, and we'll just turn all the latest options
on anyway. we don't ship a compilers for each Android version. we
already have 6 architectures * {clang,gcc} * {current,previous}version
to ship.

it doesn't seem worth having configuration in GCC that no one will ever use.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Android] Enable ifuncs on Android
       [not found]   ` <CAJgzZopFPL=5y8ts36Jd_+tH1UOz+2SG7BLZkRj5JnihuACOUw@mail.gmail.com>
@ 2014-11-14  1:23     ` Jeff Law
  2014-11-14  4:28       ` enh
  0 siblings, 1 reply; 15+ messages in thread
From: Jeff Law @ 2014-11-14  1:23 UTC (permalink / raw)
  To: enh, Alexander Ivchenko, GCC Patches; +Cc: Andrew Hsieh

On 11/13/14 10:46, enh wrote:
>     This feels like a bad idea to me simply because a new compiler with an
>     old runtime will generate code that fails, right?
>
>
> yes, but that's already true of PIE or gnu-style hash or...
That doesn't make it the right thing to do.  I would argue that's a bug 
that really needs to be fixed.

>
>     If you can't do a configure-time test, then the way to go is either a
>     compile-time option, or to use a different target.  If there's some
>     minimum version of android that has this capability, then this isn't
>     terribly hard.   You may not even need a config file for this since you
>     could define LIBC_BIONIC_USE_IFUNCS or something like that when
>     configured for a suitably new android version.
>
>
> this won't make any difference to the developers, though. they get their
> prebuilt compilers from us, and we'll just turn all the latest options
> on. we don't ship a compilers for each Android version. we already have
> 6 architectures * {clang,gcc} * {current,previous}version to ship.
But that's no reason to have a compiler which produces bogus binaries.

I really think this patch is a bad idea.

jeff

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Android] Enable ifuncs on Android
  2014-11-14  1:23     ` Jeff Law
@ 2014-11-14  4:28       ` enh
  2014-11-14  4:30         ` Andrew Hsieh
  0 siblings, 1 reply; 15+ messages in thread
From: enh @ 2014-11-14  4:28 UTC (permalink / raw)
  To: Jeff Law; +Cc: Alexander Ivchenko, GCC Patches, Andrew Hsieh

On Thu, Nov 13, 2014 at 5:12 PM, Jeff Law <law@redhat.com> wrote:
> On 11/13/14 10:46, enh wrote:
>>
>>     This feels like a bad idea to me simply because a new compiler with an
>>     old runtime will generate code that fails, right?
>>
>>
>> yes, but that's already true of PIE or gnu-style hash or...
>
> That doesn't make it the right thing to do.  I would argue that's a bug that
> really needs to be fixed.

it's not a bug. no one wants 22 different compilers or configuration
options, with a new one every six months or so.

>>     If you can't do a configure-time test, then the way to go is either a
>>     compile-time option, or to use a different target.  If there's some
>>     minimum version of android that has this capability, then this isn't
>>     terribly hard.   You may not even need a config file for this since
>> you
>>     could define LIBC_BIONIC_USE_IFUNCS or something like that when
>>     configured for a suitably new android version.
>>
>>
>> this won't make any difference to the developers, though. they get their
>> prebuilt compilers from us, and we'll just turn all the latest options
>> on. we don't ship a compilers for each Android version. we already have
>> 6 architectures * {clang,gcc} * {current,previous}version to ship.
>
> But that's no reason to have a compiler which produces bogus binaries.
>
> I really think this patch is a bad idea.

so we should just change linux_has_ifunc_p to return
HAVE_GNU_INDIRECT_FUNCTION; instead?

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Android] Enable ifuncs on Android
  2014-11-14  4:28       ` enh
@ 2014-11-14  4:30         ` Andrew Hsieh
  2014-11-14  4:35           ` H.J. Lu
  0 siblings, 1 reply; 15+ messages in thread
From: Andrew Hsieh @ 2014-11-14  4:30 UTC (permalink / raw)
  To: enh; +Cc: Jeff Law, Alexander Ivchenko, GCC Patches

What about overloading the existing option -mbionic ? -mbionic=21 and
above enable ifunc (so NDK can help enforce it)

On Fri, Nov 14, 2014 at 11:51 AM, enh <enh@google.com> wrote:
> On Thu, Nov 13, 2014 at 5:12 PM, Jeff Law <law@redhat.com> wrote:
>> On 11/13/14 10:46, enh wrote:
>>>
>>>     This feels like a bad idea to me simply because a new compiler with an
>>>     old runtime will generate code that fails, right?
>>>
>>>
>>> yes, but that's already true of PIE or gnu-style hash or...
>>
>> That doesn't make it the right thing to do.  I would argue that's a bug that
>> really needs to be fixed.
>
> it's not a bug. no one wants 22 different compilers or configuration
> options, with a new one every six months or so.
>
>>>     If you can't do a configure-time test, then the way to go is either a
>>>     compile-time option, or to use a different target.  If there's some
>>>     minimum version of android that has this capability, then this isn't
>>>     terribly hard.   You may not even need a config file for this since
>>> you
>>>     could define LIBC_BIONIC_USE_IFUNCS or something like that when
>>>     configured for a suitably new android version.
>>>
>>>
>>> this won't make any difference to the developers, though. they get their
>>> prebuilt compilers from us, and we'll just turn all the latest options
>>> on. we don't ship a compilers for each Android version. we already have
>>> 6 architectures * {clang,gcc} * {current,previous}version to ship.
>>
>> But that's no reason to have a compiler which produces bogus binaries.
>>
>> I really think this patch is a bad idea.
>
> so we should just change linux_has_ifunc_p to return
> HAVE_GNU_INDIRECT_FUNCTION; instead?



-- 
Thanks,
Andrew

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Android] Enable ifuncs on Android
  2014-11-14  4:30         ` Andrew Hsieh
@ 2014-11-14  4:35           ` H.J. Lu
  2014-11-14  4:56             ` Alexander Ivchenko
  0 siblings, 1 reply; 15+ messages in thread
From: H.J. Lu @ 2014-11-14  4:35 UTC (permalink / raw)
  To: Andrew Hsieh; +Cc: enh, Jeff Law, Alexander Ivchenko, GCC Patches

On Thu, Nov 13, 2014 at 8:27 PM, Andrew Hsieh <andrewhsieh@google.com> wrote:
> What about overloading the existing option -mbionic ? -mbionic=21 and
> above enable ifunc (so NDK can help enforce it)
>
> On Fri, Nov 14, 2014 at 11:51 AM, enh <enh@google.com> wrote:
>> On Thu, Nov 13, 2014 at 5:12 PM, Jeff Law <law@redhat.com> wrote:
>>> On 11/13/14 10:46, enh wrote:
>>>>
>>>>     This feels like a bad idea to me simply because a new compiler with an
>>>>     old runtime will generate code that fails, right?
>>>>
>>>>
>>>> yes, but that's already true of PIE or gnu-style hash or...
>>>
>>> That doesn't make it the right thing to do.  I would argue that's a bug that
>>> really needs to be fixed.
>>
>> it's not a bug. no one wants 22 different compilers or configuration
>> options, with a new one every six months or so.
>>
>>>>     If you can't do a configure-time test, then the way to go is either a
>>>>     compile-time option, or to use a different target.  If there's some
>>>>     minimum version of android that has this capability, then this isn't
>>>>     terribly hard.   You may not even need a config file for this since
>>>> you
>>>>     could define LIBC_BIONIC_USE_IFUNCS or something like that when
>>>>     configured for a suitably new android version.
>>>>
>>>>
>>>> this won't make any difference to the developers, though. they get their
>>>> prebuilt compilers from us, and we'll just turn all the latest options
>>>> on. we don't ship a compilers for each Android version. we already have
>>>> 6 architectures * {clang,gcc} * {current,previous}version to ship.
>>>
>>> But that's no reason to have a compiler which produces bogus binaries.
>>>
>>> I really think this patch is a bad idea.
>>
>> so we should just change linux_has_ifunc_p to return
>> HAVE_GNU_INDIRECT_FUNCTION; instead?
>

I don't know exactly what kind problem you are trying to solve.  Why don't
you configure GCC with  --enable-gnu-indirect-function?   If it doesn't work
for your purpose, please tell me exactly what you want to to do and I will
find a solution for you.

-- 
H.J.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Android] Enable ifuncs on Android
  2014-11-14  4:35           ` H.J. Lu
@ 2014-11-14  4:56             ` Alexander Ivchenko
  0 siblings, 0 replies; 15+ messages in thread
From: Alexander Ivchenko @ 2014-11-14  4:56 UTC (permalink / raw)
  To: H.J. Lu; +Cc: Andrew Hsieh, enh, Jeff Law, GCC Patches

-bool
-linux_has_ifunc_p (void)
-{
-  return OPTION_BIONIC ? false : HAVE_GNU_INDIRECT_FUNCTION;
-}

That is what prevent us from doing so. We need to remove OPTION_BIONIC
from that hook and that is what the patch does.

2014-11-14 8:32 GMT+04:00 H.J. Lu <hjl.tools@gmail.com>:
> On Thu, Nov 13, 2014 at 8:27 PM, Andrew Hsieh <andrewhsieh@google.com> wrote:
>> What about overloading the existing option -mbionic ? -mbionic=21 and
>> above enable ifunc (so NDK can help enforce it)
>>
>> On Fri, Nov 14, 2014 at 11:51 AM, enh <enh@google.com> wrote:
>>> On Thu, Nov 13, 2014 at 5:12 PM, Jeff Law <law@redhat.com> wrote:
>>>> On 11/13/14 10:46, enh wrote:
>>>>>
>>>>>     This feels like a bad idea to me simply because a new compiler with an
>>>>>     old runtime will generate code that fails, right?
>>>>>
>>>>>
>>>>> yes, but that's already true of PIE or gnu-style hash or...
>>>>
>>>> That doesn't make it the right thing to do.  I would argue that's a bug that
>>>> really needs to be fixed.
>>>
>>> it's not a bug. no one wants 22 different compilers or configuration
>>> options, with a new one every six months or so.
>>>
>>>>>     If you can't do a configure-time test, then the way to go is either a
>>>>>     compile-time option, or to use a different target.  If there's some
>>>>>     minimum version of android that has this capability, then this isn't
>>>>>     terribly hard.   You may not even need a config file for this since
>>>>> you
>>>>>     could define LIBC_BIONIC_USE_IFUNCS or something like that when
>>>>>     configured for a suitably new android version.
>>>>>
>>>>>
>>>>> this won't make any difference to the developers, though. they get their
>>>>> prebuilt compilers from us, and we'll just turn all the latest options
>>>>> on. we don't ship a compilers for each Android version. we already have
>>>>> 6 architectures * {clang,gcc} * {current,previous}version to ship.
>>>>
>>>> But that's no reason to have a compiler which produces bogus binaries.
>>>>
>>>> I really think this patch is a bad idea.
>>>
>>> so we should just change linux_has_ifunc_p to return
>>> HAVE_GNU_INDIRECT_FUNCTION; instead?
>>
>
> I don't know exactly what kind problem you are trying to solve.  Why don't
> you configure GCC with  --enable-gnu-indirect-function?   If it doesn't work
> for your purpose, please tell me exactly what you want to to do and I will
> find a solution for you.
>
> --
> H.J.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Android] Enable ifuncs on Android
  2014-11-13 17:42 ` Jeff Law
  2014-11-13 18:05   ` enh
       [not found]   ` <CAJgzZopFPL=5y8ts36Jd_+tH1UOz+2SG7BLZkRj5JnihuACOUw@mail.gmail.com>
@ 2014-11-14  4:59   ` H.J. Lu
  2014-11-14  5:17     ` H.J. Lu
  2014-11-14  5:32     ` Jeff Law
  2 siblings, 2 replies; 15+ messages in thread
From: H.J. Lu @ 2014-11-14  4:59 UTC (permalink / raw)
  To: Jeff Law; +Cc: Alexander Ivchenko, GCC Patches, enh, Andrew Hsieh

On Thu, Nov 13, 2014 at 9:36 AM, Jeff Law <law@redhat.com> wrote:
> On 11/12/14 03:02, Alexander Ivchenko wrote:
>>
>> Hi,
>>
>> Bionic - Android libc - supports indirect functions right now, but
>> they are disabled in gcc;
>>
>> We cannot do the configure-time check for that, because there is only
>> one version of each compiler shipped in ndk for all Android platforms.
>> On the other hand, having different runtime targets like -mandroid-19,
>> -mandroid-20 would be a nightmare. But, keeping in mind that the last
>> version of Android is a priority, I think that enabling ifuncs
>> unconditionally would be the right thing.
>>
>> Is the patch ok? Bootstrapped/regtested on x86_64-unknown-linux-gnu +
>> checked that the behavior of i686-linux-android is correct.
>>
>>
>> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
>> index eac19cf..9932323 100644
>> --- a/gcc/ChangeLog
>> +++ b/gcc/ChangeLog
>> @@ -1,3 +1,8 @@
>> +2014-11-11  Alexander Ivchenko  <alexander.ivchenko@intel.com>
>> +
>> +       * config/linux.c (linux_has_ifunc_p): Remove.
>> +       * config/linux.h (TARGET_HAS_IFUNC_P): Use the default version.
>
> This feels like a bad idea to me simply because a new compiler with an old
> runtime will generate code that fails, right?
>
> If you can't do a configure-time test, then the way to go is either a
> compile-time option, or to use a different target.  If there's some minimum
> version of android that has this capability, then this isn't terribly hard.
> You may not even need a config file for this since you could define
> LIBC_BIONIC_USE_IFUNCS or something like that when configured for a suitably
> new android version.

Hi Jeff,

I believe the patch is correct.  Not all glibcs support IFUNC. It doesn't mean
we should disable IFUNC for all glibcs.  By default, we do a configure time
check for IFUNC.  We DO want to use configure time check for IFUNC for
Android NDK.

Alexander, configure time check for IFUNC should work for Android.  If
it doesn't,
please let me know and I will fix it.


-- 
H.J.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Android] Enable ifuncs on Android
  2014-11-14  4:59   ` H.J. Lu
@ 2014-11-14  5:17     ` H.J. Lu
  2014-11-14  5:32     ` Jeff Law
  1 sibling, 0 replies; 15+ messages in thread
From: H.J. Lu @ 2014-11-14  5:17 UTC (permalink / raw)
  To: Jeff Law; +Cc: Alexander Ivchenko, GCC Patches, enh, Andrew Hsieh

On Thu, Nov 13, 2014 at 8:46 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Nov 13, 2014 at 9:36 AM, Jeff Law <law@redhat.com> wrote:
>> On 11/12/14 03:02, Alexander Ivchenko wrote:
>>>
>>> Hi,
>>>
>>> Bionic - Android libc - supports indirect functions right now, but
>>> they are disabled in gcc;
>>>
>>> We cannot do the configure-time check for that, because there is only
>>> one version of each compiler shipped in ndk for all Android platforms.
>>> On the other hand, having different runtime targets like -mandroid-19,
>>> -mandroid-20 would be a nightmare. But, keeping in mind that the last
>>> version of Android is a priority, I think that enabling ifuncs
>>> unconditionally would be the right thing.
>>>
>>> Is the patch ok? Bootstrapped/regtested on x86_64-unknown-linux-gnu +
>>> checked that the behavior of i686-linux-android is correct.
>>>
>>>
>>> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
>>> index eac19cf..9932323 100644
>>> --- a/gcc/ChangeLog
>>> +++ b/gcc/ChangeLog
>>> @@ -1,3 +1,8 @@
>>> +2014-11-11  Alexander Ivchenko  <alexander.ivchenko@intel.com>
>>> +
>>> +       * config/linux.c (linux_has_ifunc_p): Remove.
>>> +       * config/linux.h (TARGET_HAS_IFUNC_P): Use the default version.
>>
>> This feels like a bad idea to me simply because a new compiler with an old
>> runtime will generate code that fails, right?
>>
>> If you can't do a configure-time test, then the way to go is either a
>> compile-time option, or to use a different target.  If there's some minimum
>> version of android that has this capability, then this isn't terribly hard.
>> You may not even need a config file for this since you could define
>> LIBC_BIONIC_USE_IFUNCS or something like that when configured for a suitably
>> new android version.
>
> Hi Jeff,
>
> I believe the patch is correct.  Not all glibcs support IFUNC. It doesn't mean
> we should disable IFUNC for all glibcs.  By default, we do a configure time
> check for IFUNC.  We DO want to use configure time check for IFUNC for
> Android NDK.
>
> Alexander, configure time check for IFUNC should work for Android.  If
> it doesn't,
> please let me know and I will fix it.
>
>

Jeff, another thing.  Alexander original message:

---
We cannot do the configure-time check for that, because there is only
one version of each compiler shipped in ndk for all Android platforms.
On the other hand, having different runtime targets like -mandroid-19,
-mandroid-20 would be a nightmare. But, keeping in mind that the last
version of Android is a priority, I think that enabling ifuncs
unconditionally would be the right thing.
---

isn't precise.  That isn't what his patch does.  All his patch does is
to check IFUNC support at GCC configure-time when targetting
Android.  His patch alone won't "enabling ifuncs unconditionally" for
any targets, Android or not.


-- 
H.J.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Android] Enable ifuncs on Android
  2014-11-14  4:59   ` H.J. Lu
  2014-11-14  5:17     ` H.J. Lu
@ 2014-11-14  5:32     ` Jeff Law
  2014-11-14  6:06       ` H.J. Lu
  2014-11-14 16:51       ` H.J. Lu
  1 sibling, 2 replies; 15+ messages in thread
From: Jeff Law @ 2014-11-14  5:32 UTC (permalink / raw)
  To: H.J. Lu; +Cc: Alexander Ivchenko, GCC Patches, enh, Andrew Hsieh

On 11/13/14 21:46, H.J. Lu wrote:

>
> Hi Jeff,
>
> I believe the patch is correct.  Not all glibcs support IFUNC. It doesn't mean
> we should disable IFUNC for all glibcs.  By default, we do a configure time
> check for IFUNC.  We DO want to use configure time check for IFUNC for
> Android NDK.
>I
I'm sorry, I'm objecting to this patch.  It is wrong for GCC to produce 
code that is incorrect solely for the benefit of optimizing for a new 
release of a library.

The right way is to trigger the new code on some configure time option 
or a new configuration itself.

Whether or not GCC is doing this right for glibc is irrelevant, the 
patch is _wrong_.

jeff


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Android] Enable ifuncs on Android
  2014-11-14  5:32     ` Jeff Law
@ 2014-11-14  6:06       ` H.J. Lu
  2014-11-14 16:51       ` H.J. Lu
  1 sibling, 0 replies; 15+ messages in thread
From: H.J. Lu @ 2014-11-14  6:06 UTC (permalink / raw)
  To: Jeff Law; +Cc: Alexander Ivchenko, GCC Patches, enh, Andrew Hsieh

On Thu, Nov 13, 2014 at 8:59 PM, Jeff Law <law@redhat.com> wrote:
> On 11/13/14 21:46, H.J. Lu wrote:
>
>>
>> Hi Jeff,
>>
>> I believe the patch is correct.  Not all glibcs support IFUNC. It doesn't
>> mean
>> we should disable IFUNC for all glibcs.  By default, we do a configure
>> time
>> check for IFUNC.  We DO want to use configure time check for IFUNC for
>> Android NDK.
>> I
>
> I'm sorry, I'm objecting to this patch.  It is wrong for GCC to produce code
> that is incorrect solely for the benefit of optimizing for a new release of
> a library.
>
> The right way is to trigger the new code on some configure time option or a
> new configuration itself.
>
> Whether or not GCC is doing this right for glibc is irrelevant, the patch is
> _wrong_.
>

I see where the problem is now. For i[34567]86-*-linux* and x86_64-*-linux*
targets, config.gcc assumes modern glibc and enable IFUNC by default.
That is wrong for  i[34567]86-*-linux-android* and and x86_64-*-linux-android*
targets.  This patch fixes it.  The original patch should work with it.


-- 
H.J.
---
2014-11-13  H.J. Lu  <hongjiu.lu@intel.com>

* config.gcc (default_gnu_indirect_function): Don't assume
modern glibc when targeting Android.

diff --git a/gcc/config.gcc b/gcc/config.gcc
index a6b37d8..1f01a64 100644
--- a/gcc/config.gcc
+++ b/gcc/config.gcc
@@ -1404,8 +1404,11 @@ i[34567]86-*-linux* |
i[34567]86-*-kfreebsd*-gnu | i[34567]86-*-knetbsd*-gnu | i
  i[34567]86-*-linux*)
  tm_file="${tm_file} linux.h linux-android.h"
  extra_options="${extra_options} linux-android.opt"
- # Assume modern glibc
- default_gnu_indirect_function=yes
+ # Assume modern glibc unless we are targeting Android
+ case $target in
+                *-*-*android*) ;;
+ *) default_gnu_indirect_function=yes ;;
+ esac
  if test x$enable_targets = xall; then
  tm_file="${tm_file} i386/x86-64.h i386/gnu-user-common.h
i386/gnu-user64.h i386/linux-common.h i386/linux64.h"
  tm_defines="${tm_defines} TARGET_BI_ARCH=1"
@@ -1467,8 +1470,11 @@ x86_64-*-linux* | x86_64-*-kfreebsd*-gnu |
x86_64-*-knetbsd*-gnu)
  x86_64-*-linux*)
  tm_file="${tm_file} linux.h linux-android.h i386/linux-common.h
i386/linux64.h"
  extra_options="${extra_options} linux-android.opt"
- # Assume modern glibc
- default_gnu_indirect_function=yes
+ # Assume modern glibc unless we are targeting Android
+ case $target in
+                *-*-*android*) ;;
+ *) default_gnu_indirect_function=yes ;;
+ esac
    ;;
  x86_64-*-kfreebsd*-gnu)
  tm_file="${tm_file} kfreebsd-gnu.h i386/kfreebsd-gnu64.h"
[hjl@gnu-6 gcc]$

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Android] Enable ifuncs on Android
  2014-11-14  5:32     ` Jeff Law
  2014-11-14  6:06       ` H.J. Lu
@ 2014-11-14 16:51       ` H.J. Lu
  2014-11-24 11:17         ` Alexander Ivchenko
  1 sibling, 1 reply; 15+ messages in thread
From: H.J. Lu @ 2014-11-14 16:51 UTC (permalink / raw)
  To: Jeff Law; +Cc: Alexander Ivchenko, GCC Patches, enh, Andrew Hsieh

On Thu, Nov 13, 2014 at 8:59 PM, Jeff Law <law@redhat.com> wrote:
> On 11/13/14 21:46, H.J. Lu wrote:
>
>>
>> Hi Jeff,
>>
>> I believe the patch is correct.  Not all glibcs support IFUNC. It doesn't
>> mean
>> we should disable IFUNC for all glibcs.  By default, we do a configure
>> time
>> check for IFUNC.  We DO want to use configure time check for IFUNC for
>> Android NDK.
>> I
>
> I'm sorry, I'm objecting to this patch.  It is wrong for GCC to produce code
> that is incorrect solely for the benefit of optimizing for a new release of
> a library.
>
> The right way is to trigger the new code on some configure time option or a
> new configuration itself.
>
> Whether or not GCC is doing this right for glibc is irrelevant, the patch is
> _wrong_.
>

Hi Jeff,

I have checked in my patch:

https://gcc.gnu.org/ml/gcc-cvs/2014-11/msg00594.html

so that IFUNC is disabled by default for both Android and uclibc.
Is Alexander's patch OK for trunk?

Thanks.

-- 
H.J.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Android] Enable ifuncs on Android
  2014-11-14 16:51       ` H.J. Lu
@ 2014-11-24 11:17         ` Alexander Ivchenko
  2014-11-24 22:05           ` Jeff Law
  0 siblings, 1 reply; 15+ messages in thread
From: Alexander Ivchenko @ 2014-11-24 11:17 UTC (permalink / raw)
  To: H.J. Lu; +Cc: Jeff Law, GCC Patches, enh, Andrew Hsieh

*ping*

thanks,
Alexander

2014-11-14 19:49 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
> On Thu, Nov 13, 2014 at 8:59 PM, Jeff Law <law@redhat.com> wrote:
>> On 11/13/14 21:46, H.J. Lu wrote:
>>
>>>
>>> Hi Jeff,
>>>
>>> I believe the patch is correct.  Not all glibcs support IFUNC. It doesn't
>>> mean
>>> we should disable IFUNC for all glibcs.  By default, we do a configure
>>> time
>>> check for IFUNC.  We DO want to use configure time check for IFUNC for
>>> Android NDK.
>>> I
>>
>> I'm sorry, I'm objecting to this patch.  It is wrong for GCC to produce code
>> that is incorrect solely for the benefit of optimizing for a new release of
>> a library.
>>
>> The right way is to trigger the new code on some configure time option or a
>> new configuration itself.
>>
>> Whether or not GCC is doing this right for glibc is irrelevant, the patch is
>> _wrong_.
>>
>
> Hi Jeff,
>
> I have checked in my patch:
>
> https://gcc.gnu.org/ml/gcc-cvs/2014-11/msg00594.html
>
> so that IFUNC is disabled by default for both Android and uclibc.
> Is Alexander's patch OK for trunk?
>
> Thanks.
>
> --
> H.J.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Android] Enable ifuncs on Android
  2014-11-24 11:17         ` Alexander Ivchenko
@ 2014-11-24 22:05           ` Jeff Law
  0 siblings, 0 replies; 15+ messages in thread
From: Jeff Law @ 2014-11-24 22:05 UTC (permalink / raw)
  To: Alexander Ivchenko, H.J. Lu; +Cc: GCC Patches, enh, Andrew Hsieh

On 11/24/14 02:49, Alexander Ivchenko wrote:
> *ping*
>
> thanks,
> Alexander
Can you put a reference to the actual patch you want to ping (or just 
attach the patch you want to ping)?  I'll review given the updated 
context/changes from HJ.

jeff

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2014-11-24 21:26 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-11-12 10:03 [Android] Enable ifuncs on Android Alexander Ivchenko
2014-11-13 17:42 ` Jeff Law
2014-11-13 18:05   ` enh
     [not found]   ` <CAJgzZopFPL=5y8ts36Jd_+tH1UOz+2SG7BLZkRj5JnihuACOUw@mail.gmail.com>
2014-11-14  1:23     ` Jeff Law
2014-11-14  4:28       ` enh
2014-11-14  4:30         ` Andrew Hsieh
2014-11-14  4:35           ` H.J. Lu
2014-11-14  4:56             ` Alexander Ivchenko
2014-11-14  4:59   ` H.J. Lu
2014-11-14  5:17     ` H.J. Lu
2014-11-14  5:32     ` Jeff Law
2014-11-14  6:06       ` H.J. Lu
2014-11-14 16:51       ` H.J. Lu
2014-11-24 11:17         ` Alexander Ivchenko
2014-11-24 22:05           ` Jeff Law

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).