This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v2] malloc: Add memalign test.
- From: Will Newton <will dot newton at linaro dot org>
- To: "Carlos O'Donell" <carlos at redhat dot com>
- Cc: libc-alpha <libc-alpha at sourceware dot org>, Patch Tracking <patches at linaro dot org>
- Date: Thu, 3 Oct 2013 09:01:11 +0100
- Subject: Re: [PATCH v2] malloc: Add memalign test.
- Authentication-results: sourceware.org; auth=none
- References: <523042BB dot 5010502 at linaro dot org> <524C4637 dot 1070605 at redhat dot com>
On 2 October 2013 17:13, Carlos O'Donell <carlos@redhat.com> wrote:
> Will,
>
> This test case is looking great, but needs some polishing.
>
> What have you tested this on?
arm and x86_64.
> 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 -pagesize case is interesting because it demonstrates the integer
overflow issue that I fixed previously. I'll add a comment about that.
> 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.
Yes, that's the intent of the test. It's not mandated by any spec how
to behave on zero-sized allocation but essentially glibc has made the
choice to behave in a certain way and I don't believe we can now
change that.
I'll submit a v3 with the changes you requested, thanks!
--
Will Newton
Toolchain Working Group, Linaro