[WIP] Use portable code for aligned memory allocation#884
[WIP] Use portable code for aligned memory allocation#884concatime wants to merge 4 commits intozlib-ng:developfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #884 +/- ##
===========================================
+ Coverage 75.43% 75.45% +0.01%
===========================================
Files 73 73
Lines 8220 8216 -4
Branches 1359 1359
===========================================
- Hits 6201 6199 -2
+ Misses 1494 1492 -2
Partials 525 525
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
FreeBSD 11 works with |
|
FreeBSD 11 works with |
|
This is an interesting approach for sure. It does make me wonder whether one of these new methods could replace or be preferred before the apple one. Are there any macos users in the house? 😄 |
|
I would prefer to see |
This should not be required. We know for a fact that |
|
I was saying it would be good to have a unified way of detecting support for those functions, and I think it should be done in the build scripts versus doing the platform |
|
I have experience with minizip-ng on this same thing, I kept getting commit requests for people needing to change the platform |
|
But my PR focuses on standard, not platforms. The platforms should either support C11 or POSIX. We don’t care about FreeBSD or OpenBSD or any of that. What we can do on the build system is ensure one of C11 or a forced value to |
|
One issue here is that we target C99, and not C11. |
|
And I think POSIX 2001 is "next generation" enough, even 2008. |
|
It's also common knowledge that gcc is not POSIX-compliant by default... so requiring either C11 or POSIX is just recipe for disaster... POSIX is pretty much deprecated standard as far as gcc goes. |
As far as I understand, POSIX is relevant to libc, not the compiler. So Glibc/musl instead of GCC. But C11 is revelant to the compiler of course. |
|
Even though Linux is BSD-based, we decided to avoid any legacy BSD functions... As such we should also try to avoid POSIX functions if there is GNU equivalent... |
Since when?
Make sens to avoid legacy functions, BSD or not.
I disagree. We should avoid GNU equivalent and instead focus on POSIX. By doing this, we support *BSD, musl-based distros, Android (bionic libc) and even partially MacOS. |
|
@concatime We have limited Android support... we can't support all older SDK versions due to missing functions... What comes to other BSD variants (including MacOS, FreeBSD, OpenBSD, NetBSD, Haiku), they are being handled currently... |
|
I think we should support the Posix alternative. If Posix support needs a compiler parameter to be set, then this would need to be detected and added by configure and cmake. So I see what @nmoinvaz means about doing the detection there. |
|
@Dead2 I still haven't decided what alternatives we should support... It's too early to exclude anything... Even if we have C99 as recommended standard, I think we should not exclude newer standards if doing so doesn't complicate the code too much... |
c76cf8b to
b034439
Compare
|
I recommend picking the ISO C11 |
5dfe2f0 to
aa88244
Compare
|
So, we should edit the build systems to reflect this logic:
|
f23363c to
7327351
Compare
|
I have made a PR that shows kind of what I had in mind with the build system check. If you want to take from it feel free. |
8d5e0b8 to
813fedf
Compare
|
Now, FreeBSD builds w/o any added CMake CLI flags: https://builds.sr.ht/~iemaghni/job/462711 |
224b9fa to
49d1c34
Compare
| include(CheckFunctionExists) | ||
| include(CheckIncludeFile) | ||
| include(CheckSymbolExists) | ||
| include(CheckTypeSize) |
There was a problem hiding this comment.
Why reorder these? This makes it more difficult to review, since we cannot easily spot any actual changes.
CMakePushCheckState certainly looks interesting though.
Just to be clear, we prefer to have each commit do one thing (We should put this in the wiki, I guess).
So code-moving/reordering/cleanup in a separate commit/PR to make reviews/audits easier.
Changes like CMakePushCheckState is the kind of thing I'd like to se a separate PR for, and that it changes the old code to use it as well, to keep an common code style.
The more involved the changes of this PR, the less likely I can merge it into the mostly-fixes 2.0 branch. It can still make the 2.1 branch of course, but that does not have an estimated release-date yet.
My preference would be this: (I can still be persuaded otherwise though)
PR1. A minimal fix that enables building on FreeBSD, but does not touch other platforms.
PR2. Rewrite cmake to use CMakePushCheckState.
PR3. Rewrite aligned malloc detection/selection (Effectively this PR).
There was a problem hiding this comment.
I just ordered them in alphabetical order. This way, each include() has only one possible place. I can revert this and simply add include(CMakePushCheckState) at the end.
EDIT: Done.
|
Not sure whether it is a bug in the code or a bug with coverage measurements, but it doesn't look like posix_memalign() or memalign() gets used. It might be due to compiler builtins being used instead perhaps, and gcov being unable to detect that. |
49d1c34 to
d0eb14b
Compare
|
I never used codecov. So folks, I need your help to fix the check “codecov/patch”. |
| # include <stdlib.h> | ||
| #if !defined(MAL_IMPL) && !defined(_WIN32) | ||
| # if __STDC_VERSION__+0 >= 201112L | ||
| # define MAL_IMPL 2 /* ISO C11 `aligned_alloc` */ |
There was a problem hiding this comment.
Having the MAL_IMPL with number codes I think hurts readability. Perhaps we can use something more descriptive and also get away from using numbers to represent the different methods. I would be in favor of not having an intermediary MAL_IMPL.
There was a problem hiding this comment.
But I need a way to force a certain implementation from the build system. For Apple, I need to explicitly use the POSIX (MAL_IMPL=1) implementation. For Android, depending on the API level, I need to explicitly use either C11 (MAL_IMPL=2) or POSIX (MAL_IMPL=1) or the fallback memalign() (MAL_IMPL=0). If you have a cleaner way, I’m hearing.
There was a problem hiding this comment.
How about just using separate names for them? HAVE_POSIX_MEMALIGN or HAVE_ALIGNED_ALLOC, etc..?
There was a problem hiding this comment.
But then you would have something like this:
#if !defined(USE_ALIGNED_ALLOC) && !defined(USE_POSIX_MEMALIGN) && !defined(USE_MEMALIGN) && !defined(WIN32)
...
which is way too long. Also, I don’t have to set HAVE_ALIGNED_ALLOC, HAVE_POSIX_MEMALIGN and HAVE_MEMALIGN by using my method. I want the code to be platform (unix) agnostic (except for Windows) and only the build systems should problematic platforms like macOS and to a lesser extent Android (only if we decided to support older API levels). For example, the build system should not have to handle FreeBSD because it supports portable functions, nor Linux nor OpenBSD, nor Haiku, etc.
There was a problem hiding this comment.
It might be longer but it is also more readable. I'm not sure that this auto-detection based on _POSIX_C_SOURCE or __STDC_VERSION__ is necessary. The build system should just determine what is available and set HAVE_XYZ_ALLOC. zng_alloc would use the first available.
There was a problem hiding this comment.
But we don’t need HAVE_... for standard functions (ISO C or POSIX)! If a system does not implement standard functions, then they should be handled in the build system, not the code. I don’t want my code to be dependent on the presence of a build system. That’s why I have auto-detection based on __STDC_VERSION__ and _POSIX_C_SOURCE.
There was a problem hiding this comment.
You can't detect by value of _POSIX_C_SOURCE as it is defined as minimum supported version, not maximum... The build system must detect what is the maximum supported value for the compiler and then add it to compiler command line.
There was a problem hiding this comment.
@mtl1979 I already add to compiler command: https://github.com/zlib-ng/zlib-ng/pull/884/files#diff-f673bdec1661e9b36c026093309afc4ce1086fbb900f9697fb6ecaaa95c27386R30.
There was a problem hiding this comment.
@concatime I see conflicting definitions of _POSIX_C_SOURCE in configure now... In other projects we only define -D_GNU_SOURCE -D_DEFAULT_SOURCE which selects the latest supported _POSIX_C_SOURCE value on all modern compilers... The define name was recently changed, so both should be and can be safely defined.
| @@ -0,0 +1,32 @@ | |||
| # Neither Windows nor macOS support ISO C11 `aligned_alloc`. | |||
There was a problem hiding this comment.
This is almost the same thing as before but in comment form. Where when a new platform/sdk comes along are there going to be new commits for everybody wanting to add in their own URL for the docs. I guess that it is separated in its own .cmake helps.
There was a problem hiding this comment.
If these new "platform/sdk" support portable functions (e.g. Fuchsia), then no action is required in the build system.
| # libandroid_support.a, which is linked by default when targeting API 16). | ||
| # https://github.com/aosp-mirror/platform_bionic/commit/85aad909560508410101c18c6ecc6633df39c596 | ||
| # https://github.com/aosp-mirror/platform_bionic/commit/e219cefc173bf93b8ff710431784e5de30ffab8f | ||
| if(WIN32) |
There was a problem hiding this comment.
Hmm. I might be wrong actually.
d0eb14b to
9c1d420
Compare
|
Relevant informations found:
https://www.gnu.org/software/libc/manual/html_node/Aligned-Memory-Blocks.html That’s why we should use standard functions instead of GNU ones. (emphasis mine) |
|
This needs a rebase, and probably some redesign due to recent changes. |
|
I think this is already fixed in develop. |
This is a quick reborn of #766.
@Dead2, can you review this approach? I focused on standards instead of operating systems.
It should probably fix #873, will see.
We now have two standard “backends” and a fallback one:
aligned_alloc;-D _POSIX_C_SOURCE=200112L) forposix_memalign;memalign;