| Message ID | 201706121502.v5CF2Lj1000567@sellcey-dt.caveonetworks.com |
|---|---|
| State | New |
| Headers | show |
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.)
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. 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-*)
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. >
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.
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
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"