[libc++] Build the dylib with C++17 to allow aligned new/delete

[ldionne]

Summary

This allows simplifying the implementation of barriers.

[EricWF]

I'm find requiring C++17 to build the dylib, but I'm not sure everyone is?

What is the impact on our compiler support? Surely not all of the currently supported compilers provide -std=c++17?

I would say land this and see if anybody complains. Maybe add a #if _LIBCPP_STD_VER < 17 in one of the source files to enforce this?
I'm not sure all the buildbots will tolerate this change, but I'm happy to upgrade or fix those up.

[ldionne]

In D75243#1896087, @EricWF wrote:

I'm find requiring C++17 to build the dylib, but I'm not sure everyone is?

What is the impact on our compiler support? Surely not all of the currently supported compilers provide -std=c++17?

GCC 5 supports -std=c++17, and Clang 4.0 does support -std=c++1z. Furthermore, it should be safe to assume a reasonably recent Clang when building the library for serious purposes. For example, it would be surprising for a vendor to build libc++ with an old Clang and ship it.

I would say land this and see if anybody complains. Maybe add a #if _LIBCPP_STD_VER < 17 in one of the source files to enforce this?

Will do.

I'm not sure all the buildbots will tolerate this change, but I'm happy to upgrade or fix those up.

Alright, let's see if it breaks anything, but it shouldn't break the buildbots AFAICT.

[ldionne]

In D75243#1940123, @ldionne wrote:

In D75243#1896087, @EricWF wrote:

I would say land this and see if anybody complains. Maybe add a #if _LIBCPP_STD_VER < 17 in one of the source files to enforce this?

Will do.

Actually, this isn't required since we _require_ the 17 standard in CMake.

[dmajor]

In our builds with the 10.12 SDK and MACOSX_DEPLOYMENT_TARGET of 10.11, this is producing

[task 2020-07-22T21:52:27.615Z] /builds/worker/fetches/llvm-project/llvm/projects/libcxx/src/barrier.cpp:37:43: error: aligned allocation function of type 'void *(std::size_t, std::align_val_t)' is only available on macOS 10.14 or newer
[task 2020-07-22T21:52:27.615Z]         __state = unique_ptr<__state_t[]>(new __state_t[__count]);
[task 2020-07-22T21:52:27.615Z]                                           ^
[task 2020-07-22T21:52:27.615Z] /builds/worker/fetches/llvm-project/llvm/projects/libcxx/src/barrier.cpp:37:43: note: if you supply your own aligned allocation functions, use -faligned-allocation to silence this diagnostic

Is that expected? How can I determine whether our builds supplies its own aligned allocation functions?

[ldionne]

In D75243#2168014, @dmajor wrote:

In our builds with the 10.12 SDK and MACOSX_DEPLOYMENT_TARGET of 10.11, this is producing

[task 2020-07-22T21:52:27.615Z] /builds/worker/fetches/llvm-project/llvm/projects/libcxx/src/barrier.cpp:37:43: error: aligned allocation function of type 'void *(std::size_t, std::align_val_t)' is only available on macOS 10.14 or newer
[task 2020-07-22T21:52:27.615Z]         __state = unique_ptr<__state_t[]>(new __state_t[__count]);
[task 2020-07-22T21:52:27.615Z]                                           ^
[task 2020-07-22T21:52:27.615Z] /builds/worker/fetches/llvm-project/llvm/projects/libcxx/src/barrier.cpp:37:43: note: if you supply your own aligned allocation functions, use -faligned-allocation to silence this diagnostic

Is that expected? How can I determine whether our builds supplies its own aligned allocation functions?

Hmmm. There's a few interesting things here. First, I'd like to understand why you're building the dylib with a deployment target of 10.11 -- we usually make the assumption that people building libc++ are vendors, and we take for granted that they're building for new-ish platforms. For example, at Apple, we only build the dylib for the latest OS because that's what the dylib is going into, so using an older deployment target just doesn't make sense. If you have another use case, it might be useful to know about it for the future.

Second, I think it should be fine to build the dylib with -faligned-allocation because the dylib itself provides the aligned allocation functions. This error is meant to be useful in user code that would be running on a system-built libc++.dylib that doesn't contain the aligned allocation functions.

[dmajor]

In D75243#2169173, @ldionne wrote:

Hmmm. There's a few interesting things here. First, I'd like to understand why you're building the dylib with a deployment target of 10.11 -- we usually make the assumption that people building libc++ are vendors, and we take for granted that they're building for new-ish platforms. For example, at Apple, we only build the dylib for the latest OS because that's what the dylib is going into, so using an older deployment target just doesn't make sense. If you have another use case, it might be useful to know about it for the future.

I'm afraid I don't know the historical context around our Mac build settings, as I work primarily with our Windows and Linux tooling. I can try to ask around.

[froydnj]

In D75243#2169173, @ldionne wrote:

In D75243#2168014, @dmajor wrote:

In our builds with the 10.12 SDK and MACOSX_DEPLOYMENT_TARGET of 10.11, this is producing

Hmmm. There's a few interesting things here. First, I'd like to understand why you're building the dylib with a deployment target of 10.11 -- we usually make the assumption that people building libc++ are vendors, and we take for granted that they're building for new-ish platforms. For example, at Apple, we only build the dylib for the latest OS because that's what the dylib is going into, so using an older deployment target just doesn't make sense. If you have another use case, it might be useful to know about it for the future.

The wider context is that we're building clang for use on our continuous integration infrastructure and for distribution to developers so that they can use the exact same version of clang that our infrastructure is using. It is entirely possible that we should be building everything using different settings (or building a different set of things).

I would expect the same sort of issues to come up when building the official releases (releases.llvm.org releases, that is, not Apple releases); are they built for the latest macOS release always, or do they take a different set of steps to ensure broad deployability?

[ldionne]

In D75243#2169290, @froydnj wrote:

The wider context is that we're building clang for use on our continuous integration infrastructure and for distribution to developers so that they can use the exact same version of clang that our infrastructure is using. It is entirely possible that we should be building everything using different settings (or building a different set of things).

I would expect the same sort of issues to come up when building the official releases (releases.llvm.org releases, that is, not Apple releases); are they built for the latest macOS release always, or do they take a different set of steps to ensure broad deployability?

Indeed, I would expect the same kind of issue too when building the LLVM releases. Actually, I've never understood why we included the runtimes in the LLVM releases (for macOS), because that seems contrary to the way we are handling runtimes in macOS, where they are part of the OS and not the toolchain.

While I said we were slightly more aggressive on requiring that the dylib is built for new OSes, we also try to accommodate to some extent, and in this case the failure you're encountering is not expected. The documentation says that we support building for macOS 10.12 and above. So I think the right fix is to add -faligned-allocation when building.

libcxx/CMakeLists.txt

 # FIXME(EricWF): See the FIXME on LIBCXX_ENABLE_PEDANTIC.
 # Remove the -pedantic flag and -Wno-pedantic and -pedantic-errors
 # so they don't get transformed into -Wno and -errors respectively.
 remove_flags(-Wno-pedantic -pedantic-errors -pedantic)
 
 # Required flags ==============================================================
 function(cxx_add_basic_build_flags target)
 
-  # Require C++14 for all targets. C++14 is needed to ensure constant
-  # initialization for certain globals (ex global memory resources).
+  # Require C++17 for all targets. C++17 is needed to use aligned allocation
+  # in the dylib.
   set_target_properties(${target} PROPERTIES
-    CXX_STANDARD 14
+    CXX_STANDARD 17
     CXX_STANDARD_REQUIRED YES
     CXX_EXTENSIONS NO)
 
   # On all systems the system c++ standard library headers need to be excluded.
   # MSVC only has -X, which disables all default includes; including the crt.
   # Thus, we do nothing and hope we don't accidentally include any of the C++
   # headers
   target_add_compile_flags_if_supported(${target} PUBLIC -nostdinc++)

libcxx/src/barrier.cpp

 public:
     struct alignas(64) /* naturally-align the heap state */ __state_t
     {
         struct {
             __atomic_base<__barrier_phase_t> __phase = ATOMIC_VAR_INIT(0);
         } __tickets[64];
     };
 
-    ptrdiff_t&         __expected;
-    unique_ptr<char[]> __state_allocation;
-    __state_t*         __state;
+    ptrdiff_t&              __expected;
+    unique_ptr<__state_t[]> __state;
 
     _LIBCPP_HIDDEN
     __barrier_algorithm_base(ptrdiff_t& __expected)
         : __expected(__expected)
     {
         size_t const __count = (__expected + 1) >> 1;
-        size_t const __size = sizeof(__state_t) * __count;
-        size_t __allocation_size = __size + alignof(__state_t);
-        __state_allocation = unique_ptr<char[]>(new char[__allocation_size]);
-        void* __allocation = __state_allocation.get();
-        void* const __state_ = align(alignof(__state_t), __size, __allocation, __allocation_size);
-        __state = new (__state_) __barrier_algorithm_base::__state_t[__count];
+        __state = unique_ptr<__state_t[]>(new __state_t[__count]);
     }
     _LIBCPP_HIDDEN
     bool __arrive(__barrier_phase_t __old_phase)
     {
         __barrier_phase_t const __half_step = __old_phase + 1,
                                 __full_step = __old_phase + 2;
         size_t __current_expected = __expected,
             __current = hash<thread::id>()(this_thread::get_id()) % ((__expected + 1) >> 1);