HomePhabricator

[libc++] Factor out common logic for calling aligned allocation

Authored by ldionne on Nov 12 2020, 12:14 PM.

Description

[libc++] Factor out common logic for calling aligned allocation

There were a couple of places where we needed to call the underlying
platform's aligned allocation/deallocation function. Instead of having
the same logic all over the place, extract the logic into a pair of
helper functions libcpp_aligned_alloc and libcpp_aligned_free.

The code in libcxxabi/src/fallback_malloc.cpp looks like it could be
simplified after this change -- I purposefully did not simplify it
further to keep this change as straightforward as possible, since it
is touching very important parts of the library.

Also, the changes in libcxx/src/new.cpp and libcxxabi/src/stdlib_new_delete.cpp
are basically the same -- I just kept both source files in sync.

The underlying reason for this refactoring is to make it easier to support
platforms that provide aligned allocation through C11's aligned_alloc
function instead of posix_memalign. After this change, we'll only have
to add support for that in a single place.

Differential Revision: https://reviews.llvm.org/D91379

Event Timeline

In the description, you refer to fallback_malloc.cpp "The code in libcxxabi/src/fallback_malloc.cpp looks like it could be simplified after this change -- I purposefully did not simplify it further to keep this change as straightforward as possible, since it is touching very important parts of the library."

I am seeing downstream build errors for fallback_malloc.cpp with this commit, e.g.:

llvm-project/libcxxabi/src/fallback_malloc.cpp:237:10: error: no type named 'libcpp_aligned_free' in namespace 'std'
std::
libcpp_aligned_free(ptr);
~~~~~^
llvm-project/libcxxabi/src/fallback_malloc.cpp:237:32: warning: declaration shadows a local variable [-Wshadow]
std::__libcpp_aligned_free(ptr);

^

llvm-project/libcxxabi/src/fallback_malloc.cpp:233:41: note: previous declaration is here
void __aligned_free_with_fallback(void* ptr) {

^

I am seeing downstream build errors for fallback_malloc.cpp with this commit, e.g.:

llvm-project/libcxxabi/src/fallback_malloc.cpp:237:10: error: no type named 'libcpp_aligned_free' in namespace 'std'
std::
libcpp_aligned_free(ptr);
~~~~~^

How are you building the library? What's your configuration, system, compiler, etc? As you can see, our CI is green, so whatever you're experiencing is on a setup that we don't actively test.

I am seeing downstream build errors for fallback_malloc.cpp with this commit, e.g.:

llvm-project/libcxxabi/src/fallback_malloc.cpp:237:10: error: no type named 'libcpp_aligned_free' in namespace 'std'
std::
libcpp_aligned_free(ptr);
~~~~~^

How are you building the library? What's your configuration, system, compiler, etc? As you can see, our CI is green, so whatever you're experiencing is on a setup that we don't actively test.

Apologize for the lack of clarity -- I've had a chance to look into this more deeply.

Our build defines macro _LIBCPP_HAS_NO_LIBRARY_ALIGNED_ALLOCATION which results in libcpp_aligned_{alloc,free} to be not exposed in in the header file 'new'. However, in fallback_malloc.cpp, it appears this macro is only used to wrap the call to libcpp_aligned_alloc() within aligned_malloc_with_fallback() (falling back on ::malloc()), but the call to libcpp_aligned_free() within __aligned_free_with_fallback() is not wrapped.

This change appears to fix it:

--- a/libcxxabi/src/fallback_malloc.cpp
+++ b/libcxxabi/src/fallback_malloc.cpp
@@ -234,7 +234,11 @@ void __aligned_free_with_fallback(void* ptr) {
   if (is_fallback_ptr(ptr))
     fallback_free(ptr);
   else {
+#if defined(_LIBCPP_HAS_NO_LIBRARY_ALIGNED_ALLOCATION)
+    ::free(ptr);
+#else
     std::__libcpp_aligned_free(ptr);
+#endif
   }
 }

I can fix this locally to get our builds running, but is it possible that you could commit a fix upstream? I appreciate it, thanks!

I can fix this locally to get our builds running, but is it possible that you could commit a fix upstream? I appreciate it, thanks!

Done in d67e58f23a823, thanks for reporting. However, I'm still curious to understand how you're building libc++. Are you passing -D_LIBCPP_HAS_NO_LIBRARY_ALIGNED_ALLOCATION manually?

Done in d67e58f23a823, thanks for reporting. However, I'm still curious to understand how you're building libc++. Are you passing -D_LIBCPP_HAS_NO_LIBRARY_ALIGNED_ALLOCATION manually?

Thank you for the fix! Right now, we define _LIBCPP_HAS_NO_ALIGNED_ALLOCATION in a vendor-specific section of libcxx/__config along with a few other macros. However, I think this is a bit ad hoc. I think we should move this to the build system itself (use -D) and clean up the configuration.