| Message ID | 523042BB.5010502@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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?
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?
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.
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!
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"
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