| Message ID | 6157828e-4b05-5261-ea8a-e3a531697d4e@panix.com |
|---|---|
| State | New |
| Headers | show |
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);