Skip to content
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

Prefer aligned_alloc over posix_memalign #805

Merged
merged 1 commit into from
Jan 27, 2024
Merged

Conversation

AtariDreams
Copy link
Contributor

@AtariDreams AtariDreams commented Dec 15, 2023

aligned_alloc is more cleaner and portable.

@AtariDreams AtariDreams changed the title Utilize aligned_malloc from C11 rather than posix_memalign Prefer aligned_alloc over posix_memalign Dec 18, 2023
@AtariDreams
Copy link
Contributor Author

@compnerd Can we please do a test?

@compnerd
Copy link
Collaborator

Should we not have a fallback path here?

@AtariDreams
Copy link
Contributor Author

Should we not have a fallback path here?

are we not compiling with c11?

@compnerd
Copy link
Collaborator

There is no guarantee that the libc is going to be complete (e.g. Windows). Yes, I know that there is a separate Windows path, but this is already a difficult library to port, lets not complicate it further.

@AtariDreams
Copy link
Contributor Author

Collaborator

Windows is literally the only libc with this issue. I just checked.

@AtariDreams
Copy link
Contributor Author

There is no guarantee that the libc is going to be complete (e.g. Windows). Yes, I know that there is a separate Windows path, but this is already a difficult library to port, lets not complicate it further.

That's literally the point of this PR. And let's not assume that the code already assumes you are compiling for C11, as there are C99 and C11 features in this codebase like 0 length arrays.

@AtariDreams
Copy link
Contributor Author

Also, the PR I did for swift literally only checks for windows, and then the STDC version, not a complete libc. I don't see why we would need to find the one libc that doesn't have this but somehow has posix_memalign

aligned_alloc is more cleaner and portable.
@compnerd
Copy link
Collaborator

@swift-ci please test

@compnerd
Copy link
Collaborator

@swift-ci please test Windows platform

@compnerd compnerd merged commit bb1cb6a into apple:main Jan 27, 2024
2 checks passed
@AtariDreams AtariDreams deleted the buffer branch January 27, 2024 16:47
@triplef
Copy link
Contributor

triplef commented Jan 29, 2024

FYI for anyone else running into this: this change makes libdispatch require a minimum Android SDK version of 28 (Android 9 Pie) as noted here, because earlier versions don’t have aligned_alloc.

Building with an earlier minSdkVersion results in this error:

Linking CXX shared library ../libdispatch.so
ld: error: undefined symbol: aligned_alloc
>>> referenced by io.c:2377

Is there any chance to add an Android target to CI to be able to spot things like this sooner?

@finagolfin
Copy link
Contributor

Is there any chance to add an Android target to CI to be able to spot things like this sooner?

I will look into it. In the meantime, I have submitted a fallback, #811.

@triplef
Copy link
Contributor

triplef commented Feb 1, 2024

Thank you @finagolfin! 🙏

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.

None yet

4 participants