Message ID | 6157828e-4b05-5261-ea8a-e3a531697d4e@panix.com |
---|---|
State | Superseded |
Headers |
Received: (qmail 48989 invoked by alias); 27 Feb 2017 13:34:59 -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 48973 invoked by uid 89); 27 Feb 2017 13:34:58 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-10.4 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, RCVD_IN_SORBS_SPAM, SPF_PASS autolearn=ham version=3.3.2 spammy=testsinternal, tests-internal, adhoc, explaining X-HELO: mail-qk0-f178.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=xROTqr7BJAbKR3cpqSVkXzRR5NP41OPxNFHUz8ZeSfY=; b=Jvf2UiUpQxKwm73OMcjirqIfUnWi/7N2YkfqqAVa9FgWS20AsLJFqEdEyTi8UXcfgH Y/+8HDNFTnJ40yx3sjfQqdzhzcYPXOLMLtPdqIQ2EvtX+qbX+SLB/4/CSez+lTUoqNvI uB6095pOWjb32B+8n/OM1HQbtBvuWtBtE68PKji2CIuGgJRpPRNXGwjPksIMPHr4CxMc oOh0qgEg6gVGAy4u9D6hX6cTT8oPRveKVylmnZkS4HgWN5stpGqEsjDhJ/a+0lQGRyHv Wa5LPPaqPo7ZEgYewCYhQprLR4y+3f5DZd1MSglECEKd0TLAR+eOxERdILjRzY6zv2sA 8K5Q== X-Gm-Message-State: AMke39nZS8vegVWnaSWKa5hV/HZQP1h6Cx36wkIm+RSeWP2e9ZtssgrDCafABkytTKOOCg== X-Received: by 10.55.210.135 with SMTP id f129mr14243088qkj.184.1488202495680; Mon, 27 Feb 2017 05:34:55 -0800 (PST) Subject: Re: [PATCH 4/4] Add IS_IN (testsuite) and remaining fixes. To: Carlos O'Donell <carlos@redhat.com>, libc-alpha@sourceware.org References: <20170220130342.6373-1-zackw@panix.com> <20170220130342.6373-2-zackw@panix.com> <20170220130342.6373-3-zackw@panix.com> <20170220130342.6373-4-zackw@panix.com> <20170220130342.6373-5-zackw@panix.com> <402fa86b-5738-1749-a515-2e0e6c055bdd@redhat.com> Cc: joseph@codesourcery.com, adhemerval.zanella@linaro.org From: Zack Weinberg <zackw@panix.com> Message-ID: <6157828e-4b05-5261-ea8a-e3a531697d4e@panix.com> Date: Mon, 27 Feb 2017 08:34:53 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.6.0 MIME-Version: 1.0 In-Reply-To: <402fa86b-5738-1749-a515-2e0e6c055bdd@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit |
Commit Message
Zack Weinberg
Feb. 27, 2017, 1:34 p.m. UTC
On 02/20/2017 10:13 AM, Carlos O'Donell wrote: > On 02/20/2017 08:03 AM, Zack Weinberg wrote: >> This is the main change, adding a new build module called 'testsuite'. >> IS_IN (testsuite) implies _ISOMAC, as do IS_IN_build and __cplusplus >> (which means several ad-hoc tests for __cplusplus can go away). >> libc-symbols.h now suppresses almost all of *itself* when _ISOMAC is >> defined; in particular, _ISOMAC mode does not get config.h >> automatically anymore. I'm going through your low-level comments now; here are a few notes. I'll post a revised patch later today, or tomorrow. > - Fix tst-dladdr by removing DL_LOOKUP_ADDRESS usage and keeping it in > tests instead of tests-internal. I think this test should be as close > to a real application as possible. Just to be sure that I understand the change you have in mind here: is this right? printf ("info.dli_fbase = %p\n", info.dli_fbase); > - Please carry out a built artifact comparison to ensure the IS_IN > changes did not change any code generation in the library. Minimally > x86_64 and one other architecture of your choice. Will do. > - The include/stdio.h change needs a detailed comment about why we undef > _IO_MTSAFE_IO. Eegh, that's complicated. Rather than add a comment, I am going to simplify the situation. _IO_MTSAFE_IO is only ever defined during the build of glibc itself, and it does not change the public API or ABI. There are two uses in libio.h, which is a public header. One changes the definitions of a handful of internal-use-only _IO_* macros. The other controls whether libio.h defines _IO_lock_t itself (as an incomplete type) or leaves it to stdio-lock.h (which is a non-installed header). Unfortunately, some versions of stdio-lock.h can only define _IO_lock_t as a typedef, so we have to have the ability for libio.h not to do it at all. What I'm going to do is remove the internal-use-only _IO_* macros to include/libio.h, and invent a new macro _IO_lock_t_defined which all versions of stdio-lock.h will define; libio.h will define the stub _IO_lock_t if that macro is not defined, with a comment explaining that this is only relevant when building glibc itself. Then there will be no uses of _IO_MTSAFE_IO in public headers, and it won't be necessary to undefine it in include/stdio.h. >> + $(tests) $(tests-internal) $(xtests) $(test-srcs))): \ >> + $(objpfx)libpthread.so \ >> + $(objpfx)libpthread_nonshared.a > > Why doesn't this use $(shared-thread-library)? It was that way when I got here, and I don't actually see any code that *sets* $(shared-thread-library) anywhere in the Makefiles, so I can't confirm that it'd be the same thing. > - Why is tst-cancel-getpwuid_r in tests-internal? It was designed to be > a standalone cancellation test. It calls __nss_configure_lookup. I didn't look very hard when I saw that - I see now that nss.h does count as a public header, so I'll change it back. > - New test stdlib/tst-strtod1i.c should use new support/ test infrastructure. > > - New test stdlib/tst-strtod5i.c needs a copyright header and should use > new support/ test infrastructure. Will do. I am also going to make those changes to the tests they were cloned from (tst-strtod.c and tst-strtod5.c). zw
Comments
On 02/27/2017 08:34 AM, Zack Weinberg wrote: > On 02/20/2017 10:13 AM, Carlos O'Donell wrote: >> On 02/20/2017 08:03 AM, Zack Weinberg wrote: >>> This is the main change, adding a new build module called 'testsuite'. >>> IS_IN (testsuite) implies _ISOMAC, as do IS_IN_build and __cplusplus >>> (which means several ad-hoc tests for __cplusplus can go away). >>> libc-symbols.h now suppresses almost all of *itself* when _ISOMAC is >>> defined; in particular, _ISOMAC mode does not get config.h >>> automatically anymore. > > I'm going through your low-level comments now; here are a few notes. > I'll post a revised patch later today, or tomorrow. > >> - Fix tst-dladdr by removing DL_LOOKUP_ADDRESS usage and keeping it in >> tests instead of tests-internal. I think this test should be as close >> to a real application as possible. > > Just to be sure that I understand the change you have in mind here: is > this right? > > --- a/dlfcn/tst-dladdr.c > +++ b/dlfcn/tst-dladdr.c > @@ -24,8 +24,6 @@ > #include <stdlib.h> > #include <string.h> > > -#include <ldsodefs.h> > - > > #define TEST_FUNCTION do_test () > extern int do_test (void); > @@ -53,8 +51,6 @@ do_test (void) > if (ret == 0) > error (EXIT_FAILURE, 0, "dladdr failed"); > > - printf ("address of ref1 = %lx\n", > - (unsigned long int) DL_LOOKUP_ADDRESS (sym)); > printf ("ret = %d\n", ret); > printf ("info.dli_fname = %p (\"%s\")\n", info.dli_fname, > info.dli_fname); > printf ("info.dli_fbase = %p\n", info.dli_fbase); Yes. Exactly. > >> - Please carry out a built artifact comparison to ensure the IS_IN >> changes did not change any code generation in the library. Minimally >> x86_64 and one other architecture of your choice. > > Will do. OK. > - The include/stdio.h change needs a detailed comment about why we undef >> _IO_MTSAFE_IO. > > Eegh, that's complicated. Rather than add a comment, I am going to > simplify the situation. > > _IO_MTSAFE_IO is only ever defined during the build of glibc itself, and > it does not change the public API or ABI. There are two uses in > libio.h, which is a public header. One changes the definitions of a > handful of internal-use-only _IO_* macros. The other controls whether > libio.h defines _IO_lock_t itself (as an incomplete type) or leaves it > to stdio-lock.h (which is a non-installed header). Unfortunately, some > versions of stdio-lock.h can only define _IO_lock_t as a typedef, so we > have to have the ability for libio.h not to do it at all. > > What I'm going to do is remove the internal-use-only _IO_* macros to > include/libio.h, and invent a new macro _IO_lock_t_defined which all > versions of stdio-lock.h will define; libio.h will define the stub > _IO_lock_t if that macro is not defined, with a comment explaining that > this is only relevant when building glibc itself. Then there will be no > uses of _IO_MTSAFE_IO in public headers, and it won't be necessary to > undefine it in include/stdio.h. OK, make sure we don't break old libstdc++ here, which I vaguely remember was tied into this macro and stdio-lock.h. >>> + $(tests) $(tests-internal) $(xtests) $(test-srcs))): \ >>> + $(objpfx)libpthread.so \ >>> + $(objpfx)libpthread_nonshared.a >> >> Why doesn't this use $(shared-thread-library)? > > It was that way when I got here, and I don't actually see any code that > *sets* $(shared-thread-library) anywhere in the Makefiles, so I can't > confirm that it'd be the same thing. sysdeps/nptl/Makeconfig: have-thread-library = yes shared-thread-library = $(common-objpfx)nptl/libpthread_nonshared.a \ $(common-objpfx)nptl/libpthread.so static-thread-library = $(common-objpfx)nptl/libpthread.a rpath-dirs += nptl >> - Why is tst-cancel-getpwuid_r in tests-internal? It was designed to be >> a standalone cancellation test. > > It calls __nss_configure_lookup. I didn't look very hard when I saw > that - I see now that nss.h does count as a public header, so I'll > change it back. Right, it's in the implementation namespace but it's actually a public API that special programs use to get around bootstrap issues. >> - New test stdlib/tst-strtod1i.c should use new support/ test infrastructure. >> >> - New test stdlib/tst-strtod5i.c needs a copyright header and should use >> new support/ test infrastructure. > > Will do. I am also going to make those changes to the tests they were > cloned from (tst-strtod.c and tst-strtod5.c). Thanks for that too.
On Wed, Mar 1, 2017 at 1:17 PM, Carlos O'Donell <carlos@redhat.com> wrote: > On 02/27/2017 08:34 AM, Zack Weinberg wrote: >> >> I'm going through your low-level comments now; here are a few notes. >> I'll post a revised patch later today, or tomorrow. FYI, a lot of the changes you requested got reshuffled into the preparation patchset I posted this morning. I have revised the core change as well but I'm not going to post it until I do the built artifact comparison, which I may not have time for till the weekend. >> What I'm going to do is remove the internal-use-only _IO_* macros to >> include/libio.h, and invent a new macro _IO_lock_t_defined which all >> versions of stdio-lock.h will define; libio.h will define the stub >> _IO_lock_t if that macro is not defined, with a comment explaining that >> this is only relevant when building glibc itself. Then there will be no >> uses of _IO_MTSAFE_IO in public headers, and it won't be necessary to >> undefine it in include/stdio.h. > > OK, make sure we don't break old libstdc++ here, which I vaguely remember > was tied into this macro and stdio-lock.h. I'll do some archaeology and find out for sure, but I _think_ that has not been relevant since before libstdc++-v3 (so we're going all the way back to the EGCS period). Moreover, we do not install stdio-lock.h; any external software that attempts to define _IO_MTSAFE_IO without supplying a definition of _IO_lock_t will fail to compile, and do we really want to support software that provides its own definition of _IO_lock_t? >>>> + $(tests) $(tests-internal) $(xtests) $(test-srcs))): \ >>>> + $(objpfx)libpthread.so \ >>>> + $(objpfx)libpthread_nonshared.a >>> >>> Why doesn't this use $(shared-thread-library)? >> >> It was that way when I got here, and I don't actually see any code that >> *sets* $(shared-thread-library) anywhere in the Makefiles, so I can't >> confirm that it'd be the same thing. > > sysdeps/nptl/Makeconfig: Oh ghod, you mean to tell me subdirectories can have Makeconfig fragments too? > shared-thread-library = $(common-objpfx)nptl/libpthread_nonshared.a \ > $(common-objpfx)nptl/libpthread.so That's in the opposite order from the opencoded sequence in nptl/Makefile. Are we sure it doesn't matter? zw
On 03/01/2017 01:48 PM, Zack Weinberg wrote: > On Wed, Mar 1, 2017 at 1:17 PM, Carlos O'Donell <carlos@redhat.com> wrote: >> On 02/27/2017 08:34 AM, Zack Weinberg wrote: >>> >>> I'm going through your low-level comments now; here are a few notes. >>> I'll post a revised patch later today, or tomorrow. > > FYI, a lot of the changes you requested got reshuffled into the > preparation patchset I posted this morning. I have revised the core > change as well but I'm not going to post it until I do the built > artifact comparison, which I may not have time for till the weekend. Thanks. Saw that and replied already. >>> What I'm going to do is remove the internal-use-only _IO_* macros to >>> include/libio.h, and invent a new macro _IO_lock_t_defined which all >>> versions of stdio-lock.h will define; libio.h will define the stub >>> _IO_lock_t if that macro is not defined, with a comment explaining that >>> this is only relevant when building glibc itself. Then there will be no >>> uses of _IO_MTSAFE_IO in public headers, and it won't be necessary to >>> undefine it in include/stdio.h. >> >> OK, make sure we don't break old libstdc++ here, which I vaguely remember >> was tied into this macro and stdio-lock.h. > > I'll do some archaeology and find out for sure, but I _think_ that has > not been relevant since before libstdc++-v3 (so we're going all the > way back to the EGCS period). Moreover, we do not install > stdio-lock.h; any external software that attempts to define > _IO_MTSAFE_IO without supplying a definition of _IO_lock_t will fail > to compile, and do we really want to support software that provides > its own definition of _IO_lock_t? OK, I just reviewed this on our side. In RHEL7 (glibc 2.17) we install stdio-lock.h because we need to build our ancient compat versions of the compiler e.g. GCC-2.95.3: ~~~ # NPTL <bits/stdio-lock.h> is not usable outside of glibc, so include # the generic one (#162634) cp -a bits/stdio-lock.h $RPM_BUILD_ROOT%{_prefix}/include/bits/stdio-lock.h # And <bits/libc-lock.h> needs sanitizing as well. cp -a releng/libc-lock.h $RPM_BUILD_ROOT%{_prefix}/include/bits/libc-lock.h ~~~ I recently removed this requirement from Fedora and forced the old gcc package to carry it's own copies of the headers to use during builds. >>>>> + $(tests) $(tests-internal) $(xtests) $(test-srcs))): \ >>>>> + $(objpfx)libpthread.so \ >>>>> + $(objpfx)libpthread_nonshared.a >>>> >>>> Why doesn't this use $(shared-thread-library)? >>> >>> It was that way when I got here, and I don't actually see any code that >>> *sets* $(shared-thread-library) anywhere in the Makefiles, so I can't >>> confirm that it'd be the same thing. >> >> sysdeps/nptl/Makeconfig: > > Oh ghod, you mean to tell me subdirectories can have Makeconfig fragments too? Yes :-) >> shared-thread-library = $(common-objpfx)nptl/libpthread_nonshared.a \ >> $(common-objpfx)nptl/libpthread.so > > That's in the opposite order from the opencoded sequence in > nptl/Makefile. Are we sure it doesn't matter? It better not matter in this case. We are using it everywhere as our replacement.
--- a/dlfcn/tst-dladdr.c +++ b/dlfcn/tst-dladdr.c @@ -24,8 +24,6 @@ #include <stdlib.h> #include <string.h> -#include <ldsodefs.h> - #define TEST_FUNCTION do_test () extern int do_test (void); @@ -53,8 +51,6 @@ do_test (void) if (ret == 0) error (EXIT_FAILURE, 0, "dladdr failed"); - printf ("address of ref1 = %lx\n", - (unsigned long int) DL_LOOKUP_ADDRESS (sym)); printf ("ret = %d\n", ret); printf ("info.dli_fname = %p (\"%s\")\n", info.dli_fname, info.dli_fname);