[libc++] Use aligned_alloc instead of posix_memalign for C++17

[arichardson]

Summary

C++17 defines the C11 aligned_alloc, so we can use that instead of
posix_memalign. This change allows building against picolibc without
defining _DEFAULT_SOURCE/_GNU_SOURCE.
The C11 aligned_alloc function should be available on all supported
non-Windows platforms except for macOS where we need version 10.15.

There is one caveat: aligned_alloc() requires that size is a multiple of
alignment, but [new.delete.general] only states "if the value of an
alignment argument passed to any of these functions is not a valid
alignment value, the behavior is undefined".
To handle calls such as ::operator new(1, std::align_val_t(128)), we
round up size to alignment (and check for wrap-around).
This is required at least for macOS where aligned_alloc(128, 1) returns
an error instead of allocating memory (glibc ignores the specification).

[ldionne]

Thanks, I've been meaning to do that for a while.

[ldionne]

We'll have to figure out why the CI is failing though.

[arichardson]

In D138196#3933915, @ldionne wrote:

We'll have to figure out why the CI is failing though.

One of the apple failures and the AIX one appears to be a flaky timeout (high CPU load?) std/thread/thread.mutex/thread.mutex.requirements/thread.shared_mutex.requirements/thread.shared_mutex.class/try_lock_shared.pass.cpp.

The other ones are real issues: macos10.9 does not appear to have aligned_alloc. What is the preferred way to opt out older OS versions?
Not quite sure why new_faligned_allocation.pass.cpp is failing - will try to debug that one.

[arichardson]

Try to disable aligned_alloc for macOS 10.9

[arichardson]

rebase to prod CI

[ldionne]

libcxx/language.support/support.dynamic/new_faligned_allocation.pass.cpp does this in several places:

::operator new(1, std::align_val_t(128))

Here, we're allocating an object of size 1 with an alignment of 128. However, aligned_alloc requires the size to be an integral multiple of the alignment: https://en.cppreference.com/w/cpp/memory/c/aligned_alloc. It seems like only the Darwin implementation properly checks for this case.

I couldn't find anything in http://eel.is/c++draft/support.dynamic#new.syn that would place a similar restriction on the align_val_t version of operator new -- so I think we technically cannot implement it using aligned_alloc if we stick to the C Standard. I suspect this may be a good candidate for a LWG issue, since I don't think this was designed on purpose.

[arichardson]

In D138196#3944368, @ldionne wrote:

libcxx/language.support/support.dynamic/new_faligned_allocation.pass.cpp does this in several places:

::operator new(1, std::align_val_t(128))

Here, we're allocating an object of size 1 with an alignment of 128. However, aligned_alloc requires the size to be an integral multiple of the alignment: https://en.cppreference.com/w/cpp/memory/c/aligned_alloc. It seems like only the Darwin implementation properly checks for this case.

I couldn't find anything in http://eel.is/c++draft/support.dynamic#new.syn that would place a similar restriction on the align_val_t version of operator new -- so I think we technically cannot implement it using aligned_alloc if we stick to the C Standard. I suspect this may be a good candidate for a LWG issue, since I don't think this was designed on purpose.

Ah I missed that requirement - this is rather unfortunate. It looks like posix_memalign only requires sizeof(void*) alignment, so calling something like ::operator new(1, std::align_val_t(2)) should also fail there?

It sounds to me like we can't use aligned_alloc at all in this case unless we decide to round up __size to __alignment.

[arichardson]

• extend test coverage
• attempt to make it work on macOS.

[arichardson]

libcxx/test/libcxx/language.support/support.dynamic/new_faligned_allocation.pass.cpp

82

This is missing cases where size > alignment, but not a multiple of alignment.

[arichardson]

Handle ::operator new(65, std::align_val_t(32))

[arichardson]

fix typo in comment

libcxx/include/new

350

I'd hope these additional computations can be elided by the compiler in many cases - not sure how often the values are completely dynamic.

[ldionne]

LGTM w/ deployment target change.

libcxx/include/__config

723

I think this actually started in macOS 10.15, it's just that our back-deployment CI does not run for all versions between 10.9 and 10.15.

[ldionne]

Gentle ping before this becomes too hard to rebase :-)

[arichardson]

Rebase and update version guard to macos 10.15

[arichardson]

In D138196#4009494, @ldionne wrote:

Gentle ping before this becomes too hard to rebase :-)

Sorry for the delay, was working on other things for the past few weeks. Will submit this once I have a green CI run. Thanks for the review :)

[arichardson]

rebase for CI

[thakis]

This breaks our build (c++20): https://ci.chromium.org/ui/p/chromium/builders/try/android-binary-size/1337773/overview

Since this includes cstdlib instead of stdlib.h, is :: the right qualification? Should it look in std instead?

[arichardson]

In D138196#4013628, @thakis wrote:

This breaks our build (c++20): https://ci.chromium.org/ui/p/chromium/builders/try/android-binary-size/1337773/overview

Since this includes cstdlib instead of stdlib.h, is :: the right qualification? Should it look in std instead?

cstdlib only has the following using ::aligned_alloc _LIBCPP_USING_IF_EXISTS;, so I assume it would also fail with std::aligned_alloc(). I don't have a chromium checkout, but maybe the C library that your build is using doesn't provide aligned_alloc() ? Or maybe it requires an additional macro to expose it? Otherwise you could add another opt-out case to libcxx/include/__config (like the macos < 10.15 one from this commit).

[thakis]

cstdlib only has the following using ::aligned_alloc _LIBCPP_USING_IF_EXISTS;, so I assume it would also fail with std::aligned_alloc(). I don't have a chromium checkout, but maybe the C library that your build is using doesn't provide aligned_alloc() ? Or maybe it requires an additional macro to expose it? Otherwise you could add another opt-out case to libcxx/include/__config (like the macos < 10.15 one from this commit).

It's broken on android (bionic) and chromeos (some glibc, I think?).

libcxx/test/std/depr/depr.c.headers/stdlib_h.aligned_alloc.compile.pass.cpp claims "::aligned_alloc is available starting with Android P (API 28)". We support API level 24, so we can't use that yet.

So I think this breaks most people doing Android builds, so it'd probably be good if _LIBCPP_HAS_NO_C11_ALIGNED_ALLOC was set correctly automatically for at least Android.

(But we'll just define _LIBCPP_HAS_NO_C11_ALIGNED_ALLOC ourselves for now.)

[gulfem]

This seems to break our toolchain builders for Fuchsia.
https://ci.chromium.org/ui/p/fuchsia/builders/ci/clang_toolchain.prod.core.x64-release-subbuild/b8794045085201621697/overview

[thakis]

Re ChromeOS above: turns out we build some bits for Android in ChromeOS builds, so that's just the android thing again.

In D138196#4019563, @gulfem wrote:

This seems to break our toolchain builders for Fuchsia.
https://ci.chromium.org/ui/p/fuchsia/builders/ci/clang_toolchain.prod.core.x64-release-subbuild/b8794045085201621697/overview

Do you know what the aligned_alloc story in your C library is? Which C runtime library does fuchsia use?

(Chromium's Fuchsia builders are fine with this change fwiw.)

[gulfem]

In D138196#4019614, @thakis wrote:

Re ChromeOS above: turns out we build some bits for Android in ChromeOS builds, so that's just the android thing again.

In D138196#4019563, @gulfem wrote:

This seems to break our toolchain builders for Fuchsia.
https://ci.chromium.org/ui/p/fuchsia/builders/ci/clang_toolchain.prod.core.x64-release-subbuild/b8794045085201621697/overview

Do you know what the aligned_alloc story in your C library is? Which C runtime library does fuchsia use?

(Chromium's Fuchsia builders are fine with this change fwiw.)

It seems like we fixed the issue on the Fuchsia side (https://fuchsia-review.git.corp.google.com/c/fuchsia/+/782063), and I missed that fix because of the holidays.

[smeenai]

In D138196#4019550, @thakis wrote:

cstdlib only has the following using ::aligned_alloc _LIBCPP_USING_IF_EXISTS;, so I assume it would also fail with std::aligned_alloc(). I don't have a chromium checkout, but maybe the C library that your build is using doesn't provide aligned_alloc() ? Or maybe it requires an additional macro to expose it? Otherwise you could add another opt-out case to libcxx/include/__config (like the macos < 10.15 one from this commit).

It's broken on android (bionic) and chromeos (some glibc, I think?).

libcxx/test/std/depr/depr.c.headers/stdlib_h.aligned_alloc.compile.pass.cpp claims "::aligned_alloc is available starting with Android P (API 28)". We support API level 24, so we can't use that yet.

So I think this breaks most people doing Android builds, so it'd probably be good if _LIBCPP_HAS_NO_C11_ALIGNED_ALLOC was set correctly automatically for at least Android.

(But we'll just define _LIBCPP_HAS_NO_C11_ALIGNED_ALLOC ourselves for now.)

CC @danalbert and @rprichard. (We hit the same breakage as well.)

[arichardson]

In D138196#4029503, @smeenai wrote:

In D138196#4019550, @thakis wrote:

cstdlib only has the following using ::aligned_alloc _LIBCPP_USING_IF_EXISTS;, so I assume it would also fail with std::aligned_alloc(). I don't have a chromium checkout, but maybe the C library that your build is using doesn't provide aligned_alloc() ? Or maybe it requires an additional macro to expose it? Otherwise you could add another opt-out case to libcxx/include/__config (like the macos < 10.15 one from this commit).

It's broken on android (bionic) and chromeos (some glibc, I think?).

libcxx/test/std/depr/depr.c.headers/stdlib_h.aligned_alloc.compile.pass.cpp claims "::aligned_alloc is available starting with Android P (API 28)". We support API level 24, so we can't use that yet.

So I think this breaks most people doing Android builds, so it'd probably be good if _LIBCPP_HAS_NO_C11_ALIGNED_ALLOC was set correctly automatically for at least Android.

(But we'll just define _LIBCPP_HAS_NO_C11_ALIGNED_ALLOC ourselves for now.)

CC @danalbert and @rprichard. (We hit the same breakage as well.)

If you give me instructions to test this and the right macro to check I can try to submit a fix, but it will probably be a lot quicker if someone with an existing development setup can add the right line to __config.

[smeenai]

In D138196#4029676, @arichardson wrote:

In D138196#4029503, @smeenai wrote:

In D138196#4019550, @thakis wrote:

cstdlib only has the following using ::aligned_alloc _LIBCPP_USING_IF_EXISTS;, so I assume it would also fail with std::aligned_alloc(). I don't have a chromium checkout, but maybe the C library that your build is using doesn't provide aligned_alloc() ? Or maybe it requires an additional macro to expose it? Otherwise you could add another opt-out case to libcxx/include/__config (like the macos < 10.15 one from this commit).

It's broken on android (bionic) and chromeos (some glibc, I think?).

libcxx/test/std/depr/depr.c.headers/stdlib_h.aligned_alloc.compile.pass.cpp claims "::aligned_alloc is available starting with Android P (API 28)". We support API level 24, so we can't use that yet.

So I think this breaks most people doing Android builds, so it'd probably be good if _LIBCPP_HAS_NO_C11_ALIGNED_ALLOC was set correctly automatically for at least Android.

(But we'll just define _LIBCPP_HAS_NO_C11_ALIGNED_ALLOC ourselves for now.)

CC @danalbert and @rprichard. (We hit the same breakage as well.)

If you give me instructions to test this and the right macro to check I can try to submit a fix, but it will probably be a lot quicker if someone with an existing development setup can add the right line to __config.

D141184

libcxx/include/__config

 #  elif defined(__MVS__)
 #    define _LIBCPP_HAS_NO_LIBRARY_ALIGNED_ALLOCATION
 #  endif
 
 #  if defined(_LIBCPP_HAS_NO_LIBRARY_ALIGNED_ALLOCATION) || (!defined(__cpp_aligned_new) || __cpp_aligned_new < 201606)
 #    define _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
 #  endif
 
+// 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__)
+#    if (defined(__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__) &&                                                     \
+         __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ < 101500)
+#      define _LIBCPP_HAS_NO_C11_ALIGNED_ALLOC
+#    endif
+#  endif
+
 #  if defined(__APPLE__) || defined(__FreeBSD__)
 #    define _LIBCPP_HAS_DEFAULTRUNELOCALE
 #  endif
 
 #  if defined(__APPLE__) || defined(__FreeBSD__) || defined(__sun__)
 #    define _LIBCPP_WCTYPE_IS_MASK
 #  endif
 

libcxx/include/new

 
 #if !defined(_LIBCPP_HAS_NO_LIBRARY_ALIGNED_ALLOCATION)
 // Low-level helpers to call the aligned allocation and deallocation functions
 // on the target platform. This is used to implement libc++'s own memory
 // allocation routines -- if you need to allocate memory inside the library,
 // chances are that you want to use `__libcpp_allocate` instead.
 //
 // Returns the allocated memory, or `nullptr` on failure.
-inline _LIBCPP_INLINE_VISIBILITY
-void* __libcpp_aligned_alloc(std::size_t __alignment, std::size_t __size) {
-#if defined(_LIBCPP_MSVCRT_LIKE)
+inline _LIBCPP_INLINE_VISIBILITY void* __libcpp_aligned_alloc(std::size_t __alignment, std::size_t __size) {
+#  if defined(_LIBCPP_MSVCRT_LIKE)
     return ::_aligned_malloc(__size, __alignment);
-#else
+#  elif _LIBCPP_STD_VER > 14 && !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
+    // alignment value, the behavior is undefined".
+    // To handle calls such as ::operator new(1, std::align_val_t(128)), we
+    // round __size up to the next multiple of __alignment.
+    size_t __rounded_size = (__size + __alignment - 1) & ~(__alignment - 1);
+    // Rounding up could have wrapped around to zero, so we have to add another
+    // max() ternary to the actual call site to avoid succeeded in that case.
+    return ::aligned_alloc(__alignment, __size > __rounded_size ? __size : __rounded_size);
+#  else
     void* __result = nullptr;
     (void)::posix_memalign(&__result, __alignment, __size);
     // If posix_memalign fails, __result is unmodified so we still return `nullptr`.
     return __result;
-#endif
+#  endif
 }
 
 inline _LIBCPP_INLINE_VISIBILITY
 void __libcpp_aligned_free(void* __ptr) {
 #if defined(_LIBCPP_MSVCRT_LIKE)
   ::_aligned_free(__ptr);
 #else
   ::free(__ptr);

libcxx/test/libcxx/language.support/support.dynamic/new_faligned_allocation.pass.cpp

 
 #include <new>
 #include <typeinfo>
 #include <string>
 #include <cassert>
 
 #include "test_macros.h"
 
+static void test_allocations(size_t size, size_t alignment) {
+  {
+    void* ptr = ::operator new(size, std::align_val_t(alignment));
+    assert(ptr);
+    assert(reinterpret_cast<std::uintptr_t>(ptr) % alignment == 0);
+    ::operator delete(ptr, std::align_val_t(alignment));
+  }
+  {
+    void* ptr = ::operator new(size, std::align_val_t(alignment), std::nothrow);
+    assert(ptr);
+    assert(reinterpret_cast<std::uintptr_t>(ptr) % alignment == 0);
+    ::operator delete(ptr, std::align_val_t(alignment), std::nothrow);
+  }
+  {
+    void* ptr = ::operator new[](size, std::align_val_t(alignment));
+    assert(ptr);
+    assert(reinterpret_cast<std::uintptr_t>(ptr) % alignment == 0);
+    ::operator delete[](ptr, std::align_val_t(alignment));
+  }
+  {
+    void* ptr = ::operator new[](size, std::align_val_t(alignment), std::nothrow);
+    assert(ptr);
+    assert(reinterpret_cast<std::uintptr_t>(ptr) % alignment == 0);
+    ::operator delete[](ptr, std::align_val_t(alignment), std::nothrow);
+  }
+}
+
 int main(int, char**) {
   {
     static_assert(std::is_enum<std::align_val_t>::value, "");
     typedef std::underlying_type<std::align_val_t>::type UT;
     static_assert((std::is_same<UT, std::size_t>::value), "");
   }
   {
     static_assert((!std::is_constructible<std::align_val_t, std::size_t>::value), "");
 #if TEST_STD_VER >= 11
     static_assert(!std::is_constructible<std::size_t, std::align_val_t>::value, "");
 #else
     static_assert((std::is_constructible<std::size_t, std::align_val_t>::value), "");
 #endif
   }
   {
     std::align_val_t a = std::align_val_t(0);
     std::align_val_t b = std::align_val_t(32);
     assert(a != b);
     assert(a == std::align_val_t(0));
     assert(b == std::align_val_t(32));
   }
-  {
-    void *ptr = ::operator new(1, std::align_val_t(128));
-    assert(ptr);
-    assert(reinterpret_cast<std::uintptr_t>(ptr) % 128 == 0);
-    ::operator delete(ptr, std::align_val_t(128));
-  }
-  {
-    void *ptr = ::operator new(1, std::align_val_t(128), std::nothrow);
-    assert(ptr);
-    assert(reinterpret_cast<std::uintptr_t>(ptr) % 128 == 0);
-    ::operator delete(ptr, std::align_val_t(128), std::nothrow);
-  }
-  {
-    void *ptr = ::operator new[](1, std::align_val_t(128));
-    assert(ptr);
-    assert(reinterpret_cast<std::uintptr_t>(ptr) % 128 == 0);
-    ::operator delete[](ptr, std::align_val_t(128));
-  }
-  {
-    void *ptr = ::operator new[](1, std::align_val_t(128), std::nothrow);
-    assert(ptr);
-    assert(reinterpret_cast<std::uintptr_t>(ptr) % 128 == 0);
-    ::operator delete[](ptr, std::align_val_t(128), std::nothrow);
-  }
+  // First, check the basic case, a large allocation with alignment==size.
+  test_allocations(64, 64);
+  // Size being a multiple of alignment also needs to be supported.
+  test_allocations(64, 32);
+  // When aligned allocation is implemented using posix_memalign,
+  // that function requires a minimum alignment of sizeof(void*).
+  // Check that we can also create overaligned allocations with
+  // an alignment argument less than sizeof(void*).
+  test_allocations(2, 2);
+  // When implemented using the C11 aligned_alloc() function,
+  // that requires that size be a multiple of alignment.
+  // However, the C++ operator new has no such requirements.
+  // Check that we can create an overaligned allocation that does
+  // adhere to not have this constraint.
+  test_allocations(1, 128);
+  // Finally, test size > alignment, but with size not being
+  // a multiple of alignment.
+  test_allocations(65, 32);
 #ifndef TEST_HAS_NO_RTTI
   {
     // Check that libc++ doesn't define align_val_t in a versioning namespace.
     // And that it mangles the same in C++03 through C++17
 #ifdef _MSC_VER
     // MSVC uses a different C++ ABI with a different name mangling scheme.
     // The type id name doesn't seem to contain the mangled form at all.
     assert(typeid(std::align_val_t).name() == std::string("enum std::align_val_t"));
 #else
     assert(typeid(std::align_val_t).name() == std::string("St11align_val_t"));
 #endif
   }
 #endif
 
   return 0;
 }