[PATCH] D45015: [Preprocessor] Allow libc++ to detect when aligned allocation is unavailable.

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added a comment.

In https://reviews.llvm.org/D45015#1127046, @vsapsai wrote:

> In https://reviews.llvm.org/D45015#1121762, @EricWF wrote:
>
> > In https://reviews.llvm.org/D45015#1121581, @ahatanak wrote:
> >
> > > Could you elaborate on what kind of changes you are planning to make in libc++ after committing this patch?
> >
> >
> > Libc++ shouldn't actually need any changes if this current patch lands. Currently libc++ is in a "incorrect" state where
> >  it generates calls to `__builtin_operator_new(size_t, align_val_t)` when `__cpp_aligned_new` is defined but when aligned new/delete
> >  are actually unavailable.
> >
> > If we change `__cpp_aligned_new` to no longer be defined when aligned new is unavailable, then libc++ will start doing the right thing.
> >  See r328180 <https://github.com/llvm-mirror/libcxx/commit/a83128739983c83eaf1ba4c2bc0e3aa570082d15#diff-bd538fe75403fdbc4de9a1b97bc7c283> for the relevent commits which made these libc++ changes.
>
>
> Looks like in libc++ we need to remove `_LIBCPP_STD_VER` check for aligned allocations, something like
>
>    #if !defined(_LIBCPP_HAS_NO_ALIGNED_ALLOCATION) && \
>   -    (!(defined(_LIBCPP_BUILDING_NEW) || _LIBCPP_STD_VER > 14 || \
>   +    (!(defined(_LIBCPP_BUILDING_NEW) || \
>        (defined(__cpp_aligned_new) && __cpp_aligned_new >= 201606)))
>    # define _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
>    #endif
>
>
> Is that correct? I didn't check the rest of the code, probably TEST_HAS_NO_ALIGNED_ALLOCATION needs some clean up too.


Huh, that original formulation is correct-ish, and so is your proposed changes. There are two problems here:

1. We want to expose the aligned allocation declarations to users in C++17, even if the compiler will never call them because

the feature has been disabled.

2. We don't want to generate calls to them via `__builtin_operator_new` when we've only enabled the declarations because of C++17.

I'll make some changes to libc++ to address this. Thanks for pointing it out.


https://reviews.llvm.org/D45015