-
Notifications
You must be signed in to change notification settings - Fork 632
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
link error on Intel macOS when building Universal Binary (arm64) using cmake #372
Comments
Any updates on this? |
The problem that was originally reported is now fixed, but macOS universal binaries still cannot be produced. |
I'm also running into the issue of not being able to create universal binaries. I was looking into the code and the issue seems to be that all the flags for hardware-specific options are set as compiler flags, which of course means that a single compile-command cannot correctly make a binary for both architectures. I wonder if there is a specific reason to do it this way? If we instead make a header that determines the architecture (and then sets the necessary defines), we could instead include that. This would mean that when invoking clang for multiple architectures those defines can be different and then it should work fine. We could check for things like Is this a left-over from the autotools time? Would a PR to change this have a chance of being accepted? |
@omartijn Hello, did you work it around anyhow? |
No, I was waiting for a reply from a maintainer. I don't want to do the work unless it's going to be used. I'm using libpng from |
Ran into this today as well.
but the build fails in the neon code. The 1st error in a long list is:
I can build for either architecture but not a universal build for both. Also, I'm on a arm Mac trying to create a universal build. |
This is my crapy little hack to make a universal binary.
|
I'm seeing this when building a universal binary on a m1 mac as well. Is there any known workarounds? Currently just building 2 builds. If anyone has any insight on where to take a look at the problem with it I would be happy to help. |
Either disable the hardware options completely or don't try to build libpng yourself - rely on the Apple provided functionality. |
My initial bug report literally contains a workaround. |
This is a possible xv-backdoor response. @seanm responded within a few minutes of my response and responded off-topic to that response; the @seanm response is discursive and is inviting discursive responses; @seanm is the OP and should have closed this as @ctruta observed that, "The problem that was originally reported is now fixed". Context (off-immediate-topic): https://www.theregister.com/2024/04/01/xz_backdoor_open_source/ |
My apologies @seanm , that was brusque and inappropriate particularly as I did exactly the same thing myself. Cross compilation is not environment specific and not architecture specific; it needs to be supported by the cmake builds and, so far as I can see, cmake has completely adequate mechanisms for doing this. So far as I know at this point cross compilation is completely supported; not just for ARM64 but also the various other instruction sets. Given that assumption I strongly recommend that you or @ctruta close this issue, it is clearly a bug, and ignore the other issues that were raised. They need to be raised in separate issues. We should all know not to morph bug reports into separate issues; it's done often enough but it's wrong. Any single "issue" needs to be about one identifiable thing so that the thing in question can be tested and regressed. Cross compilation is one thing and it is not system-specific, multilib and extensions are separate issues that are also not system-specific and so on including system-specific things like support for specific IDEs. |
I hit this issue recently, but still had the same compiler errors as the OP when setting # build fails
cmake -S . -B build -DCMAKE_OSX_ARCHITECTURES:STRING="x86_64;arm64" -DPNG_HARDWARE_OPTIMIZATIONS=OFF
cmake --build build/
# build succeeds
cmake -S . -B build -DCMAKE_OSX_ARCHITECTURES:STRING="arm64;x86_64" -DPNG_HARDWARE_OPTIMIZATIONS=OFF
cmake --build build/ Notably, building with the optimizations enabled fails for both architecture orderings. With I don't know enough about the libpng code base to offer solutions to these issues, so I post this here so that it might help someone else in the future. For optimized libraries, it seems an approach like @zaun's solution makes the most sense as a workaround. Error log compiling with
|
The whole thing is a bug in If In other words Until it's fixed you can use
That should work because the -U, which cmake puts in It is apparently also possible to edit the |
So to summarize a possible CMake fix, if Just opened a draft PR to this effect. Could optionally restore the removed |
No. The optimizations are on by default, so "PNG_HARDWARE_OPTIMIZATIONS=ON" should do nothing and all the things that depend on it being "ON" should happen by default. I.e. all the hardware specific files for all architectures need to be compiled in regardless. The only useful setting for PNG_HARDWARE_OPTIMIZATION is OFF because that does change the behavior from the default and the only thing it should be doing is setting PNG_machine_OPT=0. If this change causes something to fail then the specific failing hardware stuff should be removed from the build until the maintainer of that hardware fixes it! Doing this consistently should fix all the obvious bugs in CMakeLists.txt by simply removing the buggy lines! For example the checks against the architecture which use "^" are apparently bogus. |
Thanks. I want to make sure I get this right. So If My question about |
It's more that the files are all compiled in unless Note that it appears the intel SSE optimizations have been disabled by default even if the compile time checks pass with It doesn't do any harm to add the extra sources when For Neon arm/foo.* contain two implementations, one assembler (which only works for 32-bit and possibly only some CPUs) and the intrinsics. There's another bug about that; the only case the assembler is required is for older compilers and compilers which don't support the intrinsics (which may be GNU specific.) In several CPU cases there is support for a run-time check or an API check. This stuff is mostly broken; e.g. the ARM API check relied on Linux specific stuff and even then it didn't work on Android! It should be sufficient in all cases except Loongarch to just build the code relying on the compiler #define checks. Loongarch won't work in a multilib because there are no compiler flags (maybe there are in bespoke Chinese compilers, but not in the US). |
It seems to me that most of the stuff in I'm guessing Mysteriously the 32-bit neon support is hard-wired off. It looks half-implemented. With the exception of the Loongarch support (which I left as-is) I simply deleted all the TARGET_ARCH MATCHES code from I think you need the pngpriv.h patch to test this; with that patch you should see a build with real code in both I'll post the patch here; it's on a different machine :-) |
Here's the patch: |
Actually, this begs a question: do you want the CMake configuration to more or less match the
Can I reference the
Thanks! I'll review all of this next week. |
What I want is for both I don't know if Cosmin wants this; certainly what he implemented does not do this. I regard the
I have a set of changes to both
I say "yes" with regard to the behavior with no options I'm pretty sure it's correct in a non-multilib build (i.e. a single CPU target). I tested 32-bit extensively a short time ago while looking at the solution to remove filter_neon.S With regard to --enable-hardware-optimizations I say yes as well and The bottom line is that given a similar set of command line options both Unfortunately I can generate a PR for the |
In fact this is the time to start libpng 1.8 on the basis that this is a problem which needs fixing and the fix is likely to be too scary for the Cosmin, given what he did to filter_neon.S Numbers are cheap and a minimal step up to 1.8 with just the removal of deprecated functionality and fixes to the build systems allow the latter to include significant simplifications which have been pending for 10 years now. |
There's a lot of project history here that I don't completely understand, but it seems like this change should be focused just on removing the optimization gates in CMake.
This is essentially what I'd been thinking and ended up doing. Like you, I left loongarch as is, though that could be simplified even further so that inclusion is entirely dependent on |
There's this issue and then there is the PR. I'm in favor of the PR being separated into multiple independent PRs (like the pngpriv.h issue is a separate issue) as this might mean @ctruta would take it in 1.6. Unfortunately that hasn't happened in the past; old, confusing and counter-productive behavior has been retained. It would be good to know if configure can actually be used to produce a Universal Binary since it is, apparently, doing the correct stuff when called without any of the command line options. |
Because of a missing "amd64" string (in lowercase) in a regex match, the CMake build was unable to pick up the PNG_HARDWARE_OPTIMIZATIONS flag on FreeBSD/amd64 (and possibly other amd64 systems as well). Rename the target arch variable from TARGET_ARCH to a more idiomatic PNG_TARGET_ARCHITECTURE, and set it to an always-lowercase string. The follow-on checks are now simpler and easier to get right.
(Emphasis mine) About that: see my most recent note #576 (comment) about my idea to surprise the world with a brand new libpng-1.8.0. |
Fixed, not: #583 massively simplifies things but I keep finding TODOs I wrote that also need doing :-) |
Trying to build current git master (a37d483) as a Universal Binary (or even just arm64 only) on a Intel Mac fails to link:
result:
This is because of https://github.com/glennrp/libpng/blob/a37d4836519517bdce6cb9d956092321eca3e73b/CMakeLists.txt#L71 which queries the compiling machine's CPU type. This doesn't work for Universal Binaries, which are basically cross compilation, because the CPU doing the building doesn't match the CPU being built for.
A working hack is to add in
OR APPLE
so that those arm-only .c files are added to the list of files to be compiled. Compiling them on Intel seems harmless because their contents are guarded with#if PNG_ARM_NEON_OPT > 0
The text was updated successfully, but these errors were encountered: