diff mbox series

[v2] Add a generic malloc test for MALLOC_ALIGNMENT

Message ID CAMe9rOo_7t4z27zvkv3zN5WhDtGJQ7xM0GEJ3fF8+seQcEa4WQ@mail.gmail.com
State Superseded
Headers show
Series [v2] Add a generic malloc test for MALLOC_ALIGNMENT | expand

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit fail Patch caused testsuite regressions

Commit Message

H.J. Lu July 9, 2021, 12:24 p.m. UTC
On Thu, Jul 8, 2021 at 9:07 PM Siddhesh Poyarekar <siddhesh@gotplt.org> wrote:
>
> > diff --git a/malloc/Makefile b/malloc/Makefile
> > index 37a9a4efab..b685ed6d61 100644
> > --- a/malloc/Makefile
> > +++ b/malloc/Makefile
> > @@ -42,6 +42,7 @@ tests := mallocbug tst-malloc tst-valloc tst-calloc tst-obstack \
> >        tst-malloc-stats-cancellation \
> >        tst-tcfree1 tst-tcfree2 tst-tcfree3 \
> >        tst-safe-linking \
> > +      tst-mallocalign1 \
> >
>
> Please remove the trailing '\'.  OK otherwise.
>

Changes in the v2 patch

1. Add a comment in malloc/tst-mallocalign1.c to indicate that it is
used to verify that MALLOC_ALIGNMENT is honored by malloc.
2. Include <malloc-alignment.h> in malloc-size.h after SIZE_SZ is
defined since <malloc-alignment.h> may use SIZE_SZ.

BTW, I kept the trailing '\' since it is used in all other places in
malloc/Makefile.

OK for master?

Thanks.

Comments

Siddhesh Poyarekar July 9, 2021, 1 p.m. UTC | #1
On 7/9/21 5:54 PM, H.J. Lu wrote:
> On Thu, Jul 8, 2021 at 9:07 PM Siddhesh Poyarekar <siddhesh@gotplt.org> wrote:
>>
>>> diff --git a/malloc/Makefile b/malloc/Makefile
>>> index 37a9a4efab..b685ed6d61 100644
>>> --- a/malloc/Makefile
>>> +++ b/malloc/Makefile
>>> @@ -42,6 +42,7 @@ tests := mallocbug tst-malloc tst-valloc tst-calloc tst-obstack \
>>>         tst-malloc-stats-cancellation \
>>>         tst-tcfree1 tst-tcfree2 tst-tcfree3 \
>>>         tst-safe-linking \
>>> +      tst-mallocalign1 \
>>>
>>
>> Please remove the trailing '\'.  OK otherwise.
>>
> 
> Changes in the v2 patch
> 
> 1. Add a comment in malloc/tst-mallocalign1.c to indicate that it is
> used to verify that MALLOC_ALIGNMENT is honored by malloc.
> 2. Include <malloc-alignment.h> in malloc-size.h after SIZE_SZ is
> defined since <malloc-alignment.h> may use SIZE_SZ.
> 
> BTW, I kept the trailing '\' since it is used in all other places in
> malloc/Makefile.
> 
> OK for master?

I forgot to ask, does it run successfully on i686 and arm?  IIRC it too 
requires 16 byte alignment and hence both i686 and x32 will fail.

If they don't succeed then it may make sense to add

tests-exclude-mcheck += tst-mallocalign1

for x32 and i686.

Siddhesh
diff mbox series

Patch

From f538de896904ee77c17391af7007a085a40235a1 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Thu, 8 Jul 2021 20:48:14 -0700
Subject: [PATCH v2] Add a generic malloc test for MALLOC_ALIGNMENT

Changes in the v2 patch

1. Add a comment in malloc/tst-mallocalign1.c to indicate that it is
used to verify that MALLOC_ALIGNMENT is honored by malloc.
2. Include <malloc-alignment.h> in malloc-size.h after SIZE_SZ is
defined since <malloc-alignment.h> may use SIZE_SZ.

BTW, I kept the trailing '\' since it is used all other places in
malloc/Makefile.

H.J.
---
1. Add sysdeps/generic/malloc-size.h to define size related macros for
malloc.
2. Move x86_64/tst-mallocalign1.c malloc and replace ALIGN_MASK with
MALLOC_ALIGN_MASK.
---
 malloc/Makefile                               |  1 +
 malloc/malloc-internal.h                      | 41 +-----------
 {sysdeps/x86_64 => malloc}/tst-mallocalign1.c | 31 +++++----
 sysdeps/generic/malloc-machine.h              |  1 -
 sysdeps/generic/malloc-size.h                 | 64 +++++++++++++++++++
 sysdeps/x86_64/Makefile                       |  4 --
 6 files changed, 81 insertions(+), 61 deletions(-)
 rename {sysdeps/x86_64 => malloc}/tst-mallocalign1.c (65%)
 create mode 100644 sysdeps/generic/malloc-size.h

diff --git a/malloc/Makefile b/malloc/Makefile
index 37a9a4efab..b685ed6d61 100644
--- a/malloc/Makefile
+++ b/malloc/Makefile
@@ -42,6 +42,7 @@  tests := mallocbug tst-malloc tst-valloc tst-calloc tst-obstack \
 	 tst-malloc-stats-cancellation \
 	 tst-tcfree1 tst-tcfree2 tst-tcfree3 \
 	 tst-safe-linking \
+	 tst-mallocalign1 \
 
 tests-static := \
 	 tst-interpose-static-nothread \
diff --git a/malloc/malloc-internal.h b/malloc/malloc-internal.h
index 258f29584e..0c7b5a183c 100644
--- a/malloc/malloc-internal.h
+++ b/malloc/malloc-internal.h
@@ -21,46 +21,7 @@ 
 
 #include <malloc-machine.h>
 #include <malloc-sysdep.h>
-
-/* INTERNAL_SIZE_T is the word-size used for internal bookkeeping of
-   chunk sizes.
-
-   The default version is the same as size_t.
-
-   While not strictly necessary, it is best to define this as an
-   unsigned type, even if size_t is a signed type. This may avoid some
-   artificial size limitations on some systems.
-
-   On a 64-bit machine, you may be able to reduce malloc overhead by
-   defining INTERNAL_SIZE_T to be a 32 bit `unsigned int' at the
-   expense of not being able to handle more than 2^32 of malloced
-   space. If this limitation is acceptable, you are encouraged to set
-   this unless you are on a platform requiring 16byte alignments. In
-   this case the alignment requirements turn out to negate any
-   potential advantages of decreasing size_t word size.
-
-   Implementors: Beware of the possible combinations of:
-     - INTERNAL_SIZE_T might be signed or unsigned, might be 32 or 64 bits,
-       and might be the same width as int or as long
-     - size_t might have different width and signedness as INTERNAL_SIZE_T
-     - int and long might be 32 or 64 bits, and might be the same width
-
-   To deal with this, most comparisons and difference computations
-   among INTERNAL_SIZE_Ts should cast them to unsigned long, being
-   aware of the fact that casting an unsigned int to a wider long does
-   not sign-extend. (This also makes checking for negative numbers
-   awkward.) Some of these casts result in harmless compiler warnings
-   on some systems.  */
-#ifndef INTERNAL_SIZE_T
-# define INTERNAL_SIZE_T size_t
-#endif
-
-/* The corresponding word size.  */
-#define SIZE_SZ (sizeof (INTERNAL_SIZE_T))
-
-/* The corresponding bit mask value.  */
-#define MALLOC_ALIGN_MASK (MALLOC_ALIGNMENT - 1)
-
+#include <malloc-size.h>
 
 /* Called in the parent process before a fork.  */
 void __malloc_fork_lock_parent (void) attribute_hidden;
diff --git a/sysdeps/x86_64/tst-mallocalign1.c b/malloc/tst-mallocalign1.c
similarity index 65%
rename from sysdeps/x86_64/tst-mallocalign1.c
rename to malloc/tst-mallocalign1.c
index 33a6375777..294e821afe 100644
--- a/sysdeps/x86_64/tst-mallocalign1.c
+++ b/malloc/tst-mallocalign1.c
@@ -1,4 +1,5 @@ 
-/* Copyright (C) 2012-2021 Free Software Foundation, Inc.
+/* Verify that MALLOC_ALIGNMENT is honored by malloc.
+   Copyright (C) 2012-2021 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
@@ -17,17 +18,16 @@ 
 
 #include <stdio.h>
 #include <stdlib.h>
+#include <inttypes.h>
+#include <malloc-size.h>
 
-/* Specified by x86-64 psABI.  */
-#define ALIGN_MASK (16 - 1)
-
-void *
+static void *
 test (size_t s)
 {
   void *p = malloc (s);
 
-  printf ("malloc: %ld, %p: %ld\n", (unsigned long) s, p,
-	  ((unsigned long) p) & ALIGN_MASK);
+  printf ("malloc: %zu, %p: %zu\n", s, p,
+	  ((uintptr_t) p) & MALLOC_ALIGN_MASK);
   return p;
 }
 
@@ -38,35 +38,34 @@  do_test (void)
   int ret = 0;
 
   p = test (2);
-  ret |= (unsigned long) p & ALIGN_MASK;
+  ret |= (uintptr_t) p & MALLOC_ALIGN_MASK;
   free (p);
 
   p = test (8);
-  ret |= (unsigned long) p & ALIGN_MASK;
+  ret |= (uintptr_t) p & MALLOC_ALIGN_MASK;
   free (p);
 
   p = test (13);
-  ret |= (unsigned long) p & ALIGN_MASK;
+  ret |= (uintptr_t) p & MALLOC_ALIGN_MASK;
   free (p);
 
   p = test (16);
-  ret |= (unsigned long) p & ALIGN_MASK;
+  ret |= (uintptr_t) p & MALLOC_ALIGN_MASK;
   free (p);
 
   p = test (23);
-  ret |= (unsigned long) p & ALIGN_MASK;
+  ret |= (uintptr_t) p & MALLOC_ALIGN_MASK;
   free (p);
 
   p = test (43);
-  ret |= (unsigned long) p & ALIGN_MASK;
+  ret |= (uintptr_t) p & MALLOC_ALIGN_MASK;
   free (p);
 
   p = test (123);
-  ret |= (unsigned long) p & ALIGN_MASK;
+  ret |= (uintptr_t) p & MALLOC_ALIGN_MASK;
   free (p);
 
   return ret;
 }
 
-#define TEST_FUNCTION do_test ()
-#include "../test-skeleton.c"
+#include <support/test-driver.c>
diff --git a/sysdeps/generic/malloc-machine.h b/sysdeps/generic/malloc-machine.h
index a71f2361f5..121d8ad0c9 100644
--- a/sysdeps/generic/malloc-machine.h
+++ b/sysdeps/generic/malloc-machine.h
@@ -21,7 +21,6 @@ 
 #define _GENERIC_MALLOC_MACHINE_H
 
 #include <atomic.h>
-#include <malloc-alignment.h>
 
 #ifndef atomic_full_barrier
 # define atomic_full_barrier() __asm ("" ::: "memory")
diff --git a/sysdeps/generic/malloc-size.h b/sysdeps/generic/malloc-size.h
new file mode 100644
index 0000000000..41901b9a44
--- /dev/null
+++ b/sysdeps/generic/malloc-size.h
@@ -0,0 +1,64 @@ 
+/* Define INTERNAL_SIZE_T, SIZE_SZ, MALLOC_ALIGNMENT and MALLOC_ALIGN_MASK
+   for malloc.
+   Copyright (C) 2021 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
+   <https://www.gnu.org/licenses/>.  */
+
+#ifndef _GENERIC_MALLOC_SIZE_H
+#define _GENERIC_MALLOC_SIZE_H
+
+/* INTERNAL_SIZE_T is the word-size used for internal bookkeeping of
+   chunk sizes.
+
+   The default version is the same as size_t.
+
+   While not strictly necessary, it is best to define this as an
+   unsigned type, even if size_t is a signed type. This may avoid some
+   artificial size limitations on some systems.
+
+   On a 64-bit machine, you may be able to reduce malloc overhead by
+   defining INTERNAL_SIZE_T to be a 32 bit `unsigned int' at the
+   expense of not being able to handle more than 2^32 of malloced
+   space. If this limitation is acceptable, you are encouraged to set
+   this unless you are on a platform requiring 16byte alignments. In
+   this case the alignment requirements turn out to negate any
+   potential advantages of decreasing size_t word size.
+
+   Implementors: Beware of the possible combinations of:
+     - INTERNAL_SIZE_T might be signed or unsigned, might be 32 or 64 bits,
+       and might be the same width as int or as long
+     - size_t might have different width and signedness as INTERNAL_SIZE_T
+     - int and long might be 32 or 64 bits, and might be the same width
+
+   To deal with this, most comparisons and difference computations
+   among INTERNAL_SIZE_Ts should cast them to unsigned long, being
+   aware of the fact that casting an unsigned int to a wider long does
+   not sign-extend. (This also makes checking for negative numbers
+   awkward.) Some of these casts result in harmless compiler warnings
+   on some systems.  */
+#ifndef INTERNAL_SIZE_T
+# define INTERNAL_SIZE_T size_t
+#endif
+
+/* The corresponding word size.  */
+#define SIZE_SZ (sizeof (INTERNAL_SIZE_T))
+
+#include <malloc-alignment.h>
+
+/* The corresponding bit mask value.  */
+#define MALLOC_ALIGN_MASK (MALLOC_ALIGNMENT - 1)
+
+#endif /* _GENERIC_MALLOC_SIZE_H */
diff --git a/sysdeps/x86_64/Makefile b/sysdeps/x86_64/Makefile
index 3fc03f4a19..22c5b63ab5 100644
--- a/sysdeps/x86_64/Makefile
+++ b/sysdeps/x86_64/Makefile
@@ -13,10 +13,6 @@  sysdep_routines += _mcount
 sysdep_noprof += _mcount
 endif
 
-ifeq ($(subdir),malloc)
-tests += tst-mallocalign1
-endif
-
 ifeq ($(subdir),string)
 sysdep_routines += strcasecmp_l-nonascii strncase_l-nonascii
 gen-as-const-headers += locale-defines.sym
-- 
2.31.1