This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 3/4] Miscellaneous 'safe' testsuite changes.
On 02/20/2017 08:03 AM, Zack Weinberg wrote:
> These are a grab bag of changes where the testsuite was using internal
> symbols of some variety, but this was straightforward to fix, and the
> fixed code should work with or without the change to compile the
> testsuite under _ISOMAC. They are, however, not *quite* obvious and I
> would appreciate a second pair of eyes.
OK to checkin if you make copies of PTR_ALIGN_DOWN and ALIGN_DOWN for
support/ and convert the one test that uses them. That way all future
tests can use this code without needing to duplicate it. Also that way
glibc developers can use these functions like they would in ineternal
code.
> math/test-misc.c was using #ifndef NO_LONG_DOUBLE, which is an internal
> configuration macro, to decide whether to do certain tests involving
> 'long double'. I changed the test to #if LDBL_MANT_DIG > DBL_MANT_DIG
> instead, which uses only public float.h macros and is equivalent on
> all supported platforms. (Note that NO_LONG_DOUBLE doesn't mean 'the
> compiler doesn't support long double', it means 'long double is the
> same as double'.) It's possible that instead we should just do these
> tests unconditionally on all platforms.
Doing all the tests on all platforms is what we should have done from the
start to verify everything works as expected. However, your changes are
the right minimal fix for math/test-misc.c.
> atomic.h cannot be used by code compiled under _ISOMAC, but
> stdatomic.h can. There are quite a few tests that use atomic.h; most
> of them were not straightforward to change to stdatomic.h, but
> nptl/tst-join7.c was, so I did that.
These are going to be old tests, because where possible all the tests
should be using C11 atomics per our new policy of moving everything towards
C11 atomics. Therefore because they are old tests there aren't going to be
any more of them added like that so converting them is valuable and makes
the most sense.
> posix/wordexp-test.c used libc-internal.h for PTR_ALIGN_DOWN; I
> duplicated the definition into the .c file, which is not ideal, but
> since this didn't come up anywhere else, inventing a new header for it
> seems like excessive polish.
Please add PTR_ALIGN_DOWN to the support/ subsystem for testing and port
the test to support/.
We will conceivably need similar macros for other tests and as a glibc
developer I like to have access to the same helper macros I use in glib.
> string/strcasestr.c had an explicit #include <config.h> - this appears
> originally to have been externally maintained code. Removed.
OK.
> sysdeps/powerpc/fpu/tst-setcontext-fpscr.c appears to have been
> written before the advent of sys/auxv.h; it was using sysdep.h
> instead, which is obviously a no-go in _ISOMAC mode. I made the
> minimal change; I think a big chunk of this file could be replaced by
> a simple call to getauxval but I'll let someone who actually has a
> powerpc machine to test on do that.
OK.
> tst-writev.c has a configuration macro 'ARTIFICIAL_LIMIT' that the
> Makefiles are expected to define, and sysdeps/unix/sysv/linux/Makefile
> was using the internal __getpagesize in the definition; changed to
> sysconf(_SC_PAGESIZE) which is the POSIX equivalent.
OK.
> ia64-linux doesn't supply 'clone', only '__clone2', which is not
> defined in the public headers(!) All the other clone tests have local
> extern declarations of __clone2, but tst-clone.c doesn't; it was
> getting away with this because include/sched.h does declare __clone2.
>
> Finally, tst-setgetname.c was using kernel-features.h (also a no-go in
> _ISOMAC mode) just for one __ASSUME macro which it doesn't really need.
OK.
> zw
>
> * math/test-misc.c: Instead of testing NO_LONG_DOUBLE, test
> whether LDBL_MANT_DIG is greater than DBL_MANT_DIG.
> * nptl/tst-join7.c: Include stdlib.h. Include stdatomic.h instead of
> atomic.h. Use C11 atomics instead of libc-internal atomics.
> * posix/wordexp-test.c: Include stdint.h. Don't include
> libc-internal.h. Define PTR_ALIGN_DOWN here.
> * string/strcasestr.c: No need to include config.h.
> * sysdeps/powerpc/fpu/tst-setcontext-fpscr.c: Include
> sys/auxv.h. Don't include sysdep.h.
> * sysdeps/unix/sysv/linux/tst-clone.c [__ia64__]: Add extern
> declaration of __clone2.
> * sysdeps/unix/sysv/linux/Makefile (CFLAGS-tst-writev.c):
> Use sysconf (_SC_PAGESIZE) instead of __getpagesize in definition of
> ARTIFICIAL_LIMIT.
> * sysdeps/unix/sysv/linux/tst-setgetname.c:
> Don't include kernel-features.h. Don't #ifndef out anything
> based on __ASSUME macros.
> ---
> math/test-misc.c | 23 +++++++++++------------
> nptl/tst-join7mod.c | 9 +++++----
> posix/wordexp-test.c | 6 +++++-
> string/strcasestr.c | 4 ----
> sysdeps/powerpc/fpu/tst-setcontext-fpscr.c | 2 +-
> sysdeps/unix/sysv/linux/Makefile | 2 +-
> sysdeps/unix/sysv/linux/tst-clone.c | 5 +++++
> sysdeps/unix/sysv/linux/tst-setgetname.c | 3 ---
> 8 files changed, 28 insertions(+), 26 deletions(-)
>
> diff --git a/math/test-misc.c b/math/test-misc.c
> index 8968b80662..09812a3ccf 100644
> --- a/math/test-misc.c
> +++ b/math/test-misc.c
> @@ -30,7 +30,7 @@ do_test (void)
> {
> int result = 0;
>
> -#ifndef NO_LONG_DOUBLE
> +#if LDBL_MANT_DIG > DBL_MANT_DIG
OK.
> {
> long double x = 0x100000001ll + (long double) 0.5;
> long double q;
> @@ -60,7 +60,7 @@ do_test (void)
> # elif LDBL_MANT_DIG == 113
> m = 0x1.ffffffffffffffffffffffffffffp-1L;
> # else
> -# error "Please adjust"
> +# error "Unsupported LDBL_MANT_DIG, please adjust"
OK.
> # endif
>
> for (i = LDBL_MAX_EXP, x = LDBL_MAX; i >= LDBL_MIN_EXP; --i, x /= 2.0L)
> @@ -720,7 +720,7 @@ do_test (void)
> }
> }
>
> -#ifndef NO_LONG_DOUBLE
> +#if LDBL_MANT_DIG > DBL_MANT_DIG
OK.
> {
> long double v1, v2;
>
> @@ -910,7 +910,7 @@ do_test (void)
> puts ("isnormal (DBL_MIN) failed");
> result = 1;
> }
> -#ifndef NO_LONG_DOUBLE
> +#if LDBL_MANT_DIG > DBL_MANT_DIG
OK.
> if (! isnormal (LDBL_MIN))
> {
> puts ("isnormal (LDBL_MIN) failed");
> @@ -960,7 +960,7 @@ do_test (void)
> }
> #endif
>
> -#ifndef NO_LONG_DOUBLE
> +#if LDBL_MANT_DIG > DBL_MANT_DIG
OK.
> {
> long double r;
>
> @@ -1027,7 +1027,7 @@ do_test (void)
> puts ("nextafter -DBL_MIN test failed");
> result = 1;
> }
> -#ifndef NO_LONG_DOUBLE
> +#if LDBL_MANT_DIG > DBL_MANT_DIG
OK.
> if (nextafterl (nextafterl (LDBL_MIN, LDBL_MIN / 2.0), LDBL_MIN)
> != LDBL_MIN)
> {
> @@ -1072,7 +1072,7 @@ do_test (void)
> }
> #endif
>
> -#ifndef NO_LONG_DOUBLE
> +#if LDBL_MANT_DIG > DBL_MANT_DIG
OK.
> volatile long double ld1 = LDBL_MAX;
> volatile long double ld2 = LDBL_MAX / 2;
> (void) &ld1;
> @@ -1087,9 +1087,8 @@ do_test (void)
> result = 1;
> }
> # endif
> -#endif
>
> -#if !defined NO_LONG_DOUBLE && LDBL_MANT_DIG == 113
> +# if LDBL_MANT_DIG == 113
OK.
The check above for 'LDBL_MANT_DIG > DBL_MANT_DIG' covers
the old NO_LONG_DOUBLE check.
> volatile long double ld3 = 0x1.0000000000010000000100000001p+1;
> volatile long double ld4 = 0x1.0000000000000000000000000001p+1;
> (void) &ld3;
> @@ -1100,14 +1099,13 @@ do_test (void)
> printf ("long double subtraction test failed %.28La\n", ld3);
> result = 1;
> }
> -#endif
> +# endif
OK.
>
> /* Skip testing IBM long double format, for 2 reasons:
> 1) it only supports FE_TONEAREST
> 2) nextafter (0.0, 1.0) == nextafterl (0.0L, 1.0L), so
> nextafter (0.0, 1.0) / 16.0L will be 0.0L. */
> -#if !defined NO_LONG_DOUBLE && LDBL_MANT_DIG >= DBL_MANT_DIG + 4 \
> - && LDBL_MANT_DIG != 106
> +# if LDBL_MANT_DIG >= DBL_MANT_DIG + 4 && LDBL_MANT_DIG != 106
OK.
Likewise.
> int oldmode = fegetround ();
> int j;
> for (j = 0; j < 4; j++)
> @@ -1197,6 +1195,7 @@ do_test (void)
> else
> puts ("ignoring this failure");
> }
> +# endif
OK.
> #endif
>
> return result;
> diff --git a/nptl/tst-join7mod.c b/nptl/tst-join7mod.c
> index 1d9c95d418..5a43404571 100644
> --- a/nptl/tst-join7mod.c
> +++ b/nptl/tst-join7mod.c
> @@ -18,17 +18,18 @@
> <http://www.gnu.org/licenses/>. */
>
> #include <stdio.h>
> +#include <stdlib.h>
> #include <string.h>
> #include <pthread.h>
> -#include <atomic.h>
> +#include <stdatomic.h>
>
> static pthread_t th;
> -static int running = 1;
> +static atomic_int running = 1;
>
> static void *
> test_run (void *p)
> {
> - while (atomic_load_relaxed (&running))
> + while (atomic_load_explicit (&running, memory_order_relaxed))
OK.
> printf ("Test running\n");
> printf ("Test finished\n");
> return NULL;
> @@ -49,7 +50,7 @@ do_init (void)
> static void __attribute__ ((destructor))
> do_end (void)
> {
> - atomic_store_relaxed (&running, 0);
> + atomic_store_explicit (&running, 0, memory_order_relaxed);
OK.
> int ret = pthread_join (th, NULL);
>
> if (ret != 0)
> diff --git a/posix/wordexp-test.c b/posix/wordexp-test.c
> index 15eb233001..af174cf7d0 100644
> --- a/posix/wordexp-test.c
> +++ b/posix/wordexp-test.c
> @@ -22,10 +22,14 @@
> #include <unistd.h>
> #include <pwd.h>
> #include <stdio.h>
> +#include <stdint.h>
> #include <stdlib.h>
> #include <string.h>
> #include <wordexp.h>
> -#include <libc-internal.h>
> +
> +#define ALIGN_DOWN(base, size) ((base) & -((__typeof__ (base)) (size)))
> +#define PTR_ALIGN_DOWN(base, size) \
> + ((__typeof__ (base)) ALIGN_DOWN ((uintptr_t) (base), (size)))
These should go into support/ and the test should be converted to use that.
>
> #define IFS " \n\t"
>
> diff --git a/string/strcasestr.c b/string/strcasestr.c
> index a9ff18c7f5..2acf003155 100644
> --- a/string/strcasestr.c
> +++ b/string/strcasestr.c
> @@ -25,10 +25,6 @@
> *
> * Stephen R. van den Berg, berg@pool.informatik.rwth-aachen.de */
>
> -#if HAVE_CONFIG_H
> -# include <config.h>
> -#endif
> -
OK.
> /* Specification. */
> #include <string.h>
>
> diff --git a/sysdeps/powerpc/fpu/tst-setcontext-fpscr.c b/sysdeps/powerpc/fpu/tst-setcontext-fpscr.c
> index 3a8d699b9a..4e3f90d4d3 100644
> --- a/sysdeps/powerpc/fpu/tst-setcontext-fpscr.c
> +++ b/sysdeps/powerpc/fpu/tst-setcontext-fpscr.c
> @@ -26,8 +26,8 @@
> #include <malloc.h>
> #include <link.h>
> #include <elf.h>
> -#include <sysdep.h>
> #include <fpu_control.h>
> +#include <sys/auxv.h>
>
> static ucontext_t ctx[3];
>
> diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
> index b3d68665f9..fc29c54c86 100644
> --- a/sysdeps/unix/sysv/linux/Makefile
> +++ b/sysdeps/unix/sysv/linux/Makefile
> @@ -20,7 +20,7 @@ sysdep_routines += clone umount umount2 readahead \
> personality
>
> CFLAGS-gethostid.c = -fexceptions
> -CFLAGS-tst-writev.c += "-DARTIFICIAL_LIMIT=0x80000000-__getpagesize()"
> +CFLAGS-tst-writev.c += "-DARTIFICIAL_LIMIT=(0x80000000-sysconf(_SC_PAGESIZE))"
OK.
>
> # Note that bits/mman-linux.h is listed here though the file lives in the
> # top-level bits/ subdirectory instead of here in sysdeps/.../linux/bits/.
> diff --git a/sysdeps/unix/sysv/linux/tst-clone.c b/sysdeps/unix/sysv/linux/tst-clone.c
> index c4e6fbe48b..1da749db8d 100644
> --- a/sysdeps/unix/sysv/linux/tst-clone.c
> +++ b/sysdeps/unix/sysv/linux/tst-clone.c
> @@ -23,6 +23,11 @@
> #include <unistd.h>
> #include <sched.h>
>
> +#ifdef __ia64__
> +extern int __clone2 (int (*__fn) (void *__arg), void *__child_stack_base,
> + size_t __child_stack_size, int __flags, void *__arg, ...);
> +#endif
OK.
> +
> int child_fn(void *arg)
> {
> puts ("FAIL: in child_fn(); should not be here");
> diff --git a/sysdeps/unix/sysv/linux/tst-setgetname.c b/sysdeps/unix/sysv/linux/tst-setgetname.c
> index 5acd614117..b1010f0446 100644
> --- a/sysdeps/unix/sysv/linux/tst-setgetname.c
> +++ b/sysdeps/unix/sysv/linux/tst-setgetname.c
> @@ -23,7 +23,6 @@
> #include <unistd.h>
> #include <fcntl.h>
> #include <errno.h>
> -#include <kernel-features.h>
>
> /* New name of process. */
> #define NEW_NAME "setname"
> @@ -101,7 +100,6 @@ do_test (int argc, char **argv)
> {
> res = get_self_comm (gettid (), name_check, TASK_COMM_LEN);
>
> -#ifndef __ASSUME_PROC_PID_TASK_COMM
> /* On this first test we look for ENOENT to be returned from
> get_self_comm to indicate that the kernel is older than
> 2.6.33 and doesn't contain comm within the proc structure.
> @@ -111,7 +109,6 @@ do_test (int argc, char **argv)
> printf ("SKIP: The kernel does not have /proc/self/task/%%lu/comm.\n");
> return 0;
> }
> -#endif
OK. We should just run this test, no need to cut it out.
>
> if (res == 0)
> {
>
--
Cheers,
Carlos.