* [PATCH][aarch64] Enable ifunc resolver attribute by default @ 2017-06-12 15:02 Steve Ellcey 2017-09-04 14:40 ` Szabolcs Nagy 0 siblings, 1 reply; 6+ messages in thread From: Steve Ellcey @ 2017-06-12 15:02 UTC (permalink / raw) To: gcc-patches I recently noticed that the GCC 'resolver' attribute used for ifunc's is not on by default for aarch64 even though all the infrastructure to support it is in place. I made memcpy an ifunc on aarch64 in glibc and am looking at possibly using it for libatomic too. For this reason I would like to enable it by default. Note that the memcpy ifunc works even when this is not enabled because glibc enables ifuncs by using the assembly language .type psuedo-op to set the resolver attribute when GCC cannot do it with an attribute. Using an ifunc in libatomic does require this to be enabled and I do not see any reason not to have it enabled by default. Tested with no regressions, OK to check in? Steve Ellcey sellcey@cavium.com 2017-06-12 Steve Ellcey <sellcey@cavium.com> * config.gcc (aarch64*-*-linux*): Enable IFUNC by default. diff --git a/gcc/config.gcc b/gcc/config.gcc index a311cd95..e4caca4 100644 --- a/gcc/config.gcc +++ b/gcc/config.gcc @@ -974,6 +974,7 @@ aarch64*-*-freebsd*) tmake_file="${tmake_file} aarch64/t-aarch64 aarch64/t-aarch64-freebsd" ;; aarch64*-*-linux*) + default_gnu_indirect_function=yes tm_file="${tm_file} dbxelf.h elfos.h gnu-user.h linux.h glibc-stdint.h" tm_file="${tm_file} aarch64/aarch64-elf.h aarch64/aarch64-linux.h" tmake_file="${tmake_file} aarch64/t-aarch64 aarch64/t-aarch64-linux" ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][aarch64] Enable ifunc resolver attribute by default 2017-06-12 15:02 [PATCH][aarch64] Enable ifunc resolver attribute by default Steve Ellcey @ 2017-09-04 14:40 ` Szabolcs Nagy 2017-09-05 17:09 ` Steve Ellcey 0 siblings, 1 reply; 6+ messages in thread From: Szabolcs Nagy @ 2017-09-04 14:40 UTC (permalink / raw) To: sellcey, gcc-patches; +Cc: nd On 12/06/17 16:02, Steve Ellcey wrote: > I recently noticed that the GCC 'resolver' attribute used for ifunc's is not > on by default for aarch64 even though all the infrastructure to support it is > in place. I made memcpy an ifunc on aarch64 in glibc and am looking at > possibly using it for libatomic too. For this reason I would like to enable > it by default. Note that the memcpy ifunc works even when this is not enabled > because glibc enables ifuncs by using the assembly language .type psuedo-op to > set the resolver attribute when GCC cannot do it with an attribute. Using > an ifunc in libatomic does require this to be enabled and I do not see any > reason not to have it enabled by default. > > Tested with no regressions, OK to check in? > this is not the right default for bionic, uclibc and musl (gcc does not distinguish between supporting ifunc in the compiler vs runtime, so when ifunc is enabled it is assumed the c runtime will have support too, hence libatomic and libgcc starts using ifuncs which breaks at runtime) so don't change the default if target matches *-*-*android*|*-*-*uclibc*|*-*-*musl*) (i think the default should be kept "no" for these targets independently of cpu arch, so the current logic that is repeated many places in config.gcc is suboptimal. and i think the attribute syntax should be always supported and this setting should only mean that ifunc use is allowed in the runtime libraries.) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][aarch64] Enable ifunc resolver attribute by default 2017-09-04 14:40 ` Szabolcs Nagy @ 2017-09-05 17:09 ` Steve Ellcey 2017-09-05 17:20 ` Szabolcs Nagy 2017-09-21 12:37 ` Joseph Myers 0 siblings, 2 replies; 6+ messages in thread From: Steve Ellcey @ 2017-09-05 17:09 UTC (permalink / raw) To: Szabolcs Nagy, gcc-patches; +Cc: nd [-- Attachment #1: Type: text/plain, Size: 1759 bytes --] On Mon, 2017-09-04 at 15:40 +0100, Szabolcs Nagy wrote: > this is not the right default for bionic, uclibc and musl > > (gcc does not distinguish between supporting ifunc in the > compiler vs runtime, so when ifunc is enabled it is assumed > the c runtime will have support too, hence libatomic and > libgcc starts using ifuncs which breaks at runtime) > > so don't change the default if target matches > *-*-*android*|*-*-*uclibc*|*-*-*musl*) > > (i think the default should be kept "no" for these targets > independently of cpu arch, so the current logic that is > repeated many places in config.gcc is suboptimal. I cleaned up config.gcc so default_gnu_indirect_function is set in a single place now and has the right defaults for android/uclibc/musl. > and i think the attribute syntax should be always supported > and this setting should only mean that ifunc use is allowed > in the runtime libraries.) I think that might be a reasonable thing to do but should be a separate patch from this change, so I have not done anything with that. I retested on aarch64 but I did not test any of the other platforms where I moved the setting of default_gnu_indirect_function, but I don't think I changed any defaults. Steve Ellcey sellcey@cavium.com 2017-09-05  Steve Ellcey  <sellcey@cavium.com>         * config.gcc: Add new case statement to set         default_gnu_indirect_function.  Remove it from x86_64-*-linux*,         i[34567]86-*, powerpc*-*-linux*spe*, powerpc*-*-linux*, s390-*-linux*,         s390x-*-linux* case statements.   Added aarch64 to the list of         supported architectures. [-- Attachment #2: ifunc.gcc.patch --] [-- Type: text/x-patch, Size: 3169 bytes --] diff --git a/gcc/config.gcc b/gcc/config.gcc index cc56c57..1a1b2fe 100644 --- a/gcc/config.gcc +++ b/gcc/config.gcc @@ -1516,14 +1516,6 @@ i[34567]86-*-linux* | i[34567]86-*-kfreebsd*-gnu | i[34567]86-*-gnu* | i[34567]8 i[34567]86-*-linux*) tm_file="${tm_file} linux.h linux-android.h" extra_options="${extra_options} linux-android.opt" - # Assume modern glibc if not targeting Android nor uclibc. - case ${target} in - *-*-*android*|*-*-*uclibc*|*-*-*musl*) - ;; - *) - 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" @@ -1582,14 +1574,6 @@ x86_64-*-linux* | x86_64-*-kfreebsd*-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 if not targeting Android nor uclibc. - case ${target} in - *-*-*android*|*-*-*uclibc*|*-*-*musl*) - ;; - *) - default_gnu_indirect_function=yes - ;; - esac ;; x86_64-*-kfreebsd*-gnu) tm_file="${tm_file} kfreebsd-gnu.h i386/kfreebsd-gnu64.h" @@ -2455,7 +2439,6 @@ powerpc*-*-linux*spe*) tm_file="${tm_file} powerpcspe/linux.h glibc-stdint.h" tmake_file="${tmake_file} powerpcspe/t-ppcos powerpcspe/t-linux" tm_file="${tm_file} powerpcspe/linuxspe.h powerpcspe/e500.h" - default_gnu_indirect_function=yes ;; powerpc*-*-linux*) tm_file="${tm_file} dbxelf.h elfos.h gnu-user.h freebsd-spec.h rs6000/sysv4.h" @@ -2535,14 +2518,6 @@ powerpc*-*-linux*) if test x${enable_secureplt} = xyes; then tm_file="rs6000/secureplt.h ${tm_file}" fi - # Assume modern glibc if not targeting Android nor uclibc. - case ${target} in - *-*-*android*|*-*-*uclibc*|*-*-*musl*) - ;; - *) - default_gnu_indirect_function=yes - ;; - esac ;; powerpc-wrs-vxworksspe) tm_file="${tm_file} elfos.h freebsd-spec.h powerpcspe/sysv4.h" @@ -2664,7 +2639,6 @@ rx-*-elf*) tmake_file="${tmake_file} rx/t-rx" ;; s390-*-linux*) - default_gnu_indirect_function=yes tm_file="s390/s390.h dbxelf.h elfos.h gnu-user.h linux.h glibc-stdint.h s390/linux.h" c_target_objs="${c_target_objs} s390-c.o" cxx_target_objs="${cxx_target_objs} s390-c.o" @@ -2674,7 +2648,6 @@ s390-*-linux*) tmake_file="${tmake_file} s390/t-s390" ;; s390x-*-linux*) - default_gnu_indirect_function=yes tm_file="s390/s390x.h s390/s390.h dbxelf.h elfos.h gnu-user.h linux.h glibc-stdint.h s390/linux.h" tm_p_file="linux-protos.h s390/s390-protos.h" c_target_objs="${c_target_objs} s390-c.o" @@ -3120,6 +3093,20 @@ case ${target} in ;; esac +# Assume the existence of indirect function support and allow the use of the +# resolver attribute. +case ${target} in +*-*-linux*android*|*-*-linux*uclibc*|*-*-linux*musl*) + ;; +*-*-linux*) + case ${target} in + aarch64*-* | i[34567]86-* | powerpc*-* | s390*-* | x86_64-*) + default_gnu_indirect_function=yes + ;; + esac + ;; +esac + # Build mkoffload tool case ${target} in *-intelmic-* | *-intelmicemul-*) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][aarch64] Enable ifunc resolver attribute by default 2017-09-05 17:09 ` Steve Ellcey @ 2017-09-05 17:20 ` Szabolcs Nagy 2017-09-21 12:37 ` Joseph Myers 1 sibling, 0 replies; 6+ messages in thread From: Szabolcs Nagy @ 2017-09-05 17:20 UTC (permalink / raw) To: sellcey, gcc-patches; +Cc: nd On 05/09/17 18:09, Steve Ellcey wrote: > On Mon, 2017-09-04 at 15:40 +0100, Szabolcs Nagy wrote: > >> this is not the right default for bionic, uclibc and musl >> >> (gcc does not distinguish between supporting ifunc in the >> compiler vs runtime, so when ifunc is enabled it is assumed >> the c runtime will have support too, hence libatomic and >> libgcc starts using ifuncs which breaks at runtime) >> >> so don't change the default if target matches >> *-*-*android*|*-*-*uclibc*|*-*-*musl*) >> >> (i think the default should be kept "no" for these targets >> independently of cpu arch, so the current logic that is >> repeated many places in config.gcc is suboptimal. > > I cleaned up config.gcc so default_gnu_indirect_function is set in a > single place now and has the right defaults for android/uclibc/musl. > thanks, it looks ok to me (but i cannot approve the patch). >> and i think the attribute syntax should be always supported >> and this setting should only mean that ifunc use is allowed >> in the runtime libraries.) > > I think that might be a reasonable thing to do but should be a > separate patch from this change, so I have not done anything > with that. > > I retested on aarch64 but I did not test any of the other platforms > where I moved the setting of default_gnu_indirect_function, but I > don't think I changed any defaults. > > Steve Ellcey > sellcey@cavium.com > > > 2017-09-05 Steve Ellcey <sellcey@cavium.com> > > * config.gcc: Add new case statement to set > default_gnu_indirect_function. Remove it from x86_64-*-linux*, > i[34567]86-*, powerpc*-*-linux*spe*, powerpc*-*-linux*, s390-*-linux*, > s390x-*-linux* case statements. Added aarch64 to the list of > supported architectures. > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][aarch64] Enable ifunc resolver attribute by default 2017-09-05 17:09 ` Steve Ellcey 2017-09-05 17:20 ` Szabolcs Nagy @ 2017-09-21 12:37 ` Joseph Myers 2017-09-22 18:06 ` Steve Ellcey 1 sibling, 1 reply; 6+ messages in thread From: Joseph Myers @ 2017-09-21 12:37 UTC (permalink / raw) To: Steve Ellcey Cc: Szabolcs Nagy, gcc-patches, nd, richard.earnshaw, james.greenhalgh, marcus.shawcroft, davem, ebotcazou, nickc, ramana.radhakrishnan, kyrylo.tkachov [-- Attachment #1: Type: text/plain, Size: 1345 bytes --] On Tue, 5 Sep 2017, Steve Ellcey wrote: > 2017-09-05  Steve Ellcey  <sellcey@cavium.com> > >         * config.gcc: Add new case statement to set >         default_gnu_indirect_function.  Remove it from x86_64-*-linux*, >         i[34567]86-*, powerpc*-*-linux*spe*, powerpc*-*-linux*, s390-*-linux*, >         s390x-*-linux* case statements.   Added aarch64 to the list of >         supported architectures. This is OK subject to AArch64 maintainer approval, or in the absence of AArch64 maintainer objections within 24 hours. I think we also need to add SPARC to the list of architectures here to fix the glibc build there, but that can be done separately. It would make sense to add ARM, since that has IFUNC support as well (but it's not needed for the glibc build there at present, as all the multi-arch ARM IFUNCs are defined in .S files). And as discussed I'm dubious of having an architecture list here at all - it would be better for GCC to support this feature for all GNU targets, so that existing GCC versions work with new binutils when the feature is added for more architectures - but that can also be considered separately. -- Joseph S. Myers joseph@codesourcery.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][aarch64] Enable ifunc resolver attribute by default 2017-09-21 12:37 ` Joseph Myers @ 2017-09-22 18:06 ` Steve Ellcey 0 siblings, 0 replies; 6+ messages in thread From: Steve Ellcey @ 2017-09-22 18:06 UTC (permalink / raw) To: Joseph Myers Cc: Szabolcs Nagy, gcc-patches, nd, richard.earnshaw, james.greenhalgh, marcus.shawcroft, davem, ebotcazou, nickc, ramana.radhakrishnan, kyrylo.tkachov On Thu, 2017-09-21 at 12:37 +0000, Joseph Myers wrote: > On Tue, 5 Sep 2017, Steve Ellcey wrote: > > > > > 2017-09-05  Steve Ellcey  <sellcey@cavium.com> > > > >         * config.gcc: Add new case statement to set > >         default_gnu_indirect_function.  Remove it from x86_64-*-linux*, > >         i[34567]86-*, powerpc*-*-linux*spe*, powerpc*-*-linux*, s390-*-linux*, > >         s390x-*-linux* case statements.   Added aarch64 to the list of > >         supported architectures. > This is OK subject to AArch64 maintainer approval, or in the absence of > AArch64 maintainer objections within 24 hours. Since I didn't see any objections I have checked this in. Steve Ellcey sellcey@cavium.com ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-09-22 18:06 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-06-12 15:02 [PATCH][aarch64] Enable ifunc resolver attribute by default Steve Ellcey 2017-09-04 14:40 ` Szabolcs Nagy 2017-09-05 17:09 ` Steve Ellcey 2017-09-05 17:20 ` Szabolcs Nagy 2017-09-21 12:37 ` Joseph Myers 2017-09-22 18:06 ` Steve Ellcey
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).