Message ID | alpine.DEB.2.20.1708211757500.3404@digraph.polyomino.org.uk |
---|---|
State | New, archived |
Headers |
Received: (qmail 67582 invoked by alias); 21 Aug 2017 17:59:38 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 67246 invoked by uid 89); 21 Aug 2017 17:59:37 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.4 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS, URIBL_RED autolearn=ham version=3.3.2 spammy= X-HELO: relay1.mentorg.com Date: Mon, 21 Aug 2017 17:59:27 +0000 From: Joseph Myers <joseph@codesourcery.com> To: "H.J. Lu" <hjl.tools@gmail.com> CC: Florian Weimer <fweimer@redhat.com>, GNU C Library <libc-alpha@sourceware.org> Subject: Re: [PATCH] Add hidden visibility to internal function prototypes In-Reply-To: <CAMe9rOpOQuMhtz4-UCp21OaCtreob7GQkr92S6Zg3uunf1vUAw@mail.gmail.com> Message-ID: <alpine.DEB.2.20.1708211757500.3404@digraph.polyomino.org.uk> References: <20170817122244.GA14297@gmail.com> <1cb1b2db-db57-85dd-02ff-a7a1371bcee3@redhat.com> <CAMe9rOrHQ7_4DPSLv-B73qOMOXcoTY+KTQGftBCTStSnQnSj5Q@mail.gmail.com> <CAMe9rOqtXP92qNahY_S8eu8a7jX35asu2Bzhxpd9TXBT9gZbyQ@mail.gmail.com> <CAMe9rOpOQuMhtz4-UCp21OaCtreob7GQkr92S6Zg3uunf1vUAw@mail.gmail.com> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" X-ClientProxiedBy: svr-ies-mbx-01.mgc.mentorg.com (139.181.222.1) To svr-ies-mbx-01.mgc.mentorg.com (139.181.222.1) |
Commit Message
Joseph Myers
Aug. 21, 2017, 5:59 p.m. UTC
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).
Comments
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
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.
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
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.
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.
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.
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?
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.
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__ */