[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 ↗

(On Diff #479400)

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

361

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 ↗

(On Diff #479411)

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/new

 // 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)
   return ::_aligned_malloc(__size, __alignment);
+#elif _LIBCPP_STD_VER > 14
+  return ::aligned_alloc(__alignment, __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
 }
 
 inline _LIBCPP_INLINE_VISIBILITY
 void __libcpp_aligned_free(void* __ptr) {
 #if defined(_LIBCPP_MSVCRT_LIKE)
   ::_aligned_free(__ptr);
 #else
   ::free(__ptr);
 #endif
 }
 #endif // !_LIBCPP_HAS_NO_ALIGNED_ALLOCATION
 
 
 template <class _Tp>
 _LIBCPP_NODISCARD_AFTER_CXX17 inline _LIBCPP_HIDE_FROM_ABI
 _LIBCPP_CONSTEXPR _Tp* __launder(_Tp* __p) _NOEXCEPT
 {
     static_assert (!(is_function<_Tp>::value), "can't launder functions" );