Skip to content

CDRIVER-5877 avoid aligned_alloc on MacOS prior to 10.15 #1841

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

barracuda156
Copy link
Contributor

@NattyNarwhal Could you please review this? Pre-Catalina macOS need the same fix as you have added for AIX in 9fe8af1

@NattyNarwhal
Copy link
Contributor

This looks correct for macOS, but it might not be correct for iOS builds (as that will be __APPLE__ but I'm unsure if MAC_OS_X_VERSION_MIN_REQUIRED is set).

That said, as mentioned in GH-1480, the right longer term fix would be putting a check in CMake/autoconf (and propagating it to things that vendor mongo-c-driver with their own build system; i.e. the PHP extension).

@eramongodb
Copy link
Contributor

Created CDRIVER-5876 to track relocating aligned_alloc availability checks from preprocessor directives into the build system.

Copy link
Contributor

@eramongodb eramongodb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difficulty of finding official documentation for <AvailabilityMacros.h> and its provided features makes me hesitant to depend on it. This may nevertheless suffice as a temporary solution until CDRIVER-5876 is resolved.

@@ -32,7 +36,7 @@ BSON_STATIC_ASSERT2 (bson_mem_vtable_t, sizeof (bson_mem_vtable_t) == sizeof (vo
static void *
_aligned_alloc_impl (size_t alignment, size_t num_bytes)
#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112L && !defined(_WIN32) && !defined(__ANDROID__) && \
!defined(_AIX)
!defined(_AIX) && !(defined(__APPLE__) & MAC_OS_X_VERSION_MIN_REQUIRED < 101500)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
!defined(_AIX) && !(defined(__APPLE__) & MAC_OS_X_VERSION_MIN_REQUIRED < 101500)
!defined(_AIX) && !(defined(MAC_OS_X_VERSION_MIN_REQUIRED) && MAC_OS_X_VERSION_MIN_REQUIRED < 101500)

Use logical OR rather than bitwise OR. Additionally recommend adjusting the condition to test for the definition of MAC_OS_X_VERSION_MIN_REQUIRED instead of defined(__APPLE__) (already used to condition the inclusion of <AvailabilityMacros.h> above) to guard against the possibility of OSX_DEPLOYMENT_TARGET and/or -mmacosx-version-min interfering with the presence of the macro (not a configuration space we control nor test).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noting here for future reference that the value of 101500 (MacOS 10.15) is consistent with libc++'s own checks for aligned_alloc support:

// It is not yet possible to use aligned_alloc() on all Apple platforms since
// 10.15 was the first version to ship an implementation of aligned_alloc().
#  if defined(__APPLE__)
#    if (defined(__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__) &&                                                     \
         __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ < 101500) ||                                                    \
        /* ... */
#      define _LIBCPP_HAS_C11_ALIGNED_ALLOC 0
#    else
#      define _LIBCPP_HAS_C11_ALIGNED_ALLOC 1
#    endif
#  elif /* ... */

@eramongodb eramongodb changed the title bson-memory: fix for Apple CDRIVER-5877 avoid aligned_alloc on MacOS prior to 10.15 Feb 3, 2025
@eramongodb
Copy link
Contributor

Filed CDRIVER-5877 to track this PR.

@kevinAlbs
Copy link
Collaborator

Thank you for alerting us of this issue @barracuda156. CDRIVER-5876 applies a fix within CMake to detect if aligned_alloc is available. CDRIVER-5876 was released in 1.30.0.

@kevinAlbs kevinAlbs closed this Feb 6, 2025
@barracuda156
Copy link
Contributor Author

@kevinAlbs Thank you very much, great! (Sorry for a delayed reply, a lot of stuff in the pipeline.)

@kevinAlbs
Copy link
Collaborator

@kevinAlbs Thank you very much, great! (Sorry for a delayed reply, a lot of stuff in the pipeline.)

Hah no worries. And thanks to @eramongodb for the change. I only reviewed it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants