[v2] malloc: Add memalign test.

Message ID 523042BB.5010502@linaro.org
State Superseded
Headers show

Commit Message

Will Newton Sept. 11, 2013, 10:15 a.m.
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

Comments

Will Newton Sept. 25, 2013, 1:08 p.m. | #1
On 11 September 2013 11:15, Will Newton <will.newton@linaro.org> 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

Ping?
Will Newton Oct. 2, 2013, 9:48 a.m. | #2
On 25 September 2013 14:08, Will Newton <will.newton@linaro.org> wrote:
> On 11 September 2013 11:15, Will Newton <will.newton@linaro.org> 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
>
> Ping?

Ping?
Carlos O'Donell Oct. 2, 2013, 4:13 p.m. | #3
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.
Will Newton Oct. 3, 2013, 8:01 a.m. | #4
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!

Patch

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;
+
+  p = memalign (sizeof (void *), -1);
+
+  save = errno;
+
+  if (p != NULL)
+    merror ("memalign (sizeof (void *), -1) succeeded.");
+
+  if (p == NULL && save != ENOMEM)
+    merror ("memalign (sizeof (void *), -1) errno is not set correctly");
+
+  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");
+
+  p = memalign (sizeof (void *), 0);
+
+  if (p == NULL)
+    merror ("memalign (sizeof (void *), 0) failed.");
+
+  free (p);
+
+  p = memalign (0x100, 10);
+
+  if (p == NULL)
+    merror ("memalign (0x100, 10) failed.");
+
+  ptrval = (unsigned long)p;
+
+  if (ptrval & 0xff)
+    merror ("pointer is not aligned to 0x100");
+
+  free (p);
+
+  return errors != 0;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"