Add hidden visibility to internal function prototypes

Message ID 116a6dce-db6b-65f0-ad65-de22ea0aff8c@redhat.com
State Committed
Headers

Commit Message

Florian Weimer Aug. 22, 2017, 10:47 a.m. UTC
  On 08/21/2017 11:15 PM, Joseph Myers wrote:
> On Mon, 21 Aug 2017, Florian Weimer wrote:
> 
>>> The test links with libc.so, but it uses various GMP objects from libc.a.
>>>
>>> Generically, any test using any objects from libc.a will have problems if 
>>> those reference hidden symbols (in this case, __assert_fail) from 
>>> elsewhere in libc.
>>
>> I don't think linking a test both against libc.a and libc.so is valid.
>>
>> In other cases, in order to test hidden symbols, we use fully static
>> linking instead.  Is this something we could do here as well?  I don't
>> think it would invalidate the tests any more than the current hybrid
>> linkage model does.
> 
> As far as I know, linking those tests statically would be OK (it would not 
> significantly affect what they are testing).  That's atext-exp, 
> atest-sincos, atest-exp2 that are using internal GMP objects from libc.a; 
> I don't know if other tests, in math/ or elsewhere, also link against 
> particular libc.a objects.

I think the attached approach will work for the math subdirectory.
Tested on aarch64 and ppc64le.

I see additional aarch64 failures, though (outside math).

Thanks,
Florian
  

Comments

Florian Weimer Aug. 22, 2017, 11:39 a.m. UTC | #1
On 08/22/2017 12:47 PM, Florian Weimer wrote:
> I think the attached approach will work for the math subdirectory.
> Tested on aarch64 and ppc64le.
> 
> I see additional aarch64 failures, though (outside math).

The failure was spurious (probably clock skew).

I have since tried “make xcheck” and “make bench-build” on both
architectures and don't see any failures anymore with the patch I posted
(and the revert reverted).

Thanks,
Florian
  
Joseph Myers Aug. 22, 2017, 11:39 a.m. UTC | #2
On Tue, 22 Aug 2017, Florian Weimer wrote:

> > As far as I know, linking those tests statically would be OK (it would not 
> > significantly affect what they are testing).  That's atext-exp, 
> > atest-sincos, atest-exp2 that are using internal GMP objects from libc.a; 
> > I don't know if other tests, in math/ or elsewhere, also link against 
> > particular libc.a objects.
> 
> I think the attached approach will work for the math subdirectory.
> Tested on aarch64 and ppc64le.
> 
> I see additional aarch64 failures, though (outside math).

To be clear: it works for math/ on top of having reapplied HJ's patch, but 
there are still similar failures in other directories (on aarch64) in the 
presence of HJ's patch?

What are those other aarch64 failures?  Maybe we should look at the full 
set of failures in all directories with the visibility changes in order to 
figure out what approaches are good for fixing such problems.

In principle it should make sense for all symbols in the static libraries 
to be hidden (maybe build them with -fvisibility=hidden?), unless there's 
a specific reason some given symbol needs to be non-hidden for normal use 
of the installed libraries, but first we need to sort out any testsuite 
breakage that causes.
  
Florian Weimer Aug. 22, 2017, 11:43 a.m. UTC | #3
On 08/22/2017 01:39 PM, Joseph Myers wrote:
> On Tue, 22 Aug 2017, Florian Weimer wrote:
> 
>>> As far as I know, linking those tests statically would be OK (it would not 
>>> significantly affect what they are testing).  That's atext-exp, 
>>> atest-sincos, atest-exp2 that are using internal GMP objects from libc.a; 
>>> I don't know if other tests, in math/ or elsewhere, also link against 
>>> particular libc.a objects.
>>
>> I think the attached approach will work for the math subdirectory.
>> Tested on aarch64 and ppc64le.
>>
>> I see additional aarch64 failures, though (outside math).
> 
> To be clear: it works for math/ on top of having reapplied HJ's patch, but 
> there are still similar failures in other directories (on aarch64) in the 
> presence of HJ's patch?

See my other message.

Initially, I saw another build failure (with HJ's patch re-applied), but
I can't reproduce it.  I don't see any check/xcgeck/bench-build failures.

I did not check s390x, ppc, ppc64, though.  (Our lab automation is a bit
overloaded right now, so I can't get a machine reservation easily.)

But I think it should be possible to apply my patch and revert the revert.

Thanks,
Florian
  
Joseph Myers Aug. 22, 2017, 11:45 a.m. UTC | #4
On Tue, 22 Aug 2017, Florian Weimer wrote:

> On 08/22/2017 12:47 PM, Florian Weimer wrote:
> > I think the attached approach will work for the math subdirectory.
> > Tested on aarch64 and ppc64le.
> > 
> > I see additional aarch64 failures, though (outside math).
> 
> The failure was spurious (probably clock skew).
> 
> I have since tried “make xcheck” and “make bench-build” on both
> architectures and don't see any failures anymore with the patch I posted
> (and the revert reverted).

Thanks.  This patch (and restoring HJ's patch) is OK to allow continued 
progress on the visibility changes (although the alternative of using 
mini-gmp in these tests is attractive as well, in line with the general 
principle of limiting tests to using public interfaces when they have no 
real need for internal interfaces).
  

Patch

math: Statically link tests of internal functionality

2017-08-22  Florian Weimer  <fweimer@redhat.com>

	math: Statically link tests of internal functionality.
	* math/Makefile (tests): Remove atest-exp, atest-sincos,
	atest-exp2.
	(tests-static): Add atest-exp, atest-sincos, atest-exp2.
	(gmp-objs): Remove assignment.
	(atest-exp, atest-sincos, atest-exp2): Remove targets.

diff --git a/math/Makefile b/math/Makefile
index 25d3e95c6c..7948d476fa 100644
--- a/math/Makefile
+++ b/math/Makefile
@@ -181,7 +181,7 @@  $(inst_libdir)/libm.a: $(common-objpfx)format.lds \
 endif
 
 # Rules for the test suite.
-tests = test-matherr-3 test-fenv atest-exp atest-sincos atest-exp2 basic-test \
+tests = test-matherr-3 test-fenv basic-test \
 	test-misc test-fpucw test-fpucw-ieee tst-definitions test-tgmath \
 	test-tgmath-ret bug-nextafter bug-nexttoward bug-tgmath1 \
 	test-tgmath-int test-tgmath2 test-powl tst-CMPLX tst-CMPLX2 test-snan \
@@ -203,6 +203,10 @@  tests-static = test-fpucw-static test-fpucw-ieee-static \
 	       test-signgam-ullong-static test-signgam-ullong-init-static
 tests-internal = test-matherr test-matherr-2
 
+# These tests use internal (unexported) GMP functions and are linked
+# statically to obtain access to these fucntions.
+tests-static += atest-exp atest-sincos atest-exp2
+
 ifneq (,$(CXX))
 tests += test-math-isinff test-math-iszero
 endif
@@ -569,11 +573,4 @@  endef
 object-suffixes-left := $(libmvec-tests)
 include $(o-iterator)
 
-gmp-objs = $(patsubst %,$(common-objpfx)stdlib/%.o,\
-		      add_n sub_n cmp addmul_1 mul_1 mul_n divmod_1 \
-		      lshift rshift mp_clz_tab udiv_qrnnd inlines \
-		      $(gmp-sysdep_routines))
-$(objpfx)atest-exp: $(gmp-objs)
-$(objpfx)atest-sincos: $(gmp-objs)
-$(objpfx)atest-exp2: $(gmp-objs)
 $(objpfx)test-fenv-tls: $(shared-thread-library)