[05/13] powerpc64le: link tests against ld.so

Message ID 20200306203721.15886-6-murphyp@linux.vnet.ibm.com
State New
Headers show
Series
  • Enable IEEE binary128 long double on powerpc64le
Related show

Commit Message

Paul E. Murphy March 6, 2020, 8:37 p.m.
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(-)

-- 
2.21.1

Comments

Joseph Myers March 7, 2020, 12:31 a.m. | #1
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.

-- 
Joseph S. Myers
joseph@codesourcery.com
Paul E Murphy March 9, 2020, 10:38 p.m. | #2
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.
Joseph Myers March 9, 2020, 11:19 p.m. | #3
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?

-- 
Joseph S. Myers
joseph@codesourcery.com
Adhemerval Zanella via Libc-alpha March 10, 2020, 9:44 p.m. | #4
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.
Joseph Myers March 10, 2020, 11:06 p.m. | #5
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.

-- 
Joseph S. Myers
joseph@codesourcery.com

Patch

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