Don't directly include asm/hwcap.h; fix compilation on musl aarch64 #936
Conversation
sys/auxv.h includes the appropriate headers to provide the HWCAP constants, on both glibc and musl, which makes it unnecessary to include asm/hwcap.h directly. And on musl, asm/hwcap.h doesn't exist.
Codecov Report
@@ Coverage Diff @@
## develop #936 +/- ##
===========================================
- Coverage 77.89% 77.59% -0.31%
===========================================
Files 74 74
Lines 8307 8301 -6
Branches 1369 1374 +5
===========================================
- Hits 6471 6441 -30
- Misses 1307 1331 +24
Partials 529 529
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
We explicitly include some redundant headers to trap differences between compilers. We don't officially support every combination there is (compiler + platform + C run-time library). |
|
@mtl1979 In this case, this isn't a difference between compilers, it's a difference between musl and glibc; musl doesn't seem to support directly including the kernel headers here. It'd be helpful to have support for aarch64-linux-musl, for the purposes of building statically compiled binaries for aarch64. I've tested that with this one change included, it's possible to compile zlib-ng for an aarch64-unknown-linux-musl target. |
|
@joshtriplett I assumed musl and glibc are both C run-time libraries... GCC usually use glibc by default, Visual C++ use its own C libary, etc... In my personal opinion, we should support combinations that enough people use... This excludes cases that are complex to include without causing possible conflicts, including duplicate names, or names that are used for two different purposes. |
|
As long as this does not trigger any regressions, I honestly don't see any problem with including the change. Supporting musl would be nice, even if it is not something we target or spend time on, especially if it is this simple. |
|
Both glibc and musl seem to most commonly use gcc as their compiler (though both can be compiled with clang as well). I completely agree that not all combinations are reasonable to support. I'm hoping that since aarch64-linux-musl works with just this one line changed, this would be OK to add. |
|
@joshtriplett Changing header filename is usually easier to accept than removing one header... This is because we assume that breaking change in one specific header file is likely break build of other projects than zlib-ng too and backfire more likely. Personally, I have worked quite a while with Linaro gcc team and Microsoft, so I have had to fix quite a lot of header related issues. |
|
Can you clarify what problem you're concerned about with this change? Would you prefer that I wrap this in an |
|
@Dead2 It's not about triggering regression, it's about opening Pandora's box... Basically this is same as Microsoft moving some intrinsics to combined header and restricting their use to only AArch64, even though ARMv8 has supported them for both 32-bit and 64-bit... In some untested configurations, hard fail is better than unknown behavior. @joshtriplett If we remove the header check for all C run-time library possibilities, we basically assert that we support all possible C run-time libraries even if they are not API compatible with glibc. I'm not sure if we should do the existence check on configure step or in just this source file. I'm inclined to accept both solutions, but it's just my opinion. |
|
@mtl1979 Ah, I see. Supporting just "glibc and musl" rather than "every possible libc" seems reasonable. I do think that'd be better done using cmake detection and compilation tests, rather than by including headers like this that exist in some implementations and not in others. As far as I can tell, musl intentionally doesn't provide any musl-specific defines, so there's no way to detect musl in this context and only stop including the header in that case. Given that, would you consider it reasonable to drop this particular header include to fix musl? |
|
@joshtriplett I think the best solution is to move the conformity test to CMakeLists.txt and configure as build test... Basically we only need to make sure the function itself and all the used defines are declared correctly. This allows us to also trap cases when someone tries to use too old version of C run-time library, for example on Android. |
|
@mtl1979 When you say "all the used defines", do you mean the |
|
I think it is fine also as long as it doesn't cause regression. |
|
@joshtriplett Failing as early as possible is the best... If I remember correctly ARM and AArch64 have slightly different names for the defines, so one has to check for the correct ones depending on if the target system is 32-bit or 64-bit. |
|
I've added an additional commit that adds compile-time detection to make sure we have the necessary hwcap defines available to do runtime detection. That way, if there's a missing definition, the user will get a message from cmake when doing initial configuration, rather than a compile-time warning or silently missing detection at runtime. I hope this addresses the concern; now, the code will compile with more C libraries, but if the C library doesn't provide the necessary definitions, the user will get a useful message from cmake. |
|
@joshtriplett The second commit fixes it for cmake, but not when using configure... |
|
Pending CI confirmation, hopefully the third commit will do the same for configure. |
|
Looks like it passed CI. So, this should cover both configure and cmake, and provide better configuration-time detection of whether runtime detection will work. |
|
Looks pretty good to me, but I find the defines are a bit long and awkward |
|
@Dead2 I agree that the defines could be as short as possible while still being descriptive... Should definitely follow the same pattern as existing ones. It's also matter of debate if the commits should be squashed before merging. |
|
Well for includes we have always started them with |
|
I can certainly modify the names. I do think it makes sense to put the As for squashing, please feel free to squash-merge if you prefer, or if you'd like, I can rebase into 2 commits or 1 commit. Let me know what you'd prefer and I'd be happy to structure it accordingly. |
This allows us to provide useful warning messages from cmake or configure if the system headers don't provide the necessary flags to do runtime detection.
joshtriplett
force-pushed
the
musl-aarch64
branch
from
31a9ecb
to
6e6fdb0
Compare
|
I pushed a new version which combines the cmake and configure commits, and which renames the definitions. |
- Include porting guide in release packages #917 - Documentation improvements #913 #949 - Added Windows ARM binaries in release packages #916 - Fix crash on ARMv7 #927 - Fix building on FreeBSD #921 - Fix building with musl on aarch64 #936 - Fix ARM float-abi detection #918 - Fix cmake detection of risc-v architectures #942 - Minor buildsystem fixes #922 #924 #933 #938 #950 - Improve zlib-compat build #915 #944 - CI/Test improvements #926 #929 #925 #937 #939 #940
- Include porting guide in release packages #917 - Documentation improvements #913 #949 - Added Windows ARM binaries in release packages #916 - Fix crash on ARMv7 #927 - Fix building on FreeBSD #921 - Fix building with musl on aarch64 #936 - Fix ARM float-abi detection #918 - Fix cmake detection of risc-v architectures #942 - Minor buildsystem fixes #922 #924 #933 #938 #950 - Improve zlib-compat build #915 #944 - CI/Test improvements #926 #929 #927 #937 #939 #940
- Include porting guide in release packages #917 - Documentation improvements #913 #949 - Added Windows ARM binaries in release packages #916 - Fix crash on ARMv7 #927 - Fix building on FreeBSD #921 - Fix building with musl on aarch64 #936 - Fix ARM float-abi detection #918 - Fix cmake detection of risc-v architectures #942 - Minor buildsystem fixes #922 #924 #933 #938 #950 - Improve zlib-compat build #915 #944 - CI/Test improvements #926 #929 #927 #937 #939 #940
- Include porting guide in release packages #917 - Documentation improvements #913 #949 - Added Windows ARM binaries in release packages #916 - Fix crash on ARMv7 #927 - Fix building on FreeBSD #921 - Fix building with musl on aarch64 #936 #952 - Fix ARM float-abi detection #918 - Fix cmake detection of risc-v architectures #942 - Minor buildsystem fixes #922 #924 #933 #938 #950 - Improve zlib-compat build #915 #944 - CI/Test improvements #926 #929 #927 #937 #939 #940
- Include porting guide in release packages #917 - Documentation improvements #913 #949 - Added Windows ARM binaries in release packages #916 - Fix crash on ARMv7 #927 - Fix building on FreeBSD #921 - Fix building with musl on aarch64 #936 #952 - Fix ARM float-abi detection #918 - Fix cmake detection of risc-v architectures #942 - Minor buildsystem fixes #922 #924 #933 #938 #950 - Improve zlib-compat build #915 #944 - CI/Test improvements #926 #929 #927 #937 #939 #940
- Include porting guide in release packages zlib-ng#917 - Documentation improvements zlib-ng#913 zlib-ng#949 - Added Windows ARM binaries in release packages zlib-ng#916 - Fix crash on ARMv7 zlib-ng#927 - Fix building on FreeBSD zlib-ng#921 - Fix building with musl on aarch64 zlib-ng#936 zlib-ng#952 - Fix ARM float-abi detection zlib-ng#918 - Fix cmake detection of risc-v architectures zlib-ng#942 - Minor buildsystem fixes zlib-ng#922 zlib-ng#924 zlib-ng#933 zlib-ng#938 zlib-ng#950 - Improve zlib-compat build zlib-ng#915 zlib-ng#944 - CI/Test improvements zlib-ng#926 zlib-ng#929 zlib-ng#927 zlib-ng#937 zlib-ng#939 zlib-ng#940
sys/auxv.h includes the appropriate headers to provide the HWCAP
constants, on both glibc and musl, which makes it unnecessary to include
asm/hwcap.h directly. And on musl, asm/hwcap.h doesn't exist.