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

Halide doesn't build with brew-installed LLVM on OSX using Make #7055

Closed
steven-johnson opened this issue Sep 28, 2022 · 20 comments
Closed

Halide doesn't build with brew-installed LLVM on OSX using Make #7055

steven-johnson opened this issue Sep 28, 2022 · 20 comments

Comments

@steven-johnson
Copy link
Contributor

A user who was trying to follow the tutorial steps on a Mac laptop reported (and I confirmed) that attempting to use a homebrew-installed version of LLVM doesn't build properly, for reasons that aren't clear.

Steps to repeat:

  • OSX Monterey 12.6, XCode 13.3, Homebrew 3.6.3
  • brew install llvm && brew link llvm --force
  • depending on your setup, you may need to explicitly export LLVM_CONFIG=/path/to/brew/llvm-config (eg /usr/local/opt/llvm/bin/llvm-config on my system)
  • make clean && make tutorial_lesson_01_basics

Results on my system:

ld: in /usr/local/Cellar/llvm/15.0.1/lib/libLLVMAArch64AsmParser.a(AArch64AsmParser.cpp.o), could not parse object file /usr/local/Cellar/llvm/15.0.1/lib/libLLVMAArch64AsmParser.a(AArch64AsmParser.cpp.o): 'Not an int attribute (Producer: 'LLVM15.0.1' Reader: 'LLVM APPLE_1_1316.0.21.2_0')', using libLTO version 'LLVM version 13.1.6, (clang-1316.0.21.2)' for architecture x86_64

(Apparently, AArch64AsmParser.cpp.o contains LLVM bitcode, not x64 native code?)

Results on user's system:

ld: in /opt/homebrew/Cellar/llvm/15.0.1/lib/libLLVMAArch64AsmParser.a(AArch64AsmParser.cpp.o), could not parse object file /opt/homebrew/Cellar/llvm/15.0.1/lib/libLLVMAArch64AsmParser.a(AArch64AsmParser.cpp.o): 'Opaque pointers are only supported in -opaque-pointers mode (Producer: 'LLVM15.0.1' Reader: 'LLVM APPLE_1_1400.0.29.102_0')', using libLTO version 'LLVM version 14.0.0, (clang-1400.0.29.102)' for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
@steven-johnson
Copy link
Contributor Author

Note: the Homebrew setup above fails in a similar way with CMake (you must specify -DHalide_SHARED_LLVM=ON when configuring CMake in order to build)

@steven-johnson
Copy link
Contributor Author

Looks like https://github.com/orgs/Homebrew/discussions/3666 is the likely issue

@carlocab
Copy link

carlocab commented Sep 28, 2022

I haven't tried your build using make, but configuring cmake with

-DHalide_SHARED_LLVM=ON -DCMAKE_SHARED_LINKER_FLAGS=-Wl,-lto_library,"$(brew --prefix llvm)/lib/libLTO.dylib"

works. For make, I suspect doing

export LDFLAGS="-Wl,-lto_library,$(brew --prefix llvm)/lib/libLTO.dylib"

should work.

@alexreinking
Copy link
Member

It's good to know that exists as a workaround, but this is clearly an undue burden on downstreams. Homebrew should not break default-setting compatibility with Xcode/AppleClang.

@carlocab
Copy link

This is hardly a burden, because there are multiple trivial workarounds you can use that can even be encoded portably in a CMakeLists.txt or Makefile if desired. I've suggested one above.

What is an undue burden is insisting that everyone else's builds and binaries that link to LLVM libraries should be slower in order to simplify linking with LLVM's static libraries -- which I'll note is a comparatively uncommon use-case. Most users install Homebrew LLVM for the bundled toolchain or as a dependency of another Homebrew package. Relying on static libraries isn't even portable; most distributions either don't ship them at all or ship an incomplete set. Homebrew likely would not either if it wasn't more work to just remove them.

So, no, in the case of static libraries, I view seamless compatibility with Apple Clang as nice-to-have but not essential. If this affected more than the static libraries, then that would be something else, but all evidence so far suggests otherwise.

@alexreinking
Copy link
Member

This is hardly a burden, because there are multiple trivial workarounds you can use that can even be encoded portably in a CMakeLists.txt or Makefile if desired. I've suggested one above.

Show the code, please. I don't think this is anywhere close to portable or trivial.

@alexreinking
Copy link
Member

alexreinking commented Sep 29, 2022

So, no, in the case of static libraries, I view seamless compatibility with Apple Clang as nice-to-have but not essential. If this affected more than the static libraries, then that would be something else, but all evidence so far suggests otherwise.

Your own suggestion showing how to link to the shared libraries seems to require another flag (-Wl,-lto_library,...). Have I misunderstood you?

@carlocab
Copy link

carlocab commented Sep 29, 2022

You have. Linking with the shared libraries works as before. The extra configuration is required to link with the static libraries. See the error message quoted above:

Results on my system:

ld: in /usr/local/Cellar/llvm/15.0.1/lib/libLLVMAArch64AsmParser.a(AArch64AsmParser.cpp.o), could not parse object file /usr/local/Cellar/llvm/15.0.1/lib/libLLVMAArch64AsmParser.a(AArch64AsmParser.cpp.o): 'Not an int attribute (Producer: 'LLVM15.0.1' Reader: 'LLVM APPLE_1_1316.0.21.2_0')', using libLTO version 'LLVM version 13.1.6, (clang-1316.0.21.2)' for architecture x86_64

Results on user's system:

ld: in /opt/homebrew/Cellar/llvm/15.0.1/lib/libLLVMAArch64AsmParser.a(AArch64AsmParser.cpp.o), could not parse object file /opt/homebrew/Cellar/llvm/15.0.1/lib/libLLVMAArch64AsmParser.a(AArch64AsmParser.cpp.o): 'Opaque pointers are only supported in -opaque-pointers mode (Producer: 'LLVM15.0.1' Reader: 'LLVM APPLE_1_1400.0.29.102_0')', using libLTO version 'LLVM version 14.0.0, (clang-1400.0.29.102)' for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

I don't even know why you're linking with a static LLVM library when Halide_SHARED_LLVM is set -- that smells like a recipe for loading from multiple versions of the same library when libLLVM.dylib is updated in-place. But I haven't taken a close look at your build system, so perhaps this is not an issue.

Note that I said only that incorporating the necessary build flags into your Makefiles or CMakeLists.txts could be done portably, and not necessarily that this would be trivial. How trivial this is is also a function of how complicated your build system is, which, as I've stated, I haven't taken a close look at.

What is trivial are the end-user workarounds. They can do

export LDFLAGS=-Wl,-lto_library,/path/to/llvm/lib/libLTO.dylib

or

export CFLAGS=-fuse-ld=/path/to/llvm/bin/ld64.lld
export CXXFLAGS=-fuse-ld=/path/to/llvm/bin/ld64.lld

This can be simplified to

export CFLAGS=-fuse-ld=lld
export CXXFLAGS=-fuse-ld=lld

if ld64.lld is in PATH. Alternatively:

export CC="$(brew --prefix llvm)/bin/clang"
export CXX="$(brew --prefix llvm)/bin/clang++"

will also work fine.

Setting -lto_library can always be done, regardless of the source or configuration of the LLVM build, as long as the LLVM distribution in use ships a clang binary. clang will not function if libLTO is not included with it.

This isn't too much of a leap above what you already require Homebrew LLVM users to do:

  • brew install llvm && brew link llvm --force
  • depending on your setup, you may need to explicitly export LLVM_CONFIG=/path/to/brew/llvm-config (eg /usr/local/opt/llvm/bin/llvm-config on my system)

However, if it really is so onerous, Homebrew users can use llvm@14, as linking with the static libraries of llvm@14 will work as before. Alternatively, they can brew install halide for the stable version that uses llvm@14, or brew install --HEAD halide for the development version that uses llvm.

This is not available for users who need to build Halide from a specific version/commit, but I'm guessing that these users will be able to handle needing to set an extra flag or two to get things to work. It's likely that anyone looking to do this needs a specific version of LLVM anyway, in which case what is provided by Homebrew will likely not suit.

As for code samples, here is one: https://github.com/banach-space/llvm-tutor/blob/65e163b676199deaf68e76ac6f2ef26458d75429/tools/CMakeLists.txt

Here's another:

cmake_minimum_required(VERSION 3.10)
project(test)

set(CMAKE_CXX_STANDARD 14)
find_package(LLVM REQUIRED)

add_executable(test test.cpp)
target_include_directories(test PRIVATE ${LLVM_INCLUDE_DIR})
target_link_libraries(test LLVMCore LLVMPasses LLVMIRReader LLVMSupport)
if(APPLE)
  find_library(LTO_Library LTO HINTS ${LLVM_LIBRARY_DIR})
  if(LTO_Library)
    set_target_properties(test PROPERTIES LINK_OPTIONS "-Wl,-lto_library,${LTO_Library}")
  endif()
endif()

But I imagine you can do better if you took more than the few minutes I spent putting this together.

In any case, even if I am mistaken about how trivial the workarounds are: I still stand by my initial position that not requiring them to link with static libraries is only nice-to-have.

@alexreinking
Copy link
Member

alexreinking commented Sep 29, 2022

I don't even know why you're linking with a static LLVM library when Halide_SHARED_LLVM is set

We are not. The source of my belief is what you said here:

but configuring cmake with

-DHalide_SHARED_LLVM=ON -DCMAKE_SHARED_LINKER_FLAGS=-Wl,-lto_library,"$(brew --prefix llvm)/lib/libLTO.dylib"

works.

I am glad to know that setting CMAKE_SHARED_LINKER_FLAGS is not required when setting Halide_SHARED_LLVM=ON.


Note that I said only that incorporating the necessary build flags into your Makefiles or CMakeLists.txts could be done portably, and not necessarily that this would be trivial.

That is not what you said. Quoting you, again,

there are multiple trivial workarounds you can use that can even be encoded portably in a CMakeLists.txt or Makefile if desired

Are you arguing that encoding a "trivial workaround" portably may not itself be trivial? If so, maybe the workaround isn't so trivial in the first place...


What is trivial are the end-user workarounds.

We must have entirely different mental models of what end-users will consider "trivial".

  1. Switching compilers is not trivial for existing projects (which might rely on compiler-specific behavior). AppleClang and upstream Clang are very much not interchangeable.
  2. Switching linkers is not trivial for existing projects (which might rely on linker-specific features).
  3. Forcing users to set special linker flags globally, even on link steps not involving LLVM, is not justifiable.

I still stand by my initial position that not requiring them to link with static libraries is only nice-to-have.

Is isn't for our users because they might want to write cross-platform software (which LLVM and Halide both are), but on Windows, LLVM does not support shared linking, full stop. So for many users, being able to link statically isn't merely nice-to-have, it's a hard technical requirement.


Here's another:

The if (APPLE) check will break builds that don't use the Homebrew-built LLVM, notably our own CI. What code do you imagine will correctly anticipate the need for attaching Homebrew's LTO library? Unless there is an easy way to check ahead of time, then the change in Homebrew is no less than a forced toolchain change to all downstreams.

@carlocab
Copy link

carlocab commented Sep 29, 2022

I don't know how to make this any more plain. You're arguing that encoding a "trivial workaround" portably is not itself trivial? Maybe the workaround isn't so trivial in the first place...

Of course there is a distinction between passing flags to your build and handling them generically with [c]make. CMake and Make are notorious for being uncooperative and making simple things difficult. That's how Meson is able to carve out its niche.

That said, we don't have to agree on what is and isn't trivial:

In any case, even if I am mistaken about how trivial the workarounds are: I still stand by my initial position that not requiring them to link with static libraries is only nice-to-have.

The if (APPLE) check will break builds that don't use the Homebrew-built LLVM, notably our own CI.

What is the error message? Is your CI cross-compiling?

What code do you imagine will correctly anticipate the need for attaching Homebrew's LTO library?

Check the output of the --version flag for nearly any executable in LLVM's bin directory:

❯ ./clang --version
Homebrew clang version 15.0.1
[snip]
❯ ./ld64.lld --version
Homebrew LLD 15.0.1
❯ ./llvm-ar --version
Homebrew LLVM version 15.0.1
[snip]
❯ ./FileCheck --version
Homebrew LLVM version 15.0.1
[snip]

then the change in Homebrew is no less than a forced toolchain change to all downstreams

Only if you need the static libraries. This applies to a small enough set of users that it just doesn't outweigh the benefit to all the others.

@alexreinking
Copy link
Member

That's how Meson is able to carve out its niche.

I'll bite... how does Meson handle this issue differently than (C)Make?

@steven-johnson
Copy link
Contributor Author

At this point it seems clear that Homebrew is taking a direction that doesn't work well with Halide, and that direction isn't likely to change anytime soon, so I suspect that the right answer here is for Halide to find an alternate recommendation for OSX users who want to install current versions of LLVM.

@carlocab
Copy link

At this point it seems clear that Homebrew is taking a direction that doesn't work well with Halide, and that direction isn't likely to change anytime soon, so I suspect that the right answer here is for Halide to find an alternate recommendation for OSX users who want to install current versions of LLVM.

This makes sense.

I apologise for the breakage. If fixing it came for free, other than the time I put into it, I'd fix it. But it doesn't.

However, if there are issues beyond linking with static libraries, those will need to be fixed. Feel free to file an issue at https://github.com/Homebrew/homebrew-core/issues, if you still have users who use our build of LLVM.

@abadams
Copy link
Member

abadams commented Sep 29, 2022

If I understand correctly, the thing that was removed was machine-code versions of llvm static libraries. Now the only versions that ship contain bitcode instead, and are useless to, e.g. gcc.

Sounds like we should to change our instructions to say that if you use homebrew, you must either use homebrew llvm as your toolchain, or only use Halide as a shared library.

Maybe this is silly for some reason, but why not add a homebrew llvm static library package that contains actual machine code rather than bitcode?

@alexreinking
Copy link
Member

Maybe this is silly for some reason, but why not add a homebrew llvm static library package that contains actual machine code rather than bitcode?

Indeed this seems like a possible compromise because, after a conversation I had with Thiago Macieira, it seems that LTO static libraries can only be linked by the exact compiler build (not version) that produced them. That is, passing -Wl,-lto_library,"$(brew --prefix llvm)/lib/libLTO.dylib to AppleClang is not guaranteed to work.

@eli-schwartz
Copy link

eli-schwartz commented Sep 30, 2022

I'll bite... how does Meson handle this issue differently than (C)Make?

For the last few days, Meson's own internal unit test "frameworks: 15 llvm (method=config-tool link-static=True)", which attempts to find llvm (using llvm-config) and link to it statically, fails with the same error as the original bug report here. (Shared linking works fine.)

So I guess the answer is that Meson handles this exactly like CMake does, and that's why Meson too is broken.

But perhaps people only specifically ask for static libraries in unittests of a build system, who knows. :)

@alexreinking
Copy link
Member

But perhaps people only specifically ask for static libraries in unittests of a build system, who knows. :)

They also ask for them pretty consistently in all sorts of deployment scenarios. Very many (a majority?) of our customers statically link LLVM's libraries to ours.

Again, based on my conversation with Thiago, I think the compilers are doing the right thing by rejecting these binaries. The only toolchain with which it is reasonable to expect them to work is the exact toolchain that created them. Hacks to trick the compiler into accepting them might well lead to corrupted builds.

Between Halide and Meson both having issues with this change, maybe it should yet be reconsidered...

@carlocab
Copy link

carlocab commented Sep 30, 2022

That is, passing -Wl,-lto_library,"$(brew --prefix llvm)/lib/libLTO.dylib to AppleClang is not guaranteed to work.

This would be a surprise to me, because this is essentially the mechanism by which LTO bootstrap builds on Darwin worked for years.

In fact there are some hacks in LLVM's CMake scripts that are specifically designed to force host tools (e.g. Apple's libtool) to use a just-built libLTO.dylib. grep for DYLD_LIBRARY_PATH, ignore any test directories for an example.

Not to mention that the just-built clang driver also passes the equivalent of -lto_library "$(brew --prefix llvm)/lib/libLTO.dylib" to Apple's ld when linking.

Of course, you could be of the view that it's okay when clang-15 does it, but not when you tell Apple clang to because reasons, but any reasons I can imagine for this are not consistent with modularity of the toolchain. I'm happy to be enlightened here, though.

Maybe this is silly for some reason, but why not add a homebrew llvm static library package that contains actual machine code rather than bitcode?

No, not silly at all. I quite like this idea, and will look into it.

However, there may be something even better available. It appears Fedora also build their toolchain with ThinLTO, but somehow they take the bitcode inside the static archives and transform them into ELF during the build process.

I don't know yet how Fedora does this, or whether it can be translated to MachO (and if so, how), but this seems to me to be the best solution here.

Edit: This is how Fedora does it: https://src.fedoraproject.org/rpms/redhat-rpm-config/blob/rawhide/f/brp-llvm-compile-lto-elf It seems like doing the exact same thing on macOS should work, but I'll need a bit to experiment with it.

@carlocab
Copy link

carlocab commented Oct 2, 2022

Implemented in Homebrew/homebrew-core#112154. Building Halide from the tip of the main branch succeeds when LLVM is rebuilt with those changes.

carlocab added a commit to carlocab/homebrew-core that referenced this issue Oct 2, 2022
In Homebrew#106925, I enabled LTO for our LLVM build. This creates static
archives that contain LLVM bitcode instead of object code. This makes
the static archives more difficult to use, requiring workarounds such as

    https://github.com/Homebrew/homebrew-core/blob/c01f1794fc3decce04b71cae03966213fc7af34d/Formula/enzyme.rb#L30

and has caused problems for multiple downstream projects.

We can fix this by converting the bitcode into object code, which is
what Fedora does with their LLVM build. They also build their toolchain
with LTO.

Alternatively, we can disable LTO, but that foregoes significant
speedups we get from enabling it.

While we're here, let's add some test coverage for features that were
recently enabled that we don't test.

Fixes:

ziglang/zig#12923
halide/Halide#7055
mesonbuild/meson#10879
Homebrew/discussions#3666
BrewTestBot pushed a commit to Homebrew/homebrew-core that referenced this issue Oct 3, 2022
In #106925, I enabled LTO for our LLVM build. This creates static
archives that contain LLVM bitcode instead of object code. This makes
the static archives more difficult to use, requiring workarounds such as

    https://github.com/Homebrew/homebrew-core/blob/c01f1794fc3decce04b71cae03966213fc7af34d/Formula/enzyme.rb#L30

and has caused problems for multiple downstream projects.

We can fix this by converting the bitcode into object code, which is
what Fedora does with their LLVM build. They also build their toolchain
with LTO.

Alternatively, we can disable LTO, but that foregoes significant
speedups we get from enabling it.

While we're here, let's add some test coverage for features that were
recently enabled that we don't test.

Fixes:

ziglang/zig#12923
halide/Halide#7055
mesonbuild/meson#10879
Homebrew/discussions#3666

Closes #112154.

Signed-off-by: Sean Molenaar <1484494+SMillerDev@users.noreply.github.com>
Signed-off-by: BrewTestBot <1589480+BrewTestBot@users.noreply.github.com>
@steven-johnson
Copy link
Contributor Author

Fixed, thanks @carlocab!

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

No branches or pull requests

5 participants