[v3] aligned_alloc: conform to C17

Message ID xn1qli1zia.fsf@greed.delorie.com
State Superseded
Headers
Series [v3] aligned_alloc: conform to C17 |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

DJ Delorie March 21, 2023, 4:47 a.m. UTC
  Changes from V2:

* Simplified debug_malloc (else propogating "check needed" everywhere*
  *made it excessively messy, and technically difficult).
* Added a test case.
* Removed new DSO symbols.

References:
https://patchwork.sourceware.org/project/glibc/patch/33ec9e0c1e587813b90e8aa771c2c8e6e379dd48.camel@posteo.net/
https://sourceware.org/bugzilla/show_bug.cgi?id=20137
https://sourceware.org/pipermail/libc-alpha/2023-February/145858.html

The memory.texi portion matches Martin's proposed patch.

man page portion, quoted to avoid CI/CD issues (I can send an official
patch separately after the glibc patch is applied):

> diff --git a/man3/posix_memalign.3 b/man3/posix_memalign.3
> index f5d6618b7..a73ff0421 100644
> --- a/man3/posix_memalign.3
> +++ b/man3/posix_memalign.3
> @@ -91,9 +91,8 @@ The function
>  is the same as
>  .BR memalign (),
>  except for the added restriction that
> -.I size
> -should be a multiple of
> -.IR alignment .
> +.I alignment
> +must be a power of two.
>  .PP
>  The obsolete function
>  .BR valloc ()

From 7139d1e689032249e6de223b3867c6b802c37ea1 Mon Sep 17 00:00:00 2001
From: DJ Delorie <dj@redhat.com>
Date: Tue, 21 Mar 2023 00:46:43 -0400
Subject: aligned_alloc: conform to C17

This patch adds the strict checking for power-of-two alignments
in aligned_alloc(), and updates the manual accordingly.
  

Comments

Florian Weimer March 21, 2023, 5:28 p.m. UTC | #1
* DJ Delorie via Libc-alpha:

> +/* For ISO C11.  */
> +static void *
> +__libc_aligned_alloc (size_t alignment, size_t bytes)
> +{

> +weak_alias (__libc_aligned_alloc, aligned_alloc)

I'm surprised that this even compiles.  This sort of makes the
function static and gives it external linkage.

Is anything calling __libc_aligned_alloc?

Perhaps you could use this?

  static void *
  weak_function
  aligned_alloc (size_t alignment, size_t bytes)
  {

Assuming that there are no internal callers.
  
DJ Delorie March 21, 2023, 5:47 p.m. UTC | #2
Florian Weimer <fw@deneb.enyo.de> writes:
> I'm surprised that this even compiles.  This sort of makes the
> function static and gives it external linkage.

It almost does the same thing as your example, but follows the
internal/external paradigm we've been using for all the other functions.

(in your example, just drop the "static", then it's the same)

And yes, it took Carlos and I a while to go through all this linkage
stuff to make sure we understood it.
  
Florian Weimer March 21, 2023, 7:08 p.m. UTC | #3
* DJ Delorie:

> Florian Weimer <fw@deneb.enyo.de> writes:
>> I'm surprised that this even compiles.  This sort of makes the
>> function static and gives it external linkage.
>
> It almost does the same thing as your example, but follows the
> internal/external paradigm we've been using for all the other functions.
>
> (in your example, just drop the "static", then it's the same)
>
> And yes, it took Carlos and I a while to go through all this linkage
> stuff to make sure we understood it.

Yeah, the static was wrong.  I think you should avoid the alias if
there aren't any internal calls.
  
DJ Delorie March 21, 2023, 8:21 p.m. UTC | #4
Florian Weimer <fw@deneb.enyo.de> writes:
> Perhaps you could use this?

One other difference that isn't obvious in the patch...

the weak alias for aligned_alloc is inside a conditional:

#if IS_IN (libc)
. . .
weak_alias (__libc_aligned_alloc, aligned_alloc)
. . .
#endif

I don't know what, if anything uses malloc.c without being in glibc, but
if such a thing existed, it would break if aligned_alloc weren't inside
this conditional.

Or the conditional could be removed?
  
Florian Weimer March 21, 2023, 8:43 p.m. UTC | #5
* DJ Delorie:

> Florian Weimer <fw@deneb.enyo.de> writes:
>> Perhaps you could use this?
>
> One other difference that isn't obvious in the patch...
>
> the weak alias for aligned_alloc is inside a conditional:
>
> #if IS_IN (libc)
> . . .
> weak_alias (__libc_aligned_alloc, aligned_alloc)
> . . .
> #endif
>
> I don't know what, if anything uses malloc.c without being in glibc, but
> if such a thing existed, it would break if aligned_alloc weren't inside
> this conditional.
>
> Or the conditional could be removed?

Siddhesh added it for libc_malloc_debug, it has !IS_IN (libc).
Surely we want libc_malloc_debug to keep defining aligned_alloc,
so this shouldn't be conditional on IS_IN (libc).
  

Patch

diff --git a/malloc/Makefile b/malloc/Makefile
index dfb51d344c..691714fb52 100644
--- a/malloc/Makefile
+++ b/malloc/Makefile
@@ -43,10 +43,12 @@  tests := mallocbug tst-malloc tst-valloc tst-calloc tst-obstack \
 	 tst-tcfree1 tst-tcfree2 tst-tcfree3 \
 	 tst-safe-linking \
 	 tst-mallocalign1 \
+	 tst-aligned-alloc \
 
 tests-static := \
 	 tst-interpose-static-nothread \
-	 tst-interpose-static-thread
+	 tst-interpose-static-thread \
+	 tst-aligned-alloc-static
 
 # Test for the malloc_set_state symbol removed in glibc 2.25.
 ifeq ($(have-GLIBC_2.23)$(build-shared),yesyes)
diff --git a/malloc/malloc-debug.c b/malloc/malloc-debug.c
index 3867d15698..da9d2340d3 100644
--- a/malloc/malloc-debug.c
+++ b/malloc/malloc-debug.c
@@ -299,7 +299,14 @@  __debug_memalign (size_t alignment, size_t bytes)
   return _debug_mid_memalign (alignment, bytes, RETURN_ADDRESS (0));
 }
 strong_alias (__debug_memalign, memalign)
-strong_alias (__debug_memalign, aligned_alloc)
+static void *
+__debug_aligned_alloc (size_t alignment, size_t bytes)
+{
+  if (!powerof2 (alignment) || alignment == 0)
+    return NULL;
+  return _debug_mid_memalign (alignment, bytes, RETURN_ADDRESS (0));
+}
+strong_alias (__debug_aligned_alloc, aligned_alloc)
 
 static void *
 __debug_pvalloc (size_t bytes)
diff --git a/malloc/malloc.c b/malloc/malloc.c
index 76c50e3f58..101d257287 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -3509,6 +3509,26 @@  __libc_memalign (size_t alignment, size_t bytes)
   void *address = RETURN_ADDRESS (0);
   return _mid_memalign (alignment, bytes, address);
 }
+libc_hidden_def (__libc_memalign)
+
+/* For ISO C11.  */
+static void *
+__libc_aligned_alloc (size_t alignment, size_t bytes)
+{
+  if (!__malloc_initialized)
+    ptmalloc_init ();
+
+  /* Similar to memalign, but ISO C17 requires an error for invalid
+     alignments.  Valid alignments are non-negative powers of two.  */
+  if (!powerof2 (alignment) || alignment == 0)
+    {
+      __set_errno (EINVAL);
+      return 0;
+    }
+
+  void *address = RETURN_ADDRESS (0);
+  return _mid_memalign (alignment, bytes, address);
+}
 
 static void *
 _mid_memalign (size_t alignment, size_t bytes, void *address)
@@ -3567,9 +3587,6 @@  _mid_memalign (size_t alignment, size_t bytes, void *address)
           ar_ptr == arena_for_chunk (mem2chunk (p)));
   return tag_new_usable (p);
 }
-/* For ISO C11.  */
-weak_alias (__libc_memalign, aligned_alloc)
-libc_hidden_def (__libc_memalign)
 
 void *
 __libc_valloc (size_t bytes)
@@ -5903,6 +5920,7 @@  weak_alias (__libc_mallinfo, mallinfo)
 strong_alias (__libc_mallinfo2, __mallinfo2)
 weak_alias (__libc_mallinfo2, mallinfo2)
 strong_alias (__libc_mallopt, __mallopt) weak_alias (__libc_mallopt, mallopt)
+weak_alias (__libc_aligned_alloc, aligned_alloc)
 
 weak_alias (__malloc_stats, malloc_stats)
 weak_alias (__malloc_usable_size, malloc_usable_size)
diff --git a/malloc/tst-aligned-alloc-static.c b/malloc/tst-aligned-alloc-static.c
new file mode 100644
index 0000000000..d504473094
--- /dev/null
+++ b/malloc/tst-aligned-alloc-static.c
@@ -0,0 +1 @@ 
+#include "tst-aligned-alloc.c"
diff --git a/malloc/tst-aligned-alloc.c b/malloc/tst-aligned-alloc.c
new file mode 100644
index 0000000000..d6739376d4
--- /dev/null
+++ b/malloc/tst-aligned-alloc.c
@@ -0,0 +1,56 @@ 
+/* Copyright (C) 2023 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/>.  */
+
+#include <errno.h>
+#include <malloc.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <libc-diag.h>
+#include <support/check.h>
+
+static int
+do_test (void)
+{
+  void *p1;
+  void *p2;
+  void *p3;
+
+  errno = 0;
+
+  p1 = aligned_alloc (64, 64);
+
+  if (p1 == NULL)
+    FAIL_EXIT1 ("aligned_alloc(64, 64) failed");
+
+  p2 = aligned_alloc (65, 64);
+
+  if (p2 != NULL)
+    FAIL_EXIT1 ("aligned_alloc(65, 64) did not fail");
+
+  p3 = aligned_alloc (0, 64);
+
+  if (p3 != NULL)
+    FAIL_EXIT1 ("aligned_alloc(0, 64) did not fail");
+
+  free (p1);
+  return 0;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
diff --git a/manual/memory.texi b/manual/memory.texi
index 9d3398a326..8952ff2bfa 100644
--- a/manual/memory.texi
+++ b/manual/memory.texi
@@ -995,7 +995,7 @@  power of two than that, use @code{aligned_alloc} or @code{posix_memalign}.
 @c Alias to memalign.
 The @code{aligned_alloc} function allocates a block of @var{size} bytes whose
 address is a multiple of @var{alignment}.  The @var{alignment} must be a
-power of two and @var{size} must be a multiple of @var{alignment}.
+power of two.
 
 The @code{aligned_alloc} function returns a null pointer on error and sets
 @code{errno} to one of the following values: