[PATCH] Reimplement aligned_alloc

[PATCH] Reimplement aligned_alloc

[Szabolcs Nagy]

The original implementation had multiple issues:

- Only worked when posix_memalign was available (Linux, RTEMS).
- Violated C11 link namespace rules by calling posix_memalign.
- Failed to set errno on error.

These can be fixed by essentially using the same implementation
for aligned_alloc as for memalign, i.e. simply calling _memalign_r
(which is always available and a "more reserved name" although
technically still not in the reserved link namespace, at least
code written in c cannot define a colliding symbol, newlib has
plenty such namespace issues so this is fine).

It is not clear what the right policy is when MALLOC_PROVIDED is set,
currently that does not cover aligned_alloc so it is kept that way.

Tested on aarch64-none-elf
---
 newlib/libc/stdlib/aligned_alloc.c | 62 +++++++++++++++---------------
 1 file changed, 30 insertions(+), 32 deletions(-)

diff --git a/newlib/libc/stdlib/aligned_alloc.c b/newlib/libc/stdlib/aligned_alloc.c
index 88413ce86..feb22c24b 100644
--- a/newlib/libc/stdlib/aligned_alloc.c
+++ b/newlib/libc/stdlib/aligned_alloc.c
@@ -1,38 +1,36 @@
-/*-
- * Copyright (c) 2015 embedded brains GmbH
- * All rights reserved.
- *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions
- * are met:
- * 1. Redistributions of source code must retain the above copyright
- *    notice, this list of conditions and the following disclaimer.
- * 2. Redistributions in binary form must reproduce the above copyright
- *    notice, this list of conditions and the following disclaimer in the
- *    documentation and/or other materials provided with the distribution.
- *
- * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
- * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
- * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
- * ARE DISCLAIMED.  IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
- * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
- * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
- * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
- * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
- * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
- * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
- * SUCH DAMAGE.
- */
+/* C11 aligned_alloc
+   Copyright (c) 2020 Arm Ltd.  All rights reserved.
 
+   SPDX-License-Identifier: BSD-3-Clause
+
+   Redistribution and use in source and binary forms, with or without
+   modification, are permitted provided that the following conditions
+   are met:
+   1. Redistributions of source code must retain the above copyright
+      notice, this list of conditions and the following disclaimer.
+   2. Redistributions in binary form must reproduce the above copyright
+      notice, this list of conditions and the following disclaimer in the
+      documentation and/or other materials provided with the distribution.
+   3. The name of the company may not be used to endorse or promote
+      products derived from this software without specific prior written
+      permission.
+
+   THIS SOFTWARE IS PROVIDED BY ARM LTD ``AS IS'' AND ANY EXPRESS OR IMPLIED
+   WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
+   MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
+   IN NO EVENT SHALL ARM LTD BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED
+   TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
+   PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+   LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
+   NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+   SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */
+
+#include <reent.h>
 #include <stdlib.h>
 
 void *
-aligned_alloc(size_t alignment, size_t size)
+aligned_alloc (size_t align, size_t size)
 {
-	void *p;
-	int error;
-
-	error = posix_memalign(&p, alignment, size);
-
-	return (error == 0 ? p : NULL);
+  return _memalign_r (_REENT, align, size);
 }
-- 
2.17.1

Re: [PATCH] Reimplement aligned_alloc

[Corinna Vinschen]

On May 19 10:52, Szabolcs Nagy wrote:
> The original implementation had multiple issues:
> 
> - Only worked when posix_memalign was available (Linux, RTEMS).
> - Violated C11 link namespace rules by calling posix_memalign.
> - Failed to set errno on error.
> 
> These can be fixed by essentially using the same implementation
> for aligned_alloc as for memalign, i.e. simply calling _memalign_r
> (which is always available and a "more reserved name" although
> technically still not in the reserved link namespace, at least
> code written in c cannot define a colliding symbol, newlib has
> plenty such namespace issues so this is fine).
> 
> It is not clear what the right policy is when MALLOC_PROVIDED is set,
> currently that does not cover aligned_alloc so it is kept that way.
> 
> Tested on aarch64-none-elf
> ---
>  newlib/libc/stdlib/aligned_alloc.c | 62 +++++++++++++++---------------
>  1 file changed, 30 insertions(+), 32 deletions(-)

Pushed.


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat

Re: [PATCH] Reimplement aligned_alloc

[Sebastian Huber]

Hello,

On 19/05/2020 11:52, Szabolcs Nagy wrote:
> - Failed to set errno on error.
why should aligned_alloc() set errno on error?

Re: [PATCH] Reimplement aligned_alloc

[Joel Sherrill]

On Tue, May 19, 2020 at 12:41 PM Sebastian Huber <
sebastian.huber@embedded-brains.de> wrote:

> Hello,
>
> On 19/05/2020 11:52, Szabolcs Nagy wrote:
> > - Failed to set errno on error.
> why should aligned_alloc() set errno on error?
>

Just to provide references.

https://en.cppreference.com/w/c/memory/aligned_alloc  does not mention
setting errno

https://pubs.opengroup.org/onlinepubs/9699919799/functions/posix_memalign.html
for posix_memalign does include errno.

--joel

Re: [PATCH] Reimplement aligned_alloc

[Szabolcs Nagy]

The 05/19/2020 19:41, Sebastian Huber wrote:
> Hello,
> 
> On 19/05/2020 11:52, Szabolcs Nagy wrote:
> > - Failed to set errno on error.
> why should aligned_alloc() set errno on error?

iso c allows standard api calls to set errno
(except not to 0) but usually does not specify
error conditions, however posix does and
requires allocation functions to set errno
to ENOMEM on failure.

posix is not yet aligned with c11 so there is
no errno specification for aligned_alloc but
that does not mean newlib should make it
deliberately inconsistent with other allocation
functions, users will try to use perror or
strerror when calls fail that may internally
use aligned_alloc and expect reasonable error
message.

Re: [PATCH] Reimplement aligned_alloc

[Brian Inglis]

>> On 19/05/2020 11:52, Szabolcs Nagy wrote:
>>> - Failed to set errno on error.

> The 05/19/2020 19:41, Sebastian Huber wrote:
>> why should aligned_alloc() set errno on error?

On 2020-05-20 02:54, Szabolcs Nagy wrote:
> iso c allows standard api calls to set errno
> (except not to 0) but usually does not specify
> error conditions, however posix does and
> requires allocation functions to set errno
> to ENOMEM on failure.
> 
> posix is not yet aligned with c11 so there is
> no errno specification for aligned_alloc but
> that does not mean newlib should make it
> deliberately inconsistent with other allocation
> functions, users will try to use perror or
> strerror when calls fail that may internally
> use aligned_alloc and expect reasonable error
> message.

IEC 9899-2011[2012] specifies no errno values for any of the 7.22.3 Memory
management functions, and under Annex J.1 Unspecified behaviour and J.2
Undefined behaviour, omits aligned_alloc from issues of related functions, but
under J.2 specifies as a portability issue only:

-- The alignment requested of the aligned_alloc function is not valid or not
supported by the implementation, or the size requested is not an integral
multiple of the alignment (7.22.3.1).

POSIX posix_memalign specifies errno = EINVAL for this case and errno = ENOMEM
for the OoM case.

It seems more useful to set errno to POSIX compatible values when returning a
NULL pointer than attempt nasal demon emission.

-- 
Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada

This email may be disturbing to some readers as it contains
too much technical detail. Reader discretion is advised.
[Data in IEC units and prefixes, physical quantities in SI.]

Re: [PATCH] Reimplement aligned_alloc

[Szabolcs Nagy]

The 05/20/2020 08:32, Brian Inglis wrote:
> >> On 19/05/2020 11:52, Szabolcs Nagy wrote:
> >>> - Failed to set errno on error.
> 
> > The 05/19/2020 19:41, Sebastian Huber wrote:
> >> why should aligned_alloc() set errno on error?
> 
> On 2020-05-20 02:54, Szabolcs Nagy wrote:
> > iso c allows standard api calls to set errno
> > (except not to 0) but usually does not specify
> > error conditions, however posix does and
> > requires allocation functions to set errno
> > to ENOMEM on failure.
> > 
> > posix is not yet aligned with c11 so there is
> > no errno specification for aligned_alloc but
> > that does not mean newlib should make it
> > deliberately inconsistent with other allocation
> > functions, users will try to use perror or
> > strerror when calls fail that may internally
> > use aligned_alloc and expect reasonable error
> > message.
> 
> IEC 9899-2011[2012] specifies no errno values for any of the 7.22.3 Memory
> management functions, and under Annex J.1 Unspecified behaviour and J.2
> Undefined behaviour, omits aligned_alloc from issues of related functions, but
> under J.2 specifies as a portability issue only:
> 
> -- The alignment requested of the aligned_alloc function is not valid or not
> supported by the implementation, or the size requested is not an integral
> multiple of the alignment (7.22.3.1).
> 
> POSIX posix_memalign specifies errno = EINVAL for this case and errno = ENOMEM
> for the OoM case.
> 
> It seems more useful to set errno to POSIX compatible values when returning a
> NULL pointer than attempt nasal demon emission.

note that posix_memalign does not set errno,
but returns an error value.

in principle aligned_alloc could check its
alignement argument too and set errno to
EINVAL, but to me that case seems less
important in practice: it's not a dynamic
runtime failure like running out of memory,
but wrong argument bug in user code that
should be fixed statically instead of
checking errno.

if posix is updated to cover aligned_alloc
and requires EINVAL for this case, then
newlib can be updated.