[libc++] Improve aligned allocation support with picolibc. #96086
Conversation
picolibc has two memory allocators, one of which doesn't provide posix_memalign(). But both of them provide C11 aligned_alloc(). Unfortunately the libc++ test support code has no option to use aligned_alloc, only posix_memalign or MSVC-style _aligned_malloc. This patch modifies the aligned allocation in libc++ test support to use the __libcpp_aligned_alloc helper function already defined in <include/__memory>, if it's available. That already has an option to use aligned_alloc and knows how to use it right. Also added version detection for picolibc in <include/__config>, so that __libcpp_aligned_alloc can switch to aligned_alloc() if that's a better choice than posix_memalign. Finally, I couldn't resist renaming the test support function `alocate_aligned_impl` so that it spells 'allocate' correctly. (Keeping track of identifiers in software is hard enough without also having to remember which ones have which spelling mistakes.)
llvmbot
added
the
libc++
|
I wondered if I should go further than this in reworking I've been conservative for the moment, because I'm not set up to test on all of the affected platforms. But I'm happy to change the patch to be more drastic if reviewers think that's a good idea! |
|
@llvm/pr-subscribers-libcxx Author: Simon Tatham (statham-arm) Changespicolibc has two memory allocators, one of which doesn't provide posix_memalign(). But both of them provide C11 aligned_alloc(). Unfortunately the libc++ test support code has no option to use aligned_alloc, only posix_memalign or MSVC-style _aligned_malloc. This patch modifies the aligned allocation in libc++ test support to use the __libcpp_aligned_alloc helper function already defined in <include/__memory>, if it's available. That already has an option to use aligned_alloc and knows how to use it right. Also added version detection for picolibc in <include/__config>, so that __libcpp_aligned_alloc can switch to aligned_alloc() if that's a better choice than posix_memalign. Finally, I couldn't resist renaming the test support function Full diff: https://github.com/llvm/llvm-project/pull/96086.diff 3 Files Affected:
diff --git a/libcxx/include/__config b/libcxx/include/__config
index dfb14fd6a380c..6fd668ae45b46 100644
--- a/libcxx/include/__config
+++ b/libcxx/include/__config
@@ -679,6 +679,9 @@ typedef __char32_t char32_t;
# define _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
# endif
+// _LIBCPP_HAS_NO_C11_ALIGNED_ALLOC should be defined on platforms
+// where you would expect C11 aligned_alloc() to exist but it doesn't.
+
// It is not yet possible to use aligned_alloc() on all Apple platforms since
// 10.15 was the first version to ship an implementation of aligned_alloc().
# if defined(__APPLE__)
@@ -691,6 +694,16 @@ typedef __char32_t char32_t;
# define _LIBCPP_HAS_NO_C11_ALIGNED_ALLOC
# endif
+// _LIBCPP_HAS_C11_ALIGNED_ALLOC may be defined on platforms where C11
+// aligned_alloc() is known to exist even if you might not expect it.
+
+// picolibc has two memory allocators, one of which doesn't provide
+// posix_memalign(). But they both reliably provide C11
+// aligned_alloc(), at least since 1.4.4.
+# if defined(__PICOLIBC__) && (__PICOLIBC__ > 1 || (__PICOLIBC__ == 1 && (__PICOLIBC_MINOR__ > 4 || (__PICOLIBC_MINOR__ == 4 && __PICOLIBC_PATCHLEVEL__ >= 4))))
+# define _LIBCPP_HAS_C11_ALIGNED_ALLOC
+# endif
+
# if defined(__APPLE__) || defined(__FreeBSD__)
# define _LIBCPP_HAS_DEFAULTRUNELOCALE
# endif
diff --git a/libcxx/include/__memory/aligned_alloc.h b/libcxx/include/__memory/aligned_alloc.h
index cb424328bcafc..f4bef48cd7f55 100644
--- a/libcxx/include/__memory/aligned_alloc.h
+++ b/libcxx/include/__memory/aligned_alloc.h
@@ -30,7 +30,7 @@ _LIBCPP_BEGIN_NAMESPACE_STD
inline _LIBCPP_HIDE_FROM_ABI void* __libcpp_aligned_alloc(std::size_t __alignment, std::size_t __size) {
# if defined(_LIBCPP_MSVCRT_LIKE)
return ::_aligned_malloc(__size, __alignment);
-# elif _LIBCPP_STD_VER >= 17 && !defined(_LIBCPP_HAS_NO_C11_ALIGNED_ALLOC)
+# elif defined(_LIBCPP_HAS_C11_ALIGNED_ALLOC) || (_LIBCPP_STD_VER >= 17 && !defined(_LIBCPP_HAS_NO_C11_ALIGNED_ALLOC))
// aligned_alloc() requires that __size is a multiple of __alignment,
// but for C++ [new.delete.general], only states "if the value of an
// alignment argument passed to any of these functions is not a valid
diff --git a/libcxx/test/support/count_new.h b/libcxx/test/support/count_new.h
index 0a95e05b72421..b20157fd0b271 100644
--- a/libcxx/test/support/count_new.h
+++ b/libcxx/test/support/count_new.h
@@ -16,6 +16,8 @@
#include <new>
#include <type_traits>
+#include <__memory/aligned_alloc.h> // reuse aligned allocation helper
+
#include "test_macros.h"
#if defined(TEST_HAS_SANITIZERS)
@@ -455,10 +457,12 @@ void operator delete[](void* p, std::nothrow_t const&) TEST_NOEXCEPT {
# define USE_ALIGNED_ALLOC
# endif
-inline void* alocate_aligned_impl(std::size_t size, std::align_val_t align) {
+inline void* allocate_aligned_impl(std::size_t size, std::align_val_t align) {
const std::size_t alignment = static_cast<std::size_t>(align);
void* ret = nullptr;
-# ifdef USE_ALIGNED_ALLOC
+# ifndef _LIBCPP_HAS_NO_LIBRARY_ALIGNED_ALLOCATION
+ ret = std::__libcpp_aligned_alloc(alignment, size);
+# elif defined USE_ALIGNED_ALLOC
ret = _aligned_malloc(size, alignment);
# else
assert(posix_memalign(&ret, std::max(alignment, sizeof(void*)), size) != EINVAL);
@@ -468,7 +472,9 @@ inline void* alocate_aligned_impl(std::size_t size, std::align_val_t align) {
inline void free_aligned_impl(void* ptr, std::align_val_t) {
if (ptr) {
-# ifdef USE_ALIGNED_ALLOC
+# ifndef _LIBCPP_HAS_NO_LIBRARY_ALIGNED_ALLOCATION
+ std::__libcpp_aligned_free(ptr);
+# elif defined USE_ALIGNED_ALLOC
::_aligned_free(ptr);
# else
::free(ptr);
@@ -479,7 +485,7 @@ inline void free_aligned_impl(void* ptr, std::align_val_t) {
// operator new(size_t, align_val_t[, nothrow_t]) and operator delete(size_t, align_val_t[, nothrow_t])
void* operator new(std::size_t s, std::align_val_t av) TEST_THROW_SPEC(std::bad_alloc) {
getGlobalMemCounter()->alignedNewCalled(s, static_cast<std::size_t>(av));
- void* p = alocate_aligned_impl(s, av);
+ void* p = allocate_aligned_impl(s, av);
if (p == nullptr)
detail::throw_bad_alloc_helper();
return p;
@@ -495,7 +501,7 @@ void* operator new(std::size_t s, std::align_val_t av, std::nothrow_t const&) TE
return nullptr;
}
# endif
- return alocate_aligned_impl(s, av);
+ return allocate_aligned_impl(s, av);
}
void operator delete(void* p, std::align_val_t av) TEST_NOEXCEPT {
@@ -511,7 +517,7 @@ void operator delete(void* p, std::align_val_t av, std::nothrow_t const&) TEST_N
// operator new[](size_t, align_val_t[, nothrow_t]) and operator delete[](size_t, align_val_t[, nothrow_t])
void* operator new[](std::size_t s, std::align_val_t av) TEST_THROW_SPEC(std::bad_alloc) {
getGlobalMemCounter()->alignedNewArrayCalled(s, static_cast<std::size_t>(av));
- void* p = alocate_aligned_impl(s, av);
+ void* p = allocate_aligned_impl(s, av);
if (p == nullptr)
detail::throw_bad_alloc_helper();
return p;
@@ -527,7 +533,7 @@ void* operator new[](std::size_t s, std::align_val_t av, std::nothrow_t const&)
return nullptr;
}
# endif
- return alocate_aligned_impl(s, av);
+ return allocate_aligned_impl(s, av);
}
void operator delete[](void* p, std::align_val_t av) TEST_NOEXCEPT {
|
| #include <new> | ||
| #include <type_traits> | ||
|
|
||
| #include <__memory/aligned_alloc.h> // reuse aligned allocation helper |
There was a problem hiding this comment.
We don't want to reuse libc++ internal functionality in the test suite. One reason is that our test suite is used by other implementations of the C++ stdlib that are not libc++, and those don't have __memory/aligned_alloc.h. Another reason is that we should avoid relying on library functionality that we're testing in the implementation of the test suite, because that's kinda circular.
There was a problem hiding this comment.
On the other hand, writing a separate set of ifdefs in two places intended to do the same job is a recipe for updating one and forgetting to update the other, so they get out of sync!
What's your alternative? I accept that we can't depend on a libc++ internal header if we're not always testing libc++ (which I didn't know, thanks). Can we move the huge pile of "guess which aligned allocation API to use" ifdefs to a place where both the tests and libc++ can include it?
There was a problem hiding this comment.
Frankly, I would just duplicate a bit of logic. I don't think it makes sense to create a lot of complexity just to solve this one instance of code duplication.
| # if defined(_LIBCPP_MSVCRT_LIKE) | ||
| return ::_aligned_malloc(__size, __alignment); | ||
| # elif _LIBCPP_STD_VER >= 17 && !defined(_LIBCPP_HAS_NO_C11_ALIGNED_ALLOC) | ||
| # elif defined(_LIBCPP_HAS_C11_ALIGNED_ALLOC) || (_LIBCPP_STD_VER >= 17 && !defined(_LIBCPP_HAS_NO_C11_ALIGNED_ALLOC)) |
There was a problem hiding this comment.
Is there a reason for introducing a new macro named _LIBCPP_HAS_C11_ALIGNED_ALLOC? It should suffice not to define _LIBCPP_HAS_NO_C11_ALIGNED_ALLOC on picolibc, no?
There was a problem hiding this comment.
The latter is already not defined, so this code will select C11 aligned_alloc with picolibc as long as _LIBCPP_STD_VER >= 17.
This change is intended to make it select aligned_alloc even when that's not true: we can't fall back to any other API because it doesn't reliably have them.
There was a problem hiding this comment.
This change is intended to make it select aligned_alloc even when that's not true
You mean use aligned_alloc in C++ < 17? I'd rather not do that. We want to avoid relying on non-Standard behavior (or introducing any complexity for that purpose), and we can't be guaranteed that the C library provides aligned_alloc before C++17.
I think it's reasonable to require that picolibc users compile with C++ >= 17 when they use an allocator that doesn't provide posix_memalign.
|
If both allocators already have aligned allocation support, why can't the second one simply implement the posix interface? |
By "simply", it sounds as if you mean "the picolibc maintainer should add an extra function, so that libc++ doesn't have to choose a working one of its existing I see that that's simple from the libc++ end, but if I took that idea to the picolibc project, I wouldn't be at all surprised to find them asking the same question in reverse – why can't libc++ "simply" use the working code it already has? All the world's not POSIX. But all the world is C, if you're implementing a C++ library. So, to my way of thinking, it's better to use functionality in the C standard than the POSIX standard if possible, and depend only on POSIX for things you can't do any other way. That's why I'm in favour of fixing this on the libc++ side. In an application, it seems to me this wouldn't have been a question at all. If you're linking programs, you don't have to keep a database of which C libraries have which functions; you'd just do a cmake test that finds out whether each function is there or not, and choose a I guess we can't reliably do checks of that kind in this context, or we'd already be doing it. If you'd rather not complicate the ifdefs further, how about adding a manual configuration option to override them? That way, if I'm trying to build for a libc that libc++ doesn't know about, I can tell it on the cmake command line what functions that libc provides. |
|
I don't think "why doesn't libc++ just add another #ifdef" is a reasonable response. While POSIX isn't available everywhere, it is still ubiquitous. There are only roughly 2x as many users of CC @mplatings |
FWIW, I personally agree with that. My preference is to try depending on C instead of POSIX whenever we can. |
|
The picolibc maintainer has merged a PR adding |
picolibc has two memory allocators, one of which doesn't provide posix_memalign(). But both of them provide C11 aligned_alloc(). Unfortunately the libc++ test support code has no option to use aligned_alloc, only posix_memalign or MSVC-style _aligned_malloc.
This patch modifies the aligned allocation in libc++ test support to use the __libcpp_aligned_alloc helper function already defined in <include/__memory>, if it's available. That already has an option to use aligned_alloc and knows how to use it right.
Also added version detection for picolibc in <include/__config>, so that __libcpp_aligned_alloc can switch to aligned_alloc() if that's a better choice than posix_memalign.
Finally, I couldn't resist renaming the test support function
alocate_aligned_implso that it spells 'allocate' correctly. (Keeping track of identifiers in software is hard enough without also having to remember which ones have which spelling mistakes.)