diff mbox series

malloc: add indirection for malloc(-like) functions in tests [BZ #32366]

Message ID 8a9a794bdf1fd6435764f3f836b4b3adce36bcfc.1733787863.git.sam@gentoo.org
State New
Headers show
Series malloc: add indirection for malloc(-like) functions in tests [BZ #32366] | expand

Commit Message

Sam James Dec. 9, 2024, 11:44 p.m. UTC 
GCC 15 introduces allocation dead code removal (DCE) for PR117370 in
r15-5255-g7828dc070510f8. This breaks various glibc tests which want
to assert various properties of the allocator without doing anything
obviously useful with the allocated memory.

Alexander Monakov rightly pointed out that we can and should do better
than passing -fno-malloc-dce to paper over the problem. Not least because
GCC 14 already does such DCE where there's no testing of malloc's return
value against NULL, and LLVM has such optimisations too.

Handle this by providing TEST_MALLOC (and friends) wrappers with a volatile
function pointer to obscure that we're calling malloc (et. al) from the
compiler.
---
There are some more functions we should perhaps cover like reallocarray
just to futureproof, but I was minded to do that separately to get this
fix in and unblock regression testers against GCC trunk.

 malloc/tst-aligned-alloc.c    | 12 +++++++-----
 malloc/tst-compathooks-off.c  | 14 ++++++++------
 malloc/tst-malloc-aux.h       | 36 +++++++++++++++++++++++++++++++++++
 malloc/tst-malloc-check.c     | 18 ++++++++++--------
 malloc/tst-malloc-too-large.c | 19 +++++++++---------
 malloc/tst-malloc.c           | 16 +++++++++-------
 malloc/tst-realloc.c          | 24 ++++++++++++-----------
 7 files changed, 93 insertions(+), 46 deletions(-)
 create mode 100644 malloc/tst-malloc-aux.h

Comments

Paul Eggert Dec. 10, 2024, 12:19 a.m. UTC | #1 
On 2024-12-09 15:44, Sam James wrote:

> +#define TEST_MALLOC malloc_indirect

How about the following instead?

   #undef malloc
   #define malloc malloc_indirect

This is what similar tests do in Gnulib.

Doing this would simplify the rest of the patch quite a bit because you 
could leave much of the existing testing code alone.

Similarly for calloc, etc.
Sam James Dec. 10, 2024, 12:25 a.m. UTC | #2 
Paul Eggert <eggert@cs.ucla.edu> writes:

> On 2024-12-09 15:44, Sam James wrote:
>
>> +#define TEST_MALLOC malloc_indirect
>
> How about the following instead?
>
>   #undef malloc
>   #define malloc malloc_indirect
>
> This is what similar tests do in Gnulib.

I should've mentioned but I tried this and it went awry:

In file included from ../test-skeleton.c:44,
                 from tst-malloc.c:100:
../support/support.h:117:3: error: ‘malloc_indirect’ attribute directive ignored [-Werror=attributes]
  117 |   __returns_nonnull;
      |   ^~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make: *** [../o-iterator.mk:9: /tmp/glibc-bld/malloc/tst-malloc.o] Error

This is because test-skeleton.c is included at the end of each test
which includes support.h after, which does:

char *xasprintf (const char *format, ...)
  __attribute__ ((format (printf, 1, 2), malloc)) __attr_dealloc_free
  __returns_nonnull;

Do you have any idea for working around that? I agree that if we can
make your suggestion work, it's far nicer.
Sam James Dec. 10, 2024, 12:31 a.m. UTC | #3 
Sam James <sam@gentoo.org> writes:

> Paul Eggert <eggert@cs.ucla.edu> writes:
>
>> On 2024-12-09 15:44, Sam James wrote:
>>
>>> +#define TEST_MALLOC malloc_indirect
>>
>> How about the following instead?
>>
>>   #undef malloc
>>   #define malloc malloc_indirect
>>
>> This is what similar tests do in Gnulib.
>
> I should've mentioned but I tried this and it went awry:
>
> In file included from ../test-skeleton.c:44,
>                  from tst-malloc.c:100:
> ../support/support.h:117:3: error: ‘malloc_indirect’ attribute directive ignored [-Werror=attributes]
>   117 |   __returns_nonnull;
>       |   ^~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> make: *** [../o-iterator.mk:9: /tmp/glibc-bld/malloc/tst-malloc.o] Error
>
> This is because test-skeleton.c is included at the end of each test
> which includes support.h after, which does:
>
> char *xasprintf (const char *format, ...)
>   __attribute__ ((format (printf, 1, 2), malloc)) __attr_dealloc_free
>   __returns_nonnull;

Actually, malloc -> __malloc__ works here. If that sounds acceptable,
I'll respin with that and post a v2 (assuming no other issues come up).
Paul Eggert Dec. 10, 2024, 12:31 a.m. UTC | #4 
On 2024-12-09 16:25, Sam James wrote:
> This is because test-skeleton.c is included at the end of each test
> which includes support.h after, which does:
> 
> char *xasprintf (const char *format, ...)
>    __attribute__ ((format (printf, 1, 2), malloc)) __attr_dealloc_free
>    __returns_nonnull;
> 
> Do you have any idea for working around that

Change "malloc" to "__malloc__" in support.h's uses of "__attribute__".
diff mbox series

Patch

diff --git a/malloc/tst-aligned-alloc.c b/malloc/tst-aligned-alloc.c
index 91167d1392..d56dfa7dd9 100644
--- a/malloc/tst-aligned-alloc.c
+++ b/malloc/tst-aligned-alloc.c
@@ -25,6 +25,8 @@ 
 #include <libc-diag.h>
 #include <support/check.h>
 
+#include "tst-malloc-aux.h"
+
 static int
 do_test (void)
 {
@@ -45,28 +47,28 @@  do_test (void)
      - A corner case SIZE_MAX / 2 + 1 alignment.
   */
 
-  p1 = aligned_alloc (64, 64);
+  p1 = TEST_ALIGNED_ALLOC (64, 64);
 
   if (p1 == NULL)
     FAIL_EXIT1 ("aligned_alloc(64, 64) failed");
 
-  p2 = aligned_alloc (1, 64);
+  p2 = TEST_ALIGNED_ALLOC (1, 64);
 
   if (p2 == NULL)
     FAIL_EXIT1 ("aligned_alloc(1, 64) failed");
 
-  p3 = aligned_alloc (65, 64);
+  p3 = TEST_ALIGNED_ALLOC (65, 64);
 
   if (p3 != NULL)
     FAIL_EXIT1 ("aligned_alloc(65, 64) did not fail");
 
-  p4 = aligned_alloc (0, 64);
+  p4 = TEST_ALIGNED_ALLOC (0, 64);
 
   if (p4 != NULL)
     FAIL_EXIT1 ("aligned_alloc(0, 64) did not fail");
 
   /* This is an alignment like 0x80000000...UL */
-  p5 = aligned_alloc (SIZE_MAX / 2 + 1, 64);
+  p5 = TEST_ALIGNED_ALLOC (SIZE_MAX / 2 + 1, 64);
 
   if (p5 != NULL)
     FAIL_EXIT1 ("aligned_alloc(SIZE_MAX/2+1, 64) did not fail");
diff --git a/malloc/tst-compathooks-off.c b/malloc/tst-compathooks-off.c
index d0106f3fb7..07790bd5d5 100644
--- a/malloc/tst-compathooks-off.c
+++ b/malloc/tst-compathooks-off.c
@@ -25,6 +25,8 @@ 
 #include <support/check.h>
 #include <support/support.h>
 
+#include "tst-malloc-aux.h"
+
 extern void (*volatile __free_hook) (void *, const void *);
 extern void *(*volatile __malloc_hook)(size_t, const void *);
 extern void *(*volatile __realloc_hook)(void *, size_t, const void *);
@@ -49,7 +51,7 @@  malloc_called (size_t bytes, const void *address)
 {
   hook_count++;
   __malloc_hook = NULL;
-  void *mem = malloc (bytes);
+  void *mem = TEST_MALLOC (bytes);
   __malloc_hook = malloc_called;
   return mem;
 }
@@ -59,7 +61,7 @@  realloc_called (void *oldptr, size_t bytes, const void *address)
 {
   hook_count++;
   __realloc_hook = NULL;
-  void *mem = realloc (oldptr, bytes);
+  void *mem = TEST_REALLOC (oldptr, bytes);
   __realloc_hook = realloc_called;
   return mem;
 }
@@ -69,7 +71,7 @@  calloc_called (size_t n, size_t size, const void *address)
 {
   hook_count++;
   __malloc_hook = NULL;
-  void *mem = calloc (n, size);
+  void *mem = TEST_CALLOC (n, size);
   __malloc_hook = malloc_called;
   return mem;
 }
@@ -109,15 +111,15 @@  static int
 do_test (void)
 {
   void *p;
-  p = malloc (0);
+  p = TEST_MALLOC (0);
   TEST_VERIFY_EXIT (p != NULL);
   call_count++;
 
-  p = realloc (p, 0);
+  p = TEST_REALLOC (p, 0);
   TEST_VERIFY_EXIT (p == NULL);
   call_count++;
 
-  p = calloc (512, 1);
+  p = TEST_CALLOC (512, 1);
   TEST_VERIFY_EXIT (p != NULL);
   call_count++;
 
diff --git a/malloc/tst-malloc-aux.h b/malloc/tst-malloc-aux.h
new file mode 100644
index 0000000000..c4078d8a2f
--- /dev/null
+++ b/malloc/tst-malloc-aux.h
@@ -0,0 +1,36 @@ 
+/* Wrappers for malloc-like functions to allow testing the implementation
+   without optimization.
+   Copyright (C) 2024 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; see the file COPYING.LIB.  If
+   not, see <https://www.gnu.org/licenses/>.  */
+
+#ifndef TST_MALLOC_AUX_H
+#define TST_MALLOC_AUX_H
+
+#include <stddef.h>
+#include <stdlib.h>
+
+static void *(*volatile aligned_alloc_indirect)(size_t, size_t) = aligned_alloc;
+static void *(*volatile calloc_indirect)(size_t, size_t) = calloc;
+static void *(*volatile malloc_indirect)(size_t) = malloc;
+static void *(*volatile realloc_indirect)(void*, size_t) = realloc;
+
+#define TEST_ALIGNED_ALLOC aligned_alloc_indirect
+#define TEST_CALLOC calloc_indirect
+#define TEST_MALLOC malloc_indirect
+#define TEST_REALLOC realloc_indirect
+
+#endif /* TST_MALLOC_AUX_H */
diff --git a/malloc/tst-malloc-check.c b/malloc/tst-malloc-check.c
index fde8863ad7..be80b262be 100644
--- a/malloc/tst-malloc-check.c
+++ b/malloc/tst-malloc-check.c
@@ -20,6 +20,8 @@ 
 #include <stdlib.h>
 #include <libc-diag.h>
 
+#include "tst-malloc-aux.h"
+
 static int errors = 0;
 
 static void
@@ -42,7 +44,7 @@  do_test (void)
      that they fail.  */
   DIAG_IGNORE_NEEDS_COMMENT (7, "-Walloc-size-larger-than=");
 #endif
-  p = malloc (-1);
+  p = TEST_MALLOC (-1);
   DIAG_POP_NEEDS_COMMENT;
 
   if (p != NULL)
@@ -50,27 +52,27 @@  do_test (void)
   else if (errno != ENOMEM)
     merror ("errno is not set correctly.");
 
-  p = malloc (10);
+  p = TEST_MALLOC (10);
   if (p == NULL)
     merror ("malloc (10) failed.");
 
-  p = realloc (p, 0);
+  p = TEST_REALLOC (p, 0);
   if (p != NULL)
     merror ("realloc (p, 0) failed.");
 
-  p = malloc (0);
+  p = TEST_MALLOC (0);
   if (p == NULL)
     merror ("malloc (0) failed.");
 
-  p = realloc (p, 0);
+  p = TEST_REALLOC (p, 0);
   if (p != NULL)
     merror ("realloc (p, 0) failed.");
 
-  q = malloc (256);
+  q = TEST_MALLOC (256);
   if (q == NULL)
     merror ("malloc (256) failed.");
 
-  p = malloc (512);
+  p = TEST_MALLOC (512);
   if (p == NULL)
     merror ("malloc (512) failed.");
 
@@ -96,7 +98,7 @@  do_test (void)
   DIAG_POP_NEEDS_COMMENT;
 #endif
 
-  p = malloc (512);
+  p = TEST_MALLOC (512);
   if (p == NULL)
     merror ("malloc (512) failed.");
 
diff --git a/malloc/tst-malloc-too-large.c b/malloc/tst-malloc-too-large.c
index 8e9e0d5fa2..d51e9a90be 100644
--- a/malloc/tst-malloc-too-large.c
+++ b/malloc/tst-malloc-too-large.c
@@ -43,6 +43,7 @@ 
 #include <unistd.h>
 #include <sys/param.h>
 
+#include "tst-malloc-aux.h"
 
 /* This function prepares for each 'too-large memory allocation' test by
    performing a small successful malloc/free and resetting errno prior to
@@ -50,7 +51,7 @@ 
 static void
 test_setup (void)
 {
-  void *volatile ptr = malloc (16);
+  void *volatile ptr = TEST_MALLOC (16);
   TEST_VERIFY_EXIT (ptr != NULL);
   free (ptr);
   errno = 0;
@@ -78,19 +79,19 @@  test_large_allocations (size_t size)
      that they fail.  */
   DIAG_IGNORE_NEEDS_COMMENT (7, "-Walloc-size-larger-than=");
 #endif
-  TEST_VERIFY (malloc (size) == NULL);
+  TEST_VERIFY (TEST_MALLOC (size) == NULL);
 #if __GNUC_PREREQ (7, 0)
   DIAG_POP_NEEDS_COMMENT;
 #endif
   TEST_VERIFY (errno == ENOMEM);
 
-  ptr_to_realloc = malloc (16);
+  ptr_to_realloc = TEST_MALLOC (16);
   TEST_VERIFY_EXIT (ptr_to_realloc != NULL);
   test_setup ();
 #if __GNUC_PREREQ (7, 0)
   DIAG_IGNORE_NEEDS_COMMENT (7, "-Walloc-size-larger-than=");
 #endif
-  TEST_VERIFY (realloc (ptr_to_realloc, size) == NULL);
+  TEST_VERIFY (TEST_REALLOC (ptr_to_realloc, size) == NULL);
 #if __GNUC_PREREQ (7, 0)
   DIAG_POP_NEEDS_COMMENT;
 #endif
@@ -109,14 +110,14 @@  test_large_allocations (size_t size)
     if ((size % nmemb) == 0)
       {
         test_setup ();
-        TEST_VERIFY (calloc (nmemb, size / nmemb) == NULL);
+        TEST_VERIFY (TEST_CALLOC (nmemb, size / nmemb) == NULL);
         TEST_VERIFY (errno == ENOMEM);
 
         test_setup ();
-        TEST_VERIFY (calloc (size / nmemb, nmemb) == NULL);
+        TEST_VERIFY (TEST_CALLOC (size / nmemb, nmemb) == NULL);
         TEST_VERIFY (errno == ENOMEM);
 
-        ptr_to_realloc = malloc (16);
+        ptr_to_realloc = TEST_MALLOC (16);
         TEST_VERIFY_EXIT (ptr_to_realloc != NULL);
         test_setup ();
         TEST_VERIFY (reallocarray (ptr_to_realloc, nmemb, size / nmemb) == NULL);
@@ -131,7 +132,7 @@  test_large_allocations (size_t size)
   DIAG_POP_NEEDS_COMMENT;
 #endif
 
-        ptr_to_realloc = malloc (16);
+        ptr_to_realloc = TEST_MALLOC (16);
         TEST_VERIFY_EXIT (ptr_to_realloc != NULL);
         test_setup ();
         TEST_VERIFY (reallocarray (ptr_to_realloc, size / nmemb, nmemb) == NULL);
@@ -199,7 +200,7 @@  test_large_aligned_allocations (size_t size)
 #if __GNUC_PREREQ (7, 0)
 	  DIAG_IGNORE_NEEDS_COMMENT (7, "-Walloc-size-larger-than=");
 #endif
-          TEST_VERIFY (aligned_alloc (align, size) == NULL);
+          TEST_VERIFY (TEST_ALIGNED_ALLOC (align, size) == NULL);
 #if __GNUC_PREREQ (7, 0)
 	  DIAG_POP_NEEDS_COMMENT;
 #endif
diff --git a/malloc/tst-malloc.c b/malloc/tst-malloc.c
index f7a6e4654c..bbe96b3aa3 100644
--- a/malloc/tst-malloc.c
+++ b/malloc/tst-malloc.c
@@ -22,6 +22,8 @@ 
 #include <libc-diag.h>
 #include <time.h>
 
+#include "tst-malloc-aux.h"
+
 static int errors = 0;
 
 static void
@@ -47,7 +49,7 @@  do_test (void)
      that they fail.  */
   DIAG_IGNORE_NEEDS_COMMENT (7, "-Walloc-size-larger-than=");
 #endif
-  p = malloc (-1);
+  p = TEST_MALLOC (-1);
   DIAG_POP_NEEDS_COMMENT;
   save = errno;
 
@@ -57,24 +59,24 @@  do_test (void)
   if (p == NULL && save != ENOMEM)
     merror ("errno is not set correctly");
 
-  p = malloc (10);
+  p = TEST_MALLOC (10);
   if (p == NULL)
     merror ("malloc (10) failed.");
 
   /* realloc (p, 0) == free (p).  */
-  p = realloc (p, 0);
+  p = TEST_REALLOC (p, 0);
   if (p != NULL)
     merror ("realloc (p, 0) failed.");
 
-  p = malloc (0);
+  p = TEST_MALLOC (0);
   if (p == NULL)
     merror ("malloc (0) failed.");
 
-  p = realloc (p, 0);
+  p = TEST_REALLOC (p, 0);
   if (p != NULL)
     merror ("realloc (p, 0) failed.");
 
-  p = malloc (513 * 1024);
+  p = TEST_MALLOC (513 * 1024);
   if (p == NULL)
     merror ("malloc (513K) failed.");
 
@@ -84,7 +86,7 @@  do_test (void)
      that they fail.  */
   DIAG_IGNORE_NEEDS_COMMENT (7, "-Walloc-size-larger-than=");
 #endif
-  q = malloc (-512 * 1024);
+  q = TEST_MALLOC (-512 * 1024);
   DIAG_POP_NEEDS_COMMENT;
   if (q != NULL)
     merror ("malloc (-512K) succeeded.");
diff --git a/malloc/tst-realloc.c b/malloc/tst-realloc.c
index f50499ecb1..d247c45ea7 100644
--- a/malloc/tst-realloc.c
+++ b/malloc/tst-realloc.c
@@ -23,6 +23,8 @@ 
 #include <libc-diag.h>
 #include <support/check.h>
 
+#include "tst-malloc-aux.h"
+
 static int
 do_test (void)
 {
@@ -39,7 +41,7 @@  do_test (void)
      that they fail.  */
   DIAG_IGNORE_NEEDS_COMMENT (7, "-Walloc-size-larger-than=");
 #endif
-  p = realloc (NULL, -1);
+  p = TEST_REALLOC (NULL, -1);
   DIAG_POP_NEEDS_COMMENT;
   save = errno;
 
@@ -53,7 +55,7 @@  do_test (void)
   errno = 0;
 
   /* realloc (NULL, ...) behaves similarly to malloc (C89).  */
-  p = realloc (NULL, 10);
+  p = TEST_REALLOC (NULL, 10);
   save = errno;
 
   if (p == NULL)
@@ -61,12 +63,12 @@  do_test (void)
 
   free (p);
 
-  p = calloc (20, 1);
+  p = TEST_CALLOC (20, 1);
   if (p == NULL)
     FAIL_EXIT1 ("calloc (20, 1) failed.");
 
   /* Check increasing size preserves contents (C89).  */
-  p = realloc (p, 200);
+  p = TEST_REALLOC (p, 200);
   if (p == NULL)
     FAIL_EXIT1 ("realloc (p, 200) failed.");
 
@@ -84,14 +86,14 @@  do_test (void)
 
   free (p);
 
-  p = realloc (NULL, 100);
+  p = TEST_REALLOC (NULL, 100);
   if (p == NULL)
     FAIL_EXIT1 ("realloc (NULL, 100) failed.");
 
   memset (p, 0xff, 100);
 
   /* Check decreasing size preserves contents (C89).  */
-  p = realloc (p, 16);
+  p = TEST_REALLOC (p, 16);
   if (p == NULL)
     FAIL_EXIT1 ("realloc (p, 16) failed.");
 
@@ -114,7 +116,7 @@  do_test (void)
      that they fail.  */
   DIAG_IGNORE_NEEDS_COMMENT (7, "-Walloc-size-larger-than=");
 #endif
-  c = realloc (p, -1);
+  c = TEST_REALLOC (p, -1);
   DIAG_POP_NEEDS_COMMENT;
   if (c != NULL)
     FAIL_EXIT1 ("realloc (p, -1) succeeded.");
@@ -132,12 +134,12 @@  do_test (void)
     FAIL_EXIT1 ("first 16 bytes were not correct after failed realloc");
 
   /* realloc (p, 0) frees p (C89) and returns NULL (glibc).  */
-  p = realloc (p, 0);
+  p = TEST_REALLOC (p, 0);
   if (p != NULL)
     FAIL_EXIT1 ("realloc (p, 0) returned non-NULL.");
 
   /* realloc (NULL, 0) acts like malloc (0) (glibc).  */
-  p = realloc (NULL, 0);
+  p = TEST_REALLOC (NULL, 0);
   if (p == NULL)
     FAIL_EXIT1 ("realloc (NULL, 0) returned NULL.");
 
@@ -147,14 +149,14 @@  do_test (void)
      space to expand in the chunk.  */
   for (size_t sz = 3; sz < 256 * 1024; sz += 2048)
     {
-      p = realloc (NULL, sz);
+      p = TEST_REALLOC (NULL, sz);
       if (p == NULL)
 	FAIL_EXIT1 ("realloc (NULL, %zu) returned NULL.", sz);
       size_t newsz = malloc_usable_size (p);
       printf ("size: %zu, usable size: %zu, extra: %zu\n",
 	      sz, newsz, newsz - sz);
       uintptr_t oldp = (uintptr_t) p;
-      void *new_p = realloc (p, newsz);
+      void *new_p = TEST_REALLOC (p, newsz);
       if ((uintptr_t) new_p != oldp)
 	FAIL_EXIT1 ("Expanding (%zu bytes) to usable size (%zu) moved block",
 		    sz, newsz);