This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 4/4] Add IS_IN (testsuite) and remaining fixes.
- From: Carlos O'Donell <carlos at redhat dot com>
- To: Zack Weinberg <zackw at panix dot com>, libc-alpha at sourceware dot org
- Cc: joseph at codesourcery dot com, adhemerval dot zanella at linaro dot org
- Date: Wed, 1 Mar 2017 13:17:47 -0500
- Subject: Re: [PATCH 4/4] Add IS_IN (testsuite) and remaining fixes.
- Authentication-results: sourceware.org; auth=none
- 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> <6157828e-4b05-5261-ea8a-e3a531697d4e@panix.com>
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.
--
Cheers,
Carlos.