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

cmake: LLVM needs to link against zlib #8932

Merged
merged 5 commits into from
Jun 3, 2021
Merged

cmake: LLVM needs to link against zlib #8932

merged 5 commits into from
Jun 3, 2021

Conversation

andrewrk
Copy link
Member

Without this, LLVM is crippled. This makes building Zig on Windows more
involved and is probably going to lead to more issues filed.

For more details see
ziglang/zig-bootstrap#57

@daurnimator
Copy link
Contributor

Is this a static-linking-only dependency? (i.e. the dynamic LLVM library would already depend on libz?)

@andrewrk
Copy link
Member Author

Yes

CMakeLists.txt Outdated
@@ -713,11 +713,14 @@ set_target_properties(zigcpp PROPERTIES
COMPILE_FLAGS ${EXE_CFLAGS}
)

find_library(ZLIB NAMES libz.a z zlib libz)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is libz.a first? shouldn't the following z find it anyway?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied this line from another place in the CMakeLists.txt in order to quickly get this working to test the zig-bootstrap changes. This could probably be cleaned up (I'm no cmake expert) and should probably be put behind some ZIG_USE_ZLIB flag which is automatically enabled if ZIG_STATIC is true.

@kubkon
Copy link
Member

kubkon commented May 29, 2021

One thing to keep in mind here: if this forces linking of -lz on macOS and LLVM links against symbols from libz, then macOS binary will be malformed. This will be until I add ability to parse "tbd"s in zld. Until then, might it make sense to feature-gate libz so that it is not pulled into macOS?

@kubkon
Copy link
Member

kubkon commented May 29, 2021

One thing to keep in mind here: if this forces linking of -lz on macOS and LLVM links against symbols from libz, then macOS binary will be malformed. This will be until I add ability to parse "tbd"s in zld. Until then, might it make sense to feature-gate libz so that it is not pulled into macOS?

I just read ziglang/zig-bootstrap#58 and since we are building libz ourselves and linking it in statically, I think we should be good on macOS without any special feature-gating or whatnot. It would only be a problem if libz was a system lib on a current macOS and only a tbd was available. Then, we would not correctly link libz since zig ld currently doesn't understand text-based definition files.

@nektro
Copy link
Contributor

nektro commented May 29, 2021

this is also presumably only a temporary measure until #8726 is complete

edit: that issue was placed a lot farther out than i remembered

For more details on why this dependency is needed, see
ziglang/zig-bootstrap#57
@andrewrk
Copy link
Member Author

I really don't understand how this is causing a segfault on freebsd while making aarch64-linux print "zig0: not found"

@andrewrk
Copy link
Member Author

(lldb) run src/stage1.zig -target x86_64-freebsd-gnu -mcpu=baseline --name zig1 --override-lib-dir /usr/home/build/zig/lib -femit-bin=/usr/home/build/zig/build/zig1.o -OReleaseFast --strip -lc --pkg-begin build_options /usr/home/build/zig/build/config.zig --pkg-end --pkg-begin compiler_rt /usr/home/build/zig/lib/std/special/compiler_rt.zig --pkg-end
Process 1585 launching
Process 1585 launched: '/usr/home/build/zig/build/zig0' (x86_64)
Process 1585 stopped
* thread #1, name = 'zig0', stop reason = signal SIGSEGV: invalid address (fault address: 0x0)
    frame #0: 0x0000000000000000
error: Bad address
(lldb) up
error: Already at the top of the stack.

🤷‍♂️

@andrewrk
Copy link
Member Author

I'm guessing that we're incorrectly finding zlib on these systems but it's invalid for whatever reason and if we provide our own build it will work fine. Just need to update the CI tarballs to test this.

to llvm 12.0.1-rc1, and -DLLVM_ENABLE_ZLIB=FORCE_ON
@andrewrk
Copy link
Member Author

I have no idea why drone CI is failing. It works fine with the bootstrap repository.

@mikdusan
Copy link
Member

mikdusan commented May 30, 2021

I really don't understand how this is causing a segfault on freebsd while making aarch64-linux print "zig0: not found"

  • examine zig0 binary with this PR on local freebsd 13.0: debug+static build of zig:
$ file zig0
zig0: ELF 64-bit LSB executable, x86-64, version 1 (FreeBSD), dynamically linked, for FreeBSD 13.0 (1300139), FreeBSD-style, with debug_info, not stripped
$ ldd zig0
zig0:
zig0: signal 11
  • notable is that it is a dynamic exe
  • here are the interesting last args to the c++ -static ... command to produce zig0:
-lrt -lexecinfo -lpthread -lm -Wl,-Bstatic -lz -Wl,-Bdynamic -lpthread -lm -Wl,-Bstatic -lz -Wl,-Bdynamic
  • here is what those same args look like on master:
-lrt  -lexecinfo  -lpthread  -lm  -lz  -lpthread  -lm  -lz
  • and after applying the same args to this PR's build (i.e. getting rid of that -Wl,-Bstatic and -Bdynamic nonsense) some sane results appear:
$ file zig0
zig0: ELF 64-bit LSB executable, x86-64, version 1 (FreeBSD), statically linked, for FreeBSD 13.0 (1300139), FreeBSD-style, with debug_info, not stripped
$ ldd zig0
ldd: zig0: not a dynamic ELF executable

That first zig0 executable is not a proper dynamic executable yet it tries to be: c++ -static means no ability to use dynamic libraries. Subsequent -Wl,-Bstatic are probably harmless, but -Wl,-Bdynamic is likely the culprit.

edit: it stands to reason that the zig0: not found error is also the same issue, instead of SEGV that elf platform gets further but cannot find/load a .so because bogus dynamic exe

to llvm 12.0.1-rc1, and -DLLVM_ENABLE_ZLIB=FORCE_ON
@kubkon
Copy link
Member

kubkon commented May 30, 2021

FYI, I've updated macos tarballs to the latest zig-bootstrap, pushed them to our s3 instance and updated the CI scripts. Fingers crossed everything is going to work out fine!

@andrewrk
Copy link
Member Author

Thanks @kubkon!

Based on @mikdusan's comment I expect FreeBSD to be fixed based solely on an updated tarball, which I will do today. And then it's just the Drone CI mystery left to solve. The nuclear option would be to stop trying to get alpine linux to build our tarballs, but instead only use it to test, and then use the bootstrap repository to provide the aarch64-linux tarballs. This is probably a good idea; arguably all the Zig tarballs we provide on the download page should be via zig-bootstrap.

@kubkon
Copy link
Member

kubkon commented May 30, 2021

Thanks @kubkon!

Based on @mikdusan's comment I expect FreeBSD to be fixed based solely on an updated tarball, which I will do today. And then it's just the Drone CI mystery left to solve. The nuclear option would be to stop trying to get alpine linux to build our tarballs, but instead only use it to test, and then use the bootstrap repository to provide the aarch64-linux tarballs. This is probably a good idea; arguably all the Zig tarballs we provide on the download page should be via zig-bootstrap.

I definitely agree with the last bit: all the Zig tarballs we provide on the download page should be via zig-bootstrap.

to llvm 12.0.1-rc1, and -DLLVM_ENABLE_ZLIB=FORCE_ON
@andrewrk
Copy link
Member Author

andrewrk commented Jun 2, 2021

FreeBSD tarball is updated; let's see if it passes. I also created the aarch64-musl tarball (zig+llvm+lld+clang-aarch64-linux-musl-0.8.0-dev.2703+c12704a33.tar.xz) but need to rework the docker image to use it which will have to wait until my mind is fresh tomorrow.

@andrewrk andrewrk force-pushed the llvm-needs-zlib branch 5 times, most recently from bcc464d to 98afc04 Compare June 2, 2021 23:54
@andrewrk
Copy link
Member Author

andrewrk commented Jun 3, 2021

whew, I think this may finally be in a passing state.

@andrewrk andrewrk merged commit 87dae0c into master Jun 3, 2021
@andrewrk andrewrk deleted the llvm-needs-zlib branch June 3, 2021 06:13
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

Successfully merging this pull request may close these issues.

6 participants