| Message ID | 20200306203721.15886-6-murphyp@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Enable IEEE binary128 long double on powerpc64le | expand |
On Fri, 6 Mar 2020, Paul E. Murphy wrote: > In preparation for the transition of the format of long double - from > IBM Extended Precision to IEEE 754 128-bits floating-point - on > powerpc64le, this patch adds the linking of the loader to the tests for > long double, since after the switch they will also depend on > __parse_hwcap_and_convert_at_platform. I'm afraid this patch looks unmaintainable. It adds a duplicate list of libm tests, with no obvious logic for what goes in that list, to an architecture-specific Makefile. That seems like a recipe for patches that add a libm test, changing only architecture-independent code, accidentally breaking the build for powerpc64le. Things should be designed in such a way that normal architecture-independent changes, such as adding new libm tests, do not require any knowledge of the existence of such a powerpc64le-specific list. As these are generally normal tests, not tests in tests-internal, I presume they do not in fact use any internal glibc interfaces and so would work fine when built with an installed compiler and glibc. So I think you need to identify exactly what is different (to cause the problem this patch is addressing) between normal builds of user code with installed tools, and the build of tests as part of the glibc testsuite build, and fix that difference (globally, not limited to these particular tests or this particular architecture) in such a way that these tests just work without a duplicate architecture-specific list of tests being needed.
On 3/6/20 6:31 PM, Joseph Myers wrote: > On Fri, 6 Mar 2020, Paul E. Murphy wrote: > >> In preparation for the transition of the format of long double - from >> IBM Extended Precision to IEEE 754 128-bits floating-point - on >> powerpc64le, this patch adds the linking of the loader to the tests for >> long double, since after the switch they will also depend on >> __parse_hwcap_and_convert_at_platform. > > I'm afraid this patch looks unmaintainable. > > It adds a duplicate list of libm tests, with no obvious logic for what > goes in that list, to an architecture-specific Makefile. That seems like > a recipe for patches that add a libm test, changing only > architecture-independent code, accidentally breaking the build for > powerpc64le. > > Things should be designed in such a way that normal > architecture-independent changes, such as adding new libm tests, do not > require any knowledge of the existence of such a powerpc64le-specific > list. > > As these are generally normal tests, not tests in tests-internal, I > presume they do not in fact use any internal glibc interfaces and so would > work fine when built with an installed compiler and glibc. So I think you > need to identify exactly what is different (to cause the problem this > patch is addressing) between normal builds of user code with installed > tools, and the build of tests as part of the glibc testsuite build, and > fix that difference (globally, not limited to these particular tests or > this particular architecture) in such a way that these tests just work > without a duplicate architecture-specific list of tests being needed. > I don't disagree. I think this workaround can be unilaterally added to gnulib-tests on ppc64le [1]. Though, I suspect that this is not the most accurate solution. Looking at the comments in Makerules: # Compiler arguments to use to link a shared object with libc and # ld.so. This is intended to be as similar as possible to a default # link with an installed libc. and poking around with GCC, I admit I don't fully understand all the (seemingly) implicit behavior of the default linking command. Does: -Wl,--as-needed $(elf-objpfx)ld.so -Wl,--no-as-needed ... -lgcc_s ... accurately match the behavior of the default link command on ppc64le? [1] This change, and implied removal any f128-loader-link usage. --- a/sysdeps/powerpc/powerpc64/le/Makefile +++ b/sysdeps/powerpc/powerpc64/le/Makefile @@ -5,7 +5,7 @@ type-float128-CFLAGS := -mfloat128 # libgcc requires __tcb_parse_hwcap_and_convert_at_platform when built with # a binary128 type. That symbol is provided by the loader on dynamically # linked executables, forcing to link the loader after libgcc link. -f128-loader-link = -Wl,--as-needed $(elf-objpfx)ld.so -Wl,--no-as-needed +gnulib-tests += -Wl,--as-needed $(elf-objpfx)ld.so -Wl,--no-as-needed # Bootstrapping code for enabling IEEE 128. This can be removed and # any indirections simplified once IEEE 128 long double is enabled.
On Mon, 9 Mar 2020, Paul E Murphy wrote: > I don't disagree. I think this workaround can be unilaterally added to > gnulib-tests on ppc64le [1]. Though, I suspect that this is not the most > accurate solution. > > Looking at the comments in Makerules: > > # Compiler arguments to use to link a shared object with libc and > # ld.so. This is intended to be as similar as possible to a default > # link with an installed libc. > > and poking around with GCC, I admit I don't fully understand all the > (seemingly) implicit behavior of the default linking command. Does: > > -Wl,--as-needed $(elf-objpfx)ld.so -Wl,--no-as-needed ... -lgcc_s ... > > accurately match the behavior of the default link command on ppc64le? As I understand it, GCC does "-lgcc -lc -lgcc" (with various --as-needed etc. variations). But glibc is doing something like "-lc -lgcc", and your patch is proposing adding an ld.so link to the makefile variable expanding to some form of -lgcc. (In both cases, there is an as-needed ld.so link as part of the -lc link; directly in the linker command in the glibc case, via the libc.so linker script in the normal installed GCC case.) So I wonder what would break if we added linking with libgcc before linking with libc in the relevant settings in glibc, which would seem closest to what installed GCC does by default. When (across glibc architectures) does (static) libgcc define some symbols also defined by glibc's libc, and what would be the effects of linking first with libgcc and thus getting the libgcc versions of any such symbols instead of the libc ones? And would linking with libgcc in that place (as well as after libc) fix the issue you see?
On 3/9/20 6:19 PM, Joseph Myers wrote: > On Mon, 9 Mar 2020, Paul E Murphy wrote: > >> I don't disagree. I think this workaround can be unilaterally added to >> gnulib-tests on ppc64le [1]. Though, I suspect that this is not the most >> accurate solution. >> >> Looking at the comments in Makerules: >> >> # Compiler arguments to use to link a shared object with libc and >> # ld.so. This is intended to be as similar as possible to a default >> # link with an installed libc. >> >> and poking around with GCC, I admit I don't fully understand all the >> (seemingly) implicit behavior of the default linking command. Does: >> >> -Wl,--as-needed $(elf-objpfx)ld.so -Wl,--no-as-needed ... -lgcc_s ... >> >> accurately match the behavior of the default link command on ppc64le? > > As I understand it, GCC does "-lgcc -lc -lgcc" (with various --as-needed > etc. variations). But glibc is doing something like "-lc -lgcc", and your > patch is proposing adding an ld.so link to the makefile variable expanding > to some form of -lgcc. (In both cases, there is an as-needed ld.so link > as part of the -lc link; directly in the linker command in the glibc case, > via the libc.so linker script in the normal installed GCC case.) > > So I wonder what would break if we added linking with libgcc before > linking with libc in the relevant settings in glibc, which would seem > closest to what installed GCC does by default. When (across glibc > architectures) does (static) libgcc define some symbols also defined by > glibc's libc, and what would be the effects of linking first with libgcc > and thus getting the libgcc versions of any such symbols instead of the > libc ones? And would linking with libgcc in that place (as well as after > libc) fix the issue you see? > Are you suggesting something like [2] would be more accurate? Nothing outright broke on powerpc64le when testing on ppc64le with all the ieee128 patches. As long as libgcc is linked prior to ld, the workaround used by f128-loader-link is never needed for tests which implicitly utilize K{F,C}mode libgcc routines. [2]: diff --git a/Makeconfig b/Makeconfig index f252842979..0e80234897 100644 --- a/Makeconfig +++ b/Makeconfig @@ -570,7 +570,7 @@ link-libc-before-gnulib = $(common-objpfx)libc.so$(libc.so-version) \ -Wl,--no-as-needed link-libc = $(link-libc-before-gnulib) $(gnulib) -link-libc-tests-after-rpath-link = $(link-libc-before-gnulib) $(gnulib-tests) +link-libc-tests-after-rpath-link = -lgcc $(link-libc-before-gnulib) $(gnulib-tests) link-libc-tests = $(link-libc-tests-rpath-link) \ $(link-libc-tests-after-rpath-link) # Pretty printer test programs always require rpath instead of rpath-link.
On Tue, 10 Mar 2020, Paul E Murphy via Libc-alpha wrote:
> Are you suggesting something like [2] would be more accurate? Nothing
It should use $(gnulib-tests) in place of -lgcc, but something like that,
yes.
diff --git a/sysdeps/powerpc/powerpc64/le/Makefile b/sysdeps/powerpc/powerpc64/le/Makefile index 480e113637..ee2b78bb5f 100644 --- a/sysdeps/powerpc/powerpc64/le/Makefile +++ b/sysdeps/powerpc/powerpc64/le/Makefile @@ -35,12 +35,29 @@ CFLAGS-test-math-iscanonical.cc += -mfloat128 CFLAGS-test-math-iseqsig.cc += -mfloat128 CFLAGS-test-math-issignaling.cc += -mfloat128 CFLAGS-test-math-iszero.cc += -mfloat128 -$(foreach test, \ - test-float128% test-ifloat128% test-float64x% test-ifloat64x% \ +$(foreach test,\ + basic-test \ + bug-nextafter \ + bug-nexttoward \ + test-fenv-clear \ + test-iszero-excess-precision \ + test-math-iscanonical \ + test-math-iseqsig \ + test-math-issignaling \ + test-math-iszero \ + test-misc \ + test-nan-overflow \ + test-nan-payload \ + test-snan \ + test-tgmath \ + test-tgmath2 \ + tst-CMPLX2 \ + test-%-ldbl-128ibm \ + test-ldouble% test-ildouble% \ + test-float128% test-ifloat128% \ + test-float64x% test-ifloat64x% \ $(foreach pair,$(f128-pairs),test-$(pair)%) \ - test-math-iscanonical test-math-iseqsig test-math-issignaling \ - test-math-iszero, \ - $(objpfx)$(test)): \ + ,$(objpfx)$(test)): \ gnulib-tests += $(f128-loader-link) CFLAGS-s_logbl-power7.c += $(type-ldouble-CFLAGS) @@ -99,11 +116,23 @@ CFLAGS-tst-strfrom-locale.c += -mfloat128 CFLAGS-strfrom-skeleton.c += -mfloat128 CFLAGS-tst-strtod-nan-sign.c += -mfloat128 CFLAGS-tst-wcstod-nan-sign.c += -mfloat128 -$(foreach test,bug-strtod bug-strtod2 bug-strtod2 tst-strtod-round \ -tst-wcstod-round tst-strtod6 tst-strrom tst-strfrom-locale \ -tst-strtod-nan-locale tst-wcstod-nan-locale \ -strfrom-skeleton tst-strtod-nan-sign tst-wcstod-nan-sign, \ -$(objpfx)$(test)): gnulib-tests += $(f128-loader-link) + +$(foreach test, \ + bug-strtod \ + bug-strtod2 \ + strfrom-skeleton \ + tst-strfrom-locale \ + tst-strrom \ + tst-strtod \ + tst-strtod-nan-locale \ + tst-strtod-nan-sign \ + tst-strtod-round \ + tst-strtod6 \ + tst-wcstod-nan-locale \ + tst-wcstod-nan-sign \ + tst-wcstod-round \ + ,$(objpfx)$(test)): \ + gnulib-tests += $(f128-loader-link) # When building glibc with support for _Float128, the powers of ten tables in # fpioconst.c and in the string conversion functions must be extended. Some @@ -124,6 +153,12 @@ ifeq ($(subdir),stdio-common) CFLAGS-printf_fp.c = -mfloat128 CFLAGS-printf_fphex.c = -mfloat128 CFLAGS-printf_size.c = -mfloat128 +$(foreach test, \ + tst-sprintf2 \ + tst-sprintf3 \ + tstdiomisc \ + ,$(objpfx)$(test)): \ + gnulib-tests += $(f128-loader-link) endif
From: "Gabriel F. T. Gomes" <gabriel@inconstante.eti.br> As noted in the following commit commit 91ac3a7d8474480685632cd25f844d3154c69fdf Author: Tulio Magno Quites Machado Filho <tuliom@linux.vnet.ibm.com> Date: Mon Jul 17 17:48:59 2017 -0300 powerpc: Fix float128 IFUNC relocations [BZ #21707] libgcc depends on a symbol, __parse_hwcap_and_convert_at_platform, provided by the loaders. This dependency reflected on the need to link some float128 tests against the loader. In preparation for the transition of the format of long double - from IBM Extended Precision to IEEE 754 128-bits floating-point - on powerpc64le, this patch adds the linking of the loader to the tests for long double, since after the switch they will also depend on __parse_hwcap_and_convert_at_platform. Tested on powerpc64le. --- sysdeps/powerpc/powerpc64/le/Makefile | 55 ++++++++++++++++++++++----- 1 file changed, 45 insertions(+), 10 deletions(-)