[PATCH] Add hidden visibility to internal function prototypes

[PATCH] Add hidden visibility to internal function prototypes

[H.J. Lu]

Add hidden visibility to internal function prototypes to allow direct
access to internal functions within libc.a without using GOT when the
compiler defaults to -fPIE.

Size comparison of elf/ldconfig when the compiler defaults to -fPIE:

On x86-64:
        text	   data	    bss	    dec	    hex
Before: 619646	  20132	   5488	 645266	  9d892
After : 619502	  20132	   5488	 645122	  9d802
On i686:
        text	   data	    bss	    dec	    hex
Before: 550333	  10748	   3060	 564141	  89bad
After : 546453	  10732	   3060	 560245	  88c75

Any comments?

H.J.
---
	* include/libc-symbols.h (__hidden_proto_hiddenattr): New for
	the compiler defaulting to -fPIE.
	(hidden_proto): Likewise.
	(hidden_tls_proto): Likewise.
	(__hidden_proto): Likewise.
---
 include/libc-symbols.h | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/include/libc-symbols.h b/include/libc-symbols.h
index d6a1c260f6..6c75a6f012 100644
--- a/include/libc-symbols.h
+++ b/include/libc-symbols.h
@@ -513,8 +513,20 @@ for linking")
 # endif
 #else
 # ifndef __ASSEMBLER__
-#  define hidden_proto(name, attrs...)
-#  define hidden_tls_proto(name, attrs...)
+#  if BUILD_PIE_DEFAULT && IS_IN (libc) && !defined LIBC_NONSHARED \
+      && !defined NO_HIDDEN
+#   define __hidden_proto_hiddenattr(attrs...) \
+  __attribute__ ((visibility ("hidden"), ##attrs))
+#   define hidden_proto(name, attrs...) \
+  __hidden_proto (name, , name, ##attrs)
+#   define hidden_tls_proto(name, attrs...) \
+  __hidden_proto (name, __thread, name, ##attrs)
+#  define __hidden_proto(name, thread, internal, attrs...)	     \
+  extern thread __typeof (name) name __hidden_proto_hiddenattr (attrs);
+# else
+#   define hidden_proto(name, attrs...)
+#   define hidden_tls_proto(name, attrs...)
+# endif
 # else
 #  define HIDDEN_JUMPTARGET(name) JUMPTARGET(name)
 # endif /* Not  __ASSEMBLER__ */
-- 
2.13.5

Re: [PATCH] Add hidden visibility to internal function prototypes

[Florian Weimer]

On 08/17/2017 02:22 PM, H.J. Lu wrote:
> Add hidden visibility to internal function prototypes to allow direct
> access to internal functions within libc.a without using GOT when the
> compiler defaults to -fPIE.
> 
> Size comparison of elf/ldconfig when the compiler defaults to -fPIE:

This is for static linking, right?

Is there a reason to restrict this to PIE builds?  Why not use hidden
visibility unconditionally?

Thanks,
Florian

Re: [PATCH] Add hidden visibility to internal function prototypes

[H.J. Lu]

On Thu, Aug 17, 2017 at 5:34 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 08/17/2017 02:22 PM, H.J. Lu wrote:
>> Add hidden visibility to internal function prototypes to allow direct
>> access to internal functions within libc.a without using GOT when the
>> compiler defaults to -fPIE.
>>
>> Size comparison of elf/ldconfig when the compiler defaults to -fPIE:
>
> This is for static linking, right?
>
> Is there a reason to restrict this to PIE builds?  Why not use hidden
> visibility unconditionally?
>

Yes, there is only used for static PIE build and we can replace
BUILD_PIE_DEFAULT with !defined SHARED.  But it makes
no difference for non-PIE static build.

-- 
H.J.

Re: [PATCH] Add hidden visibility to internal function prototypes

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 964 bytes --]

On Thu, Aug 17, 2017 at 5:40 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Aug 17, 2017 at 5:34 AM, Florian Weimer <fweimer@redhat.com> wrote:
>> On 08/17/2017 02:22 PM, H.J. Lu wrote:
>>> Add hidden visibility to internal function prototypes to allow direct
>>> access to internal functions within libc.a without using GOT when the
>>> compiler defaults to -fPIE.
>>>
>>> Size comparison of elf/ldconfig when the compiler defaults to -fPIE:
>>
>> This is for static linking, right?
>>
>> Is there a reason to restrict this to PIE builds?  Why not use hidden
>> visibility unconditionally?
>>
>
> Yes, there is only used for static PIE build and we can replace
> BUILD_PIE_DEFAULT with !defined SHARED.  But it makes
> no difference for non-PIE static build.
>

Here is the updated patch to add hidden visibility to internal function
prototypes used within libc.a.

Tested on x86-64 and i686 with and without GCC defaulting to PIE.

OK for master?


-- 
H.J.

[-- Attachment #2: 0001-Add-hidden-visibility-to-internal-function-prototype.patch --]
[-- Type: text/x-patch, Size: 2070 bytes --]

From 236f3b88f74d408d3ab925809da3bd6d8a995730 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Sun, 13 Aug 2017 07:37:34 -0700
Subject: [PATCH] Add hidden visibility to internal function prototypes

Add hidden visibility to internal function prototypes to allow direct
access to internal functions within libc.a without using GOT when the
compiler defaults to -fPIE.

Size comparison of elf/ldconfig when the compiler defaults to -fPIE:

On x86-64:
        text	   data	    bss	    dec	    hex
Before: 619646	  20132	   5488	 645266	  9d892
After : 619502	  20132	   5488	 645122	  9d802
On i686:
        text	   data	    bss	    dec	    hex
Before: 550333	  10748	   3060	 564141	  89bad
After : 546453	  10732	   3060	 560245	  88c75

	* include/libc-symbols.h (__hidden_proto_hiddenattr): New for
	building libc.a.
	(hidden_proto): Likewise.
	(hidden_tls_proto): Likewise.
	(__hidden_proto): Likewise.
---
 include/libc-symbols.h | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/include/libc-symbols.h b/include/libc-symbols.h
index d6a1c260f6..fe3571af52 100644
--- a/include/libc-symbols.h
+++ b/include/libc-symbols.h
@@ -513,8 +513,20 @@ for linking")
 # endif
 #else
 # ifndef __ASSEMBLER__
-#  define hidden_proto(name, attrs...)
-#  define hidden_tls_proto(name, attrs...)
+#  if !defined SHARED && IS_IN (libc) && !defined LIBC_NONSHARED \
+      && !defined NO_HIDDEN
+#   define __hidden_proto_hiddenattr(attrs...) \
+  __attribute__ ((visibility ("hidden"), ##attrs))
+#   define hidden_proto(name, attrs...) \
+  __hidden_proto (name, , name, ##attrs)
+#   define hidden_tls_proto(name, attrs...) \
+  __hidden_proto (name, __thread, name, ##attrs)
+#  define __hidden_proto(name, thread, internal, attrs...)	     \
+  extern thread __typeof (name) name __hidden_proto_hiddenattr (attrs);
+# else
+#   define hidden_proto(name, attrs...)
+#   define hidden_tls_proto(name, attrs...)
+# endif
 # else
 #  define HIDDEN_JUMPTARGET(name) JUMPTARGET(name)
 # endif /* Not  __ASSEMBLER__ */
-- 
2.13.5

Re: [PATCH] Add hidden visibility to internal function prototypes

[H.J. Lu]

On Thu, Aug 17, 2017 at 11:25 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Aug 17, 2017 at 5:40 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Thu, Aug 17, 2017 at 5:34 AM, Florian Weimer <fweimer@redhat.com> wrote:
>>> On 08/17/2017 02:22 PM, H.J. Lu wrote:
>>>> Add hidden visibility to internal function prototypes to allow direct
>>>> access to internal functions within libc.a without using GOT when the
>>>> compiler defaults to -fPIE.
>>>>
>>>> Size comparison of elf/ldconfig when the compiler defaults to -fPIE:
>>>
>>> This is for static linking, right?
>>>
>>> Is there a reason to restrict this to PIE builds?  Why not use hidden
>>> visibility unconditionally?
>>>
>>
>> Yes, there is only used for static PIE build and we can replace
>> BUILD_PIE_DEFAULT with !defined SHARED.  But it makes
>> no difference for non-PIE static build.
>>
>
> Here is the updated patch to add hidden visibility to internal function
> prototypes used within libc.a.
>
> Tested on x86-64 and i686 with and without GCC defaulting to PIE.
>
> OK for master?
>

I will check it in tomorrow.


-- 
H.J.

Re: [PATCH] Add hidden visibility to internal function prototypes

[Joseph Myers]

The commit

commit 568ff4296c72534e49c8d9c83c33f3a0069cccc7
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Mon Aug 21 05:50:38 2017 -0700

    Add hidden visibility to internal function prototypes

breaks the build of the testsuite for many platforms:

https://sourceware.org/ml/libc-testresults/2017-q3/msg00300.html

The errors are of the following form, building math/atest-exp:

[...]
/scratch/jmyers/glibc-bot/build/glibcs/aarch64-linux-gnu/glibc/stdlib/rshift.o: In function `__mpn_rshift':
/scratch/jmyers/glibc-bot/build/glibcs/aarch64-linux-gnu/glibc-src/stdlib/rshift.c:45: undefined reference to `__assert_fail'
/scratch/jmyers/glibc-bot/install/compilers/aarch64-linux-gnu/lib/gcc/aarch64-glibc-linux-gnu/7.2.1/../../../../aarch64-glibc-linux-gnu/bin/ld: /scratch/jmyers/glibc-bot/build/glibcs/aarch64-linux-gnu/glibc/math/atest-exp: hidden symbol `__assert_fail' isn't defined
/scratch/jmyers/glibc-bot/install/compilers/aarch64-linux-gnu/lib/gcc/aarch64-glibc-linux-gnu/7.2.1/../../../../aarch64-glibc-linux-gnu/bin/ld: final link failed: Bad value

This test is using various objects from the build of libc.  Some of
those objects contain references to __assert_fail.  Because those
references are hidden, they cannot refer to __assert_fail from
libc.so.  Given the tests using internal objects those symbols in
libc.a can't safely be made hidden, so this patch reverts the problem
commit until any alternative approach that doesn't break the build can
be found.

Committed (having tested on aarch64 with build-many-glibcs.py that this 
fixes the build).

diff --git a/ChangeLog b/ChangeLog
index b0f3a17..0e0ab13 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,14 @@
 2017-08-21  Joseph Myers  <joseph@codesourcery.com>
 
+	Revert:
+	2017-08-21  H.J. Lu  <hongjiu.lu@intel.com>
+
+	* include/libc-symbols.h (__hidden_proto_hiddenattr): New for
+	building libc.a.
+	(hidden_proto): Likewise.
+	(hidden_tls_proto): Likewise.
+	(__hidden_proto): Likewise.
+
 	[BZ #21973]
 	* sysdeps/sparc/sparc32/fpu/w_sqrt_compat.S: Remove file.
 	* sysdeps/sparc/sparc32/fpu/w_sqrtf_compat.S: Likewise.
diff --git a/include/libc-symbols.h b/include/libc-symbols.h
index fe3571a..d6a1c26 100644
--- a/include/libc-symbols.h
+++ b/include/libc-symbols.h
@@ -513,20 +513,8 @@ for linking")
 # endif
 #else
 # ifndef __ASSEMBLER__
-#  if !defined SHARED && IS_IN (libc) && !defined LIBC_NONSHARED \
-      && !defined NO_HIDDEN
-#   define __hidden_proto_hiddenattr(attrs...) \
-  __attribute__ ((visibility ("hidden"), ##attrs))
-#   define hidden_proto(name, attrs...) \
-  __hidden_proto (name, , name, ##attrs)
-#   define hidden_tls_proto(name, attrs...) \
-  __hidden_proto (name, __thread, name, ##attrs)
-#  define __hidden_proto(name, thread, internal, attrs...)	     \
-  extern thread __typeof (name) name __hidden_proto_hiddenattr (attrs);
-# else
-#   define hidden_proto(name, attrs...)
-#   define hidden_tls_proto(name, attrs...)
-# endif
+#  define hidden_proto(name, attrs...)
+#  define hidden_tls_proto(name, attrs...)
 # else
 #  define HIDDEN_JUMPTARGET(name) JUMPTARGET(name)
 # endif /* Not  __ASSEMBLER__ */

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Add hidden visibility to internal function prototypes

[H.J. Lu]

On Mon, Aug 21, 2017 at 10:59 AM, Joseph Myers <joseph@codesourcery.com> wrote:
> The commit
>
> commit 568ff4296c72534e49c8d9c83c33f3a0069cccc7
> Author: H.J. Lu <hjl.tools@gmail.com>
> Date:   Mon Aug 21 05:50:38 2017 -0700
>
>     Add hidden visibility to internal function prototypes
>
> breaks the build of the testsuite for many platforms:
>
> https://sourceware.org/ml/libc-testresults/2017-q3/msg00300.html
>
> The errors are of the following form, building math/atest-exp:
>
> [...]
> /scratch/jmyers/glibc-bot/build/glibcs/aarch64-linux-gnu/glibc/stdlib/rshift.o: In function `__mpn_rshift':
> /scratch/jmyers/glibc-bot/build/glibcs/aarch64-linux-gnu/glibc-src/stdlib/rshift.c:45: undefined reference to `__assert_fail'
> /scratch/jmyers/glibc-bot/install/compilers/aarch64-linux-gnu/lib/gcc/aarch64-glibc-linux-gnu/7.2.1/../../../../aarch64-glibc-linux-gnu/bin/ld: /scratch/jmyers/glibc-bot/build/glibcs/aarch64-linux-gnu/glibc/math/atest-exp: hidden symbol `__assert_fail' isn't defined
> /scratch/jmyers/glibc-bot/install/compilers/aarch64-linux-gnu/lib/gcc/aarch64-glibc-linux-gnu/7.2.1/../../../../aarch64-glibc-linux-gnu/bin/ld: final link failed: Bad value
>
> This test is using various objects from the build of libc.  Some of
> those objects contain references to __assert_fail.  Because those
> references are hidden, they cannot refer to __assert_fail from
> libc.so.  Given the tests using internal objects those symbols in
> libc.a can't safely be made hidden, so this patch reverts the problem
> commit until any alternative approach that doesn't break the build can
> be found.

Can't these link against libc.so instead of libc.a?

> Committed (having tested on aarch64 with build-many-glibcs.py that this
> fixes the build).
>
> diff --git a/ChangeLog b/ChangeLog
> index b0f3a17..0e0ab13 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,5 +1,14 @@
>  2017-08-21  Joseph Myers  <joseph@codesourcery.com>
>
> +       Revert:
> +       2017-08-21  H.J. Lu  <hongjiu.lu@intel.com>
> +
> +       * include/libc-symbols.h (__hidden_proto_hiddenattr): New for
> +       building libc.a.
> +       (hidden_proto): Likewise.
> +       (hidden_tls_proto): Likewise.
> +       (__hidden_proto): Likewise.
> +
>         [BZ #21973]
>         * sysdeps/sparc/sparc32/fpu/w_sqrt_compat.S: Remove file.
>         * sysdeps/sparc/sparc32/fpu/w_sqrtf_compat.S: Likewise.
> diff --git a/include/libc-symbols.h b/include/libc-symbols.h
> index fe3571a..d6a1c26 100644
> --- a/include/libc-symbols.h
> +++ b/include/libc-symbols.h
> @@ -513,20 +513,8 @@ for linking")
>  # endif
>  #else
>  # ifndef __ASSEMBLER__
> -#  if !defined SHARED && IS_IN (libc) && !defined LIBC_NONSHARED \
> -      && !defined NO_HIDDEN
> -#   define __hidden_proto_hiddenattr(attrs...) \
> -  __attribute__ ((visibility ("hidden"), ##attrs))
> -#   define hidden_proto(name, attrs...) \
> -  __hidden_proto (name, , name, ##attrs)
> -#   define hidden_tls_proto(name, attrs...) \
> -  __hidden_proto (name, __thread, name, ##attrs)
> -#  define __hidden_proto(name, thread, internal, attrs...)          \
> -  extern thread __typeof (name) name __hidden_proto_hiddenattr (attrs);
> -# else
> -#   define hidden_proto(name, attrs...)
> -#   define hidden_tls_proto(name, attrs...)
> -# endif
> +#  define hidden_proto(name, attrs...)
> +#  define hidden_tls_proto(name, attrs...)
>  # else
>  #  define HIDDEN_JUMPTARGET(name) JUMPTARGET(name)
>  # endif /* Not  __ASSEMBLER__ */
>
> --
> Joseph S. Myers
> joseph@codesourcery.com



-- 
H.J.

Re: [PATCH] Add hidden visibility to internal function prototypes

[Joseph Myers]

On Mon, 21 Aug 2017, H.J. Lu wrote:

> > This test is using various objects from the build of libc.  Some of
> > those objects contain references to __assert_fail.  Because those
> > references are hidden, they cannot refer to __assert_fail from
> > libc.so.  Given the tests using internal objects those symbols in
> > libc.a can't safely be made hidden, so this patch reverts the problem
> > commit until any alternative approach that doesn't break the build can
> > be found.
> 
> Can't these link against libc.so instead of libc.a?

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.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Add hidden visibility to internal function prototypes

[Florian Weimer]

On 08/21/2017 08:11 PM, Joseph Myers wrote:
> On Mon, 21 Aug 2017, H.J. Lu wrote:
> 
>>> This test is using various objects from the build of libc.  Some of
>>> those objects contain references to __assert_fail.  Because those
>>> references are hidden, they cannot refer to __assert_fail from
>>> libc.so.  Given the tests using internal objects those symbols in
>>> libc.a can't safely be made hidden, so this patch reverts the problem
>>> commit until any alternative approach that doesn't break the build can
>>> be found.
>>
>> Can't these link against libc.so instead of libc.a?
> 
> 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.

Thanks,
Florian

Re: [PATCH] Add hidden visibility to internal function prototypes

[Joseph Myers]

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.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Add hidden visibility to internal function prototypes

[H.J. Lu]

On Mon, Aug 21, 2017 at 2:15 PM, Joseph Myers <joseph@codesourcery.com> 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;

Linking against both libc.a and libc.so isn't supported since many data
structures are defined in different places in libc.a and libc.so.

> I don't know if other tests, in math/ or elsewhere, also link against
> particular libc.a objects.
>

I  am testing a patch to provide a simple __assert_fail implementation for
these math tests.

Personally, I believe these tests are wrong.  if we need to access the
internal interfaces of libc, we should export them as private interface,
not by abusing libc.a and libc.so.


-- 
H.J.

Re: [PATCH] Add hidden visibility to internal function prototypes

[Joseph Myers]

On Mon, 21 Aug 2017, H.J. Lu wrote:

> > I don't know if other tests, in math/ or elsewhere, also link against
> > particular libc.a objects.
> 
> I  am testing a patch to provide a simple __assert_fail implementation for
> these math tests.

That seems like a workaround rather than a proper fix for this issue.

> Personally, I believe these tests are wrong.  if we need to access the
> internal interfaces of libc, we should export them as private interface,
> not by abusing libc.a and libc.so.

The point isn't to access internal interfaces of libc.  It's to access 
multiple-precision functionality that happens to be present in libc, in 
order to use it for a different purpose (to cross-check libm function 
implementations).  I don't think we should export interfaces only used by 
tests; GLIBC_PRIVATE should be for interfaces used by other shared 
libraries.

Now, it would be reasonable to build those GMP files again (configured as 
in-testsuite not in-libc) just for use in those tests.  That might be the 
cleanest approach for avoiding the problem, but it might also run into 
other problems if building those files as in-testsuite doesn't actually 
work because they depend on internal details of headers that are disabled 
for the in-testsuite case.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Add hidden visibility to internal function prototypes

[H.J. Lu]

On Mon, Aug 21, 2017 at 2:40 PM, Joseph Myers <joseph@codesourcery.com> wrote:
> On Mon, 21 Aug 2017, H.J. Lu wrote:
>
>> > I don't know if other tests, in math/ or elsewhere, also link against
>> > particular libc.a objects.
>>
>> I  am testing a patch to provide a simple __assert_fail implementation for
>> these math tests.
>
> That seems like a workaround rather than a proper fix for this issue.

It is a workaround for the issues in those tests so that the valid optimization
in libc can enabled.

>> Personally, I believe these tests are wrong.  if we need to access the
>> internal interfaces of libc, we should export them as private interface,
>> not by abusing libc.a and libc.so.
>
> The point isn't to access internal interfaces of libc.  It's to access
> multiple-precision functionality that happens to be present in libc, in
> order to use it for a different purpose (to cross-check libm function
> implementations).  I don't think we should export interfaces only used by
> tests; GLIBC_PRIVATE should be for interfaces used by other shared
> libraries.
>
> Now, it would be reasonable to build those GMP files again (configured as
> in-testsuite not in-libc) just for use in those tests.  That might be the
> cleanest approach for avoiding the problem, but it might also run into
> other problems if building those files as in-testsuite doesn't actually
> work because they depend on internal details of headers that are disabled
> for the in-testsuite case.
>

Why not require and use the real gmp library for those tests?

-- 
H.J.

Re: [PATCH] Add hidden visibility to internal function prototypes

[Joseph Myers]

On Mon, 21 Aug 2017, H.J. Lu wrote:

> > Now, it would be reasonable to build those GMP files again (configured as
> > in-testsuite not in-libc) just for use in those tests.  That might be the
> > cleanest approach for avoiding the problem, but it might also run into
> > other problems if building those files as in-testsuite doesn't actually
> > work because they depend on internal details of headers that are disabled
> > for the in-testsuite case.
> 
> Why not require and use the real gmp library for those tests?

Requiring additional libraries for the target excessively complicates 
testing.  GMP is a particular pain in this regard given how it may 
sometimes try to choose an ABI that's not the one the compiler defaults 
to, or have problems with more unusual ABIs such as get included in glibc 
testing.

On the other hand, copying mini-gmp.c and mini-gmp.h from the GMP sources 
into glibc, and using those unmodified in the tests, may well be a 
reasonable approach if it works: mini-gmp is designed to be copied like 
that, and avoids all the complications around ABI choice and assembly 
sources.  That way you'd avoid additional dependencies on the target.  But 
we should definitely avoid local changes to mini-gmp; inclusion of 
mini-gmp would mean adding it to the list at 
https://sourceware.org/glibc/wiki/Regeneration and including it in the 
list in scripts/update-copyrights of files that don't get copyright dates 
updated because of being imported verbatim from elsewhere.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Add hidden visibility to internal function prototypes

[Florian Weimer]

[-- Attachment #1: Type: text/plain, Size: 1170 bytes --]

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

[-- Attachment #2: math-static-tests.patch --]
[-- Type: text/x-patch, Size: 1823 bytes --]

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)

Re: [PATCH] Add hidden visibility to internal function prototypes

[Florian Weimer]

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

Re: [PATCH] Add hidden visibility to internal function prototypes

[Joseph Myers]

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.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Add hidden visibility to internal function prototypes

[Florian Weimer]

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

Re: [PATCH] Add hidden visibility to internal function prototypes

[Joseph Myers]

[-- Attachment #1: Type: text/plain, Size: 883 bytes --]

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).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Add hidden visibility to internal function prototypes

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 1648 bytes --]

On Tue, Aug 22, 2017 at 4:45 AM, Joseph Myers <joseph@codesourcery.com> wrote:
> 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).

I tried mini-gmp from gmp 6.1.2.  Some GMP functions are missing from
mini-gmp.  It seems that these math tests depend on the GMP in glibc.

I didn't see the issue on x86 because

./sysdeps/x86_64/rshift.S
./sysdeps/powerpc/powerpc32/rshift.S
./sysdeps/m68k/m680x0/rshift.S
./sysdeps/hppa/rshift.S
./sysdeps/i386/rshift.S
./sysdeps/i386/i586/rshift.S
./sysdeps/sparc/sparc32/rshift.S
./sysdeps/sparc/sparc64/rshift.S
./sysdeps/mips/rshift.S
./sysdeps/mips/mips64/rshift.S
./sysdeps/alpha/rshift.S
./sysdeps/alpha/alphaev5/rshift.S

Only targets which use stdlib/rshift.c have this issue.  This patch
copies the debug approach from stdlib/lshift.c and works on aarch64,

-- 
H.J.

[-- Attachment #2: 0001-rshift.c-Replace-assert-with-DEBUG-and-abort.patch --]
[-- Type: text/x-patch, Size: 1091 bytes --]

From 3478fe3a92a8c85fa342a38ef3bf9bcc704bea94 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Mon, 21 Aug 2017 17:07:36 -0700
Subject: [PATCH] rshift.c: Replace assert with DEBUG and abort

assert in stdlib/rshift.c should be for debug purpose only and there is
no such check in any rshift assembly implementations nor in lshift.c.
This patch replaces assert with DEBUG and abort, similar to lshift.c
so that generic GMP codes from libc.a can be linked with libc.so in
atest-exp, atest-exp2 and atest-sincos, which depend on the GMP
implementation in glibc.

	* stdlib/rshift.c (mpn_rshift): Replace ssert with DEBUG and
	abort.
---
 stdlib/rshift.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/stdlib/rshift.c b/stdlib/rshift.c
index d4c7f77769..ab69dd7915 100644
--- a/stdlib/rshift.c
+++ b/stdlib/rshift.c
@@ -42,7 +42,10 @@ mpn_rshift (register mp_ptr wp,
   register mp_size_t i;
   mp_limb_t retval;
 
-  assert (usize != 0 && cnt != 0);
+#ifdef DEBUG
+  if (usize == 0 || cnt == 0)
+    abort ();
+#endif
 
   sh_1 = cnt;
 
-- 
2.13.5

Re: [PATCH] Add hidden visibility to internal function prototypes

[Florian Weimer]

On 08/22/2017 02:16 PM, H.J. Lu wrote:
> Only targets which use stdlib/rshift.c have this issue.  This patch
> copies the debug approach from stdlib/lshift.c and works on aarch64,

I think the existing assert better reflects the actual intent here.  We
should use assert in stdlib/lshift.c as well.

With staticaly linked tests, the use of __assert_fail is a non-issue.

Thanks,
Florian