search

swiftlang / swift-corelibs-libdispatch

The libdispatch Project, (a.k.a. Grand Central Dispatch), for concurrency on multicore hardware
swift.org
Apache License 2.0
2.47k stars 460 forks source link

Prefer aligned_alloc over posix_memalign #805

Closed AreaZR closed 8 months ago

AreaZR commented 10 months ago

aligned_alloc is more cleaner and portable.

AreaZR commented 10 months ago

@compnerd Can we please do a test?

compnerd commented 10 months ago

Should we not have a fallback path here?

AreaZR commented 10 months ago

Should we not have a fallback path here?

are we not compiling with c11?

compnerd commented 10 months ago

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.

AreaZR commented 10 months ago

Collaborator

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

AreaZR commented 10 months ago

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.

AreaZR commented 10 months ago

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

compnerd commented 9 months ago

@swift-ci please test

compnerd commented 9 months ago

@swift-ci please test Windows platform

triplef commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

Thank you @finagolfin! 🙏