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 v2] malloc: Add memalign test.


Will,

This test case is looking great, but needs some polishing.

What have you tested this on?

I'd like to see testing on atleast one 32-bit and one 64-bit
architecture.

On 09/11/2013 06:15 AM, Will Newton wrote:
> 
> ChangeLog:
> 
> 2013-08-16  Will Newton  <will.newton@linaro.org>
> 
>     * malloc/Makefile: Add tst-memalign.
>     * malloc/tst-memalign.c: New file.
> ---
>  malloc/Makefile       |  2 +-
>  malloc/tst-memalign.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 89 insertions(+), 1 deletion(-)
>  create mode 100644 malloc/tst-memalign.c
> 
> Changes in v2:
>  - Check errno in -pagesize failure case
> 
> diff --git a/malloc/Makefile b/malloc/Makefile
> index 17d146b..d482879 100644
> --- a/malloc/Makefile
> +++ b/malloc/Makefile
> @@ -27,7 +27,7 @@ headers := $(dist-headers) obstack.h mcheck.h
>  tests := mallocbug tst-malloc tst-valloc tst-calloc tst-obstack \
>  	 tst-mallocstate tst-mcheck tst-mallocfork tst-trim1 \
>  	 tst-malloc-usable tst-realloc tst-posix_memalign \
> -	 tst-pvalloc
> +	 tst-pvalloc tst-memalign
>  test-srcs = tst-mtrace
> 
>  routines = malloc morecore mcheck mtrace obstack
> diff --git a/malloc/tst-memalign.c b/malloc/tst-memalign.c
> new file mode 100644
> index 0000000..d46a2ef
> --- /dev/null
> +++ b/malloc/tst-memalign.c
> @@ -0,0 +1,88 @@
> +/* Copyright (C) 2013 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <errno.h>
> +#include <malloc.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <unistd.h>
> +
> +static int errors = 0;
> +
> +static void
> +merror (const char *msg)
> +{
> +  ++errors;
> +  printf ("Error: %s\n", msg);
> +}
> +
> +static int
> +do_test (void)
> +{
> +  void *p;
> +  unsigned long pagesize = getpagesize();
> +  unsigned long ptrval;
> +  int save;
> +
> +  errno = 0;
> +

Please add a comment to this test explaining what you're testing.

> +  p = memalign (sizeof (void *), -1);

OK, should fail with ENOMEM.

> +
> +  save = errno;
> +
> +  if (p != NULL)
> +    merror ("memalign (sizeof (void *), -1) succeeded.");
> +

OK.

> +  if (p == NULL && save != ENOMEM)
> +    merror ("memalign (sizeof (void *), -1) errno is not set correctly");
> +

OK.

Needs free (p) to be pedantically correct since the allocator
may have allocated something and we should try to return the
memory we allocated.

~~~

Similarly this needs a comment explaining what you're testing.

> +  errno = 0;
> +
> +  p = memalign (pagesize, -pagesize);
> +
> +  save = errno;
> +
> +  if (p != NULL)
> +    merror ("memalign (pagesize, -pagesize) succeeded.");
> +
> +  if (p == NULL && save != ENOMEM)
> +    merror ("memalign (pagesize, -pagesize) errno is not set correctly");
> +

Should also fail with ENOMEM, but didn't we just test this?

The value of -pagesize is just going to be a little smaller than -1
when converted to the unsigned size_t.

Needs free (p).

~~~

Similarly this needs a comment explaining what you're testing.

> +  p = memalign (sizeof (void *), 0);

OK, test for zero-sized allocation behaviour.

> +
> +  if (p == NULL)
> +    merror ("memalign (sizeof (void *), 0) failed.");
> +

There is no guarantee that I am aware of that requires 
that a memalign of size 0 should succeed.

In fact it is equally valid to get a NULL or a unique
address you can pass to free so I don't see what you
can test easily.

If you are testing the existing implementation behaviour,
then that's good, and your comment should mention that.
This test would then alert us if we changed the behaviour.

> +  free (p);

OK.

~~~

Similarly this needs a comment explaining what you're testing.

> +
> +  p = memalign (0x100, 10);
> +
> +  if (p == NULL)
> +    merror ("memalign (0x100, 10) failed.");

OK.

> +
> +  ptrval = (unsigned long)p;

Space after cast.

> +
> +  if (ptrval & 0xff)

No implicit boolean coersion please.

Use `ptrval & 0xff != 0'

> +    merror ("pointer is not aligned to 0x100");
> +
> +  free (p);
> +
> +  return errors != 0;
> +}
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
> 

Please post a v2 for review.

Cheers,
Carlos.


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