This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by arichardson on Nov 17 2022, 3:29 AM.

Details

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).

Diff Detail

Event Timeline

arichardson created this revision.Nov 17 2022, 3:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2022, 3:29 AM
arichardson requested review of this revision.Nov 17 2022, 3:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2022, 3:29 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
ldionne accepted this revision.Nov 17 2022, 7:13 AM
ldionne added a subscriber: ldionne.

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

This revision is now accepted and ready to land.Nov 17 2022, 7:13 AM

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

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.

Try to disable aligned_alloc for macOS 10.9

arichardson edited the summary of this revision. (Show Details)Nov 21 2022, 2:58 AM

rebase to prod CI

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.

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.

  • extend test coverage
  • attempt to make it work on macOS.
arichardson edited the summary of this revision. (Show Details)Dec 1 2022, 1:16 PM
arichardson edited the summary of this revision. (Show Details)
arichardson added inline comments.Dec 1 2022, 1:18 PM
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 edited the summary of this revision. (Show Details)Dec 1 2022, 1:29 PM

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

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 accepted this revision.Dec 2 2022, 2:18 PM

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.

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

Rebase and update version guard to macos 10.15

arichardson edited the summary of this revision. (Show Details)Dec 21 2022, 2:07 AM

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 :)

ldionne accepted this revision.Dec 21 2022, 6:17 AM

rebase for CI

thakis added a subscriber: thakis.Dec 22 2022, 11:12 AM

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?

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).

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.)

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

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.)

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

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.

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.)

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.

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