[3/4] Miscellaneous 'safe' testsuite changes.

Message ID 20170220130342.6373-4-zackw@panix.com
State Superseded
Headers

Commit Message

Zack Weinberg Feb. 20, 2017, 1:03 p.m. UTC
  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.

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.

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.

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.

string/strcasestr.c had an explicit #include <config.h> - this appears
originally to have been externally maintained code.  Removed.

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.

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.

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.

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(-)
  

Comments

Carlos O'Donell Feb. 20, 2017, 2:11 p.m. UTC | #1
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)
>         {
>
  
Joseph Myers Feb. 20, 2017, 2:52 p.m. UTC | #2
On Mon, 20 Feb 2017, Zack Weinberg wrote:

> 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.

stdatomic.h is not available before GCC 4.9, and our minimum version for 
building glibc is 4.7 (although you can't use build-many-glibcs.py with 
versions before 4.9 because of missing features for bootstrapping cross 
compilers with glibc, in particular --with-glibc-version).
  
Zack Weinberg Feb. 20, 2017, 3:34 p.m. UTC | #3
On Mon, Feb 20, 2017 at 9:52 AM, Joseph Myers <joseph@codesourcery.com> wrote:
> On Mon, 20 Feb 2017, Zack Weinberg wrote:
>> 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.
>
> stdatomic.h is not available before GCC 4.9, and our minimum version for
> building glibc is 4.7 (although you can't use build-many-glibcs.py with
> versions before 4.9 because of missing features for bootstrapping cross
> compilers with glibc, in particular --with-glibc-version).

I don't recall exactly what was wrong with atomic.h in _ISOMAC mode
but I suspect it is not trivial to fix.  I *could* just give up here
and move tst-join7 to tests-internal, but if the "new policy" is to
use C11 atomics, perhaps that is sufficient reason to bump the minimum
version requirement to 4.9??

zw
  
Carlos O'Donell Feb. 20, 2017, 4:05 p.m. UTC | #4
On 02/20/2017 10:34 AM, Zack Weinberg wrote:
> On Mon, Feb 20, 2017 at 9:52 AM, Joseph Myers <joseph@codesourcery.com> wrote:
>> On Mon, 20 Feb 2017, Zack Weinberg wrote:
>>> 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.
>>
>> stdatomic.h is not available before GCC 4.9, and our minimum version for
>> building glibc is 4.7 (although you can't use build-many-glibcs.py with
>> versions before 4.9 because of missing features for bootstrapping cross
>> compilers with glibc, in particular --with-glibc-version).
> 
> I don't recall exactly what was wrong with atomic.h in _ISOMAC mode
> but I suspect it is not trivial to fix.  I *could* just give up here
> and move tst-join7 to tests-internal, but if the "new policy" is to
> use C11 atomics, perhaps that is sufficient reason to bump the minimum
> version requirement to 4.9??

Please restart this in a different thread, for context:

C11 (internally, not exposed via headers):
https://sourceware.org/glibc/wiki/Consensus#Standards_we_use

RFC: requiring GCC >= 4.7 to build glibc
https://sourceware.org/ml/libc-alpha/2015-08/msg00851.html

Google developers appeared to be blockers here because of Ubuntu 14.04LTS
using gcc 4.8, and that should be verified again, include Mike Frysinger
to answer that question.

Paul Eggart recommended gcc 4.9.

Upstream GCC has no FSF supported 4x. branches, they are all 5/6/7 support.

Therefore if we are going to move the minimum required build compiler it
should be to some supported FSF branch e.g. gcc 5.4.

I think that users expecting new new glibc to build with old old gcc have
their expectations set wrong. They should be using old glibc branches and
working on backports if they need ABI/API-neutral fixes.

This has nothing to do with the compiler used to build the applications
though, there we need to continue to support very old gcc's.
  
Joseph Myers Feb. 20, 2017, 4:15 p.m. UTC | #5
On Mon, 20 Feb 2017, Zack Weinberg wrote:

> 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.

That macros is only used there.  Effectively, its function is to say "when 
we remove support for 2.6.32 kernels this code can be removed".  So if we 
remove the conditional we should also remove the macro definition and find 
some other way to track that this code should be removed when we increase 
the minimum kernel version for x86_64 / x86.  (Or we could decide that an 
extra year of support for 2.6.32 kernels on those architectures, compared 
to other architectures having required 3.2 in glibc 2.24, is enough, and 
just increase the minimum version so the conditional code is no longer 
needed.)
  
Joseph Myers Feb. 20, 2017, 4:26 p.m. UTC | #6
On Mon, 20 Feb 2017, Carlos O'Donell wrote:

> > 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.

I agree the tests of long double functions should be run even when long 
double functions alias the double ones.  But that depends on patch 4 
(which stops most tests from being built with _LIBC defined) to do cleanly 
(as in the case where they are aliases, the long double functions aren't 
declared when _LIBC is defined, since the code defining them as aliases 
wouldn't work if they were declared - you can't define aliases when 
declarations with different types are visible).  The changes to use 
LDBL_MANT_DIG > DBL_MANT_DIG seem clearly the right thing for patch 3 in 
this series.
  
Joseph Myers Feb. 20, 2017, 4:30 p.m. UTC | #7
On Mon, 20 Feb 2017, Zack Weinberg wrote:

> I don't recall exactly what was wrong with atomic.h in _ISOMAC mode
> but I suspect it is not trivial to fix.  I *could* just give up here
> and move tst-join7 to tests-internal, but if the "new policy" is to
> use C11 atomics, perhaps that is sufficient reason to bump the minimum
> version requirement to 4.9??

Policy for internal operations is to use operations matched to C11 
semantics as far as possible; not necessarily the public <stdatomic.h> 
interfaces or the __atomic_* built-in functions (in cases where that 
brings in a libatomic dependency, that's not suitable for use in glibc, 
but __atomic_*, which were introduced in GCC 4.7, may be used on 
architectures where they don't involve problematic dependencies, and may 
well be a sensible default to use on architectures where there isn't a 
reason to do something different).

I'd be fine with increasing the minimum GCC version, although I don't 
think there's that much conditional code to remove with an increase to 
4.9.
  
Zack Weinberg Feb. 25, 2017, 4:14 p.m. UTC | #8
On 02/20/2017 09:52 AM, Joseph Myers wrote:
> On Mon, 20 Feb 2017, Zack Weinberg wrote:
> 
>> 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.
> 
> stdatomic.h is not available before GCC 4.9, and our minimum version
>  for building glibc is 4.7 (although you can't use 
> build-many-glibcs.py with versions before 4.9 because of missing 
> features for bootstrapping cross compilers with glibc, in particular
>  --with-glibc-version).
...
>> 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.
> 
> That macros is only used there.  Effectively, its function is to say 
> "when we remove support for 2.6.32 kernels this code can be 
> removed".

Rather than hold up this patch on questions of whether we can bump the
minimum compiler or kernel requirement, I am going to drop these changes
and move the affected tests to tests-internal.

zw
  
Zack Weinberg Feb. 25, 2017, 4:34 p.m. UTC | #9
On 02/20/2017 09:11 AM, Carlos O'Donell wrote:
> On 02/20/2017 08:03 AM, Zack Weinberg wrote:
>>
>> 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.

wordexp-test.c is currently not even using the old test skeleton, and
does some fairly hairy things with fork handlers.  I would prefer not to
go down a rabbit hole.  Instead I propose to do something similar to
what I did for the DIAG_* macros: introduce a new header in include/
called libc-pointer-arith.h; move cast_to_integer, ALIGN_UP, ALIGN_DOWN,
PTR_ALIGN_UP, and PTR_ALIGN_DOWN there; have libc-internal.h include it;
have wordexp-test.c include it instead of libc-internal.h; and call that
good enough for now.  OK?

zw
  
Florian Weimer Feb. 25, 2017, 9:06 p.m. UTC | #10
* Zack Weinberg:

> wordexp-test.c is currently not even using the old test skeleton, and
> does some fairly hairy things with fork handlers.  I would prefer not to
> go down a rabbit hole.  Instead I propose to do something similar to
> what I did for the DIAG_* macros: introduce a new header in include/
> called libc-pointer-arith.h; move cast_to_integer, ALIGN_UP, ALIGN_DOWN,
> PTR_ALIGN_UP, and PTR_ALIGN_DOWN there; have libc-internal.h include it;
> have wordexp-test.c include it instead of libc-internal.h; and call that
> good enough for now.  OK?

If we move into that direction, can we also replace #include
<libc-internal.h> in sysdeps/x86_64/nptl/tls.h?

I think this is one major cause why things compile on x86-64, but not
other architectures.
  
Carlos O'Donell March 1, 2017, 8:19 p.m. UTC | #11
On 02/25/2017 11:34 AM, Zack Weinberg wrote:
> On 02/20/2017 09:11 AM, Carlos O'Donell wrote:
>> On 02/20/2017 08:03 AM, Zack Weinberg wrote:
>>>
>>> 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.
> 
> wordexp-test.c is currently not even using the old test skeleton, and
> does some fairly hairy things with fork handlers.  I would prefer not to
> go down a rabbit hole.  Instead I propose to do something similar to
> what I did for the DIAG_* macros: introduce a new header in include/
> called libc-pointer-arith.h; move cast_to_integer, ALIGN_UP, ALIGN_DOWN,
> PTR_ALIGN_UP, and PTR_ALIGN_DOWN there; have libc-internal.h include it;
> have wordexp-test.c include it instead of libc-internal.h; and call that
> good enough for now.  OK?

Yeah, that seems like a sensible compromise.
  

Patch

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
   {
     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"
 # 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
   {
     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
   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
   {
     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
   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
   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
   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
 
 /* 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
   int oldmode = fegetround ();
   int j;
   for (j = 0; j < 4; j++)
@@ -1197,6 +1195,7 @@  do_test (void)
       else
 	puts ("ignoring this failure");
     }
+# endif
 #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))
     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);
   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)))
 
 #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
-
 /* 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))"
 
 # 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
+
 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
 
       if (res == 0)
        {