Page MenuHomePhabricator

build: reduce CMake handling for zlib
ClosedPublic

Authored by compnerd on Nov 26 2019, 9:21 PM.

Details

Summary

Rather than handling zlib handling manually, use find_package from CMake to find zlib properly. Use this to normalize the LLVM_ENABLE_ZLIB, HAVE_ZLIB, HAVE_ZLIB_H. Furthermore, require zlib if LLVM_ENABLE_ZLIB is set to YES, which requires the distributor to explicitly select whether zlib is enabled or not. This simplifies the CMake handling and usage in the rest of the tooling.

Diff Detail

Event Timeline

compnerd created this revision.Nov 26 2019, 9:21 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptNov 26 2019, 9:21 PM
Herald added subscribers: lldb-commits, Restricted Project, hiraditya, mgorny. · View Herald Transcript
labath added a subscriber: labath.Nov 26 2019, 11:42 PM

Please take a look at D70519 for the issues with this approach.

Also, while I do agree with you that we should not auto-select dependencies, I think this runs contrary to the llvm philosophy that the default built should "just work" (I don't know if that's formalized anywhere, but I know it's certainly what (some?) people expect).

compnerd added a comment.EditedNov 27 2019, 12:39 PM

@labath I think you are misunderstanding the patch. This is not autoselecting the dependencies. It is simply doing that based on an existing option that we have - LLVM_ENABLE_ZLIB. We could always search for zlib and override the results with LLVM_ENABLE_ZLIB as well. The current build will continue to just work - zlib is used only for the compressed debug sections (which requires the user to opt-in).

Having one canonical variable controlling zlib support seems indeed desirable.

With this patch, what is the output of llvm-config --system-libs ?

@compnerd What's the answer to this for this patch?

Having one canonical variable controlling zlib support seems indeed desirable.

With this patch, what is the output of llvm-config --system-libs ?

@compnerd What's the answer to this for this patch?

Sorry, didn't see the question. From my local build:

 $ ./bin/llvm-config --system-libs
/usr/lib/x86_64-linux-gnu/libz.so -lrt -ldl -ltinfo -lpthread -lm /usr/lib/x86_64-linux-gnu/libxml2.so

Having one canonical variable controlling zlib support seems indeed desirable.

With this patch, what is the output of llvm-config --system-libs ?

@compnerd What's the answer to this for this patch?

Sorry, didn't see the question. From my local build:

 $ ./bin/llvm-config --system-libs
/usr/lib/x86_64-linux-gnu/libz.so -lrt -ldl -ltinfo -lpthread -lm /usr/lib/x86_64-linux-gnu/libxml2.so

So that's the issue Pavel raised in the other review. Can we make this return -lz with the current approach?

/usr/lib/x86_64-linux-gnu/libz.so is definitely better than -l/usr/lib/x86_64-linux-gnu/libz.so, as it's at least a valid link line. It's not completely equivalent, and may not work in all cases -- last time I accidentally changed this, I got about a dozen emails from people I broke, and I wouldn't be surprised if one of them depended on the subtle differences here. However, the fact that libxml is also spelled out this way gives me some hope...

That said, this still leaves us with the "problem" that the default cmake configuration will be broken for people who don't have zlib installed (pretty much everyone on windows, at least). You can try that out by deleting/renaming/uninstalling zlib.h from your system. With this patch you get a configuration error whereas previously zlib support was automatically disabled. Whether this "problem" is a problem or "working as intended" depends on what do you expect out of your build system, but it's definitely not consistent with how other llvm dependencies are managed.

Anyway, I don't really have a horse in this race, but I figured I should at least warn you about the problems you're likely to run into. (and I really wish @beanz would say something here, as he's the one who's complaining about build system inconsistencies...)

beanz added a comment.Dec 4 2019, 1:28 PM

@labath sorry for not replying on-list. I've actually been discussing offline with @compnerd. If llvm-config is printing an absolute path, I'm concerned about how that will interact with binary package distributions for the llvm-dev packages. Not sure if @tstellar has any thoughts on that.

beanz added a comment.Dec 4 2019, 2:44 PM

I conferred off-list with @compnerd. He and I talked through my concerns, and I believe this patch is fine as-is, so LGTM.

beanz accepted this revision.Dec 17 2019, 9:46 AM

I think this is good, and it has been sitting a while.

This revision is now accepted and ready to land.Dec 17 2019, 9:46 AM

this is now in master, and I am seeing build failures in cross-building clang, e.g. when building clang for arm on a x86_64 host. its resorting to finding, libz from buildhost instead of target sysroot ( using --sysroot) and failing in link step. e.g.

FAILED: bin/llvm-config
...
.... -o bin/llvm-config -Wl,-rpath,"\$ORIGIN/../lib" lib/libLLVMSupport.a /usr/lib/libz.so -lrt -ldl -ltinfo -lm lib/libLLVMDemangle.a
...

aarch64-yoe-linux-musl/aarch64-yoe-linux-musl-ld: /usr/lib/libz.so: error adding symbols: file in wrong format
clang-10: error: linker command failed with exit code 1 (use -v to see invocation)

you can see that its adding /usr/lib/libz.so to linker cmdline while cross linking.

this is now in master, and I am seeing build failures in cross-building clang, e.g. when building clang for arm on a x86_64 host. its resorting to finding, libz from buildhost instead of target sysroot ( using --sysroot) and failing in link step. e.g.

FAILED: bin/llvm-config
...
.... -o bin/llvm-config -Wl,-rpath,"\$ORIGIN/../lib" lib/libLLVMSupport.a /usr/lib/libz.so -lrt -ldl -ltinfo -lm lib/libLLVMDemangle.a
...

aarch64-yoe-linux-musl/aarch64-yoe-linux-musl-ld: /usr/lib/libz.so: error adding symbols: file in wrong format
clang-10: error: linker command failed with exit code 1 (use -v to see invocation)

you can see that its adding /usr/lib/libz.so to linker cmdline while cross linking.

Have you set CMAKE_SYSROOT appropriately?

this is now in master, and I am seeing build failures in cross-building clang, e.g. when building clang for arm on a x86_64 host. its resorting to finding, libz from buildhost instead of target sysroot ( using --sysroot) and failing in link step. e.g.

FAILED: bin/llvm-config
...
.... -o bin/llvm-config -Wl,-rpath,"\$ORIGIN/../lib" lib/libLLVMSupport.a /usr/lib/libz.so -lrt -ldl -ltinfo -lm lib/libLLVMDemangle.a
...

aarch64-yoe-linux-musl/aarch64-yoe-linux-musl-ld: /usr/lib/libz.so: error adding symbols: file in wrong format
clang-10: error: linker command failed with exit code 1 (use -v to see invocation)

you can see that its adding /usr/lib/libz.so to linker cmdline while cross linking.

Have you set CMAKE_SYSROOT appropriately?

That is not the problem. Since in cases of target its finding is properly.
there are multiple pieces where some are being built for native( build host) some for target so it thinks its building for
buildhost (native) llvm-config e.g. is one such case. Replacing it with -lz fixes it becasue compile environment is providing
right --sysroot for native or cross cases and it always links with correct libs.

hans added a subscriber: hans.Mar 3 2020, 2:06 AM

There were some concerns raised on this patch, and also in PR44780.

I think at this point it's safer to revert these changes and start over. I've pushed the revert in 916be8fd6a0a0feea4cefcbeb0c22c65848d7a2e and will merge that to 10.x after it's baked a little.