Re: [PATCH v2] malloc: Add memalign test.

[Will Newton <will dot newton at linaro dot org>]

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