This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 4/4] Add IS_IN (testsuite) and remaining fixes.


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]